Commit 8ee23cc
authored
fix(rocalution): add missing break in complex32 case of rocsparseio readers (ROCm#10425)
## Summary
Nine `switch` statements in
`projects/rocalution/src/base/host/host_io.cpp` pick
the conversion to apply when a matrix file's value type differs from the
in-memory `ValueType`. In all nine, the `complex32` case has a body but
no
`break`, so control falls into the `complex64` case:
```cpp
case rocsparseio_type_complex32:
{
copy_mixed_arrays(nnz, val[0], (const std::complex<float>*)tmp_val);
}
case rocsparseio_type_complex64:
{
copy_mixed_arrays(nnz, val[0], (const std::complex<double>*)tmp_val);
break;
}
```
The result is a heap buffer over-read plus silent corruption of the
values that
were just converted correctly.
Related: found during the same monorepo sweep as ROCm#10413.
## Why it is a bug
Three facts combine, all in-tree:
1. **The scratch buffer is sized from the file's type, not the in-memory
type.**
`host_io.cpp:918-922`:
```cpp
size_t sizeof_val_type;
status = rocsparseio_type_get_size(file_val_type, &sizeof_val_type);
tmp_val = malloc(nnz * sizeof_val_type);
```
and the `switch` is on `file_val_type`, so reaching the `complex32` case
means the buffer is `nnz * 8` bytes.
2. **The two widths differ by 2x.** `src/utils/rocsparseio.hpp:415-421`:
```cpp
case complex32: return sizeof(float) * 2; // 8
case complex64: return sizeof(double) * 2; // 16
```
3. **`copy_mixed_arrays` reads `size` elements of its source type.**
`host_io.cpp:656-667`:
```cpp
for(size_t i = 0; i < size; ++i)
{
x[i] = static_cast<X>(y[i]);
}
```
So the fallthrough reads `nnz * 16` bytes from an `nnz * 8` byte
allocation: an
`nnz * 8` byte over-read. It then writes those garbage values over
`val[0]`,
discarding the correct conversion the `complex32` case had already
performed.
## Reachability, stated precisely
Exactly one configuration reaches the fallthrough: **a file whose value
type is
`complex32`, read into `ValueType` `std::complex<double>`.** I checked
the other
possibilities rather than assuming:
| `ValueType` | What happens | Reaches fallthrough |
| --- | --- | --- |
| `std::complex<float>` | `required_val_type == complex32 ==
file_val_type`, so `same_val_type` is true and the whole `switch` is
skipped | no |
| `std::complex<double>` | `complex32` case runs the real
`complex<double> <- complex<float>` conversion, then falls through |
**yes** |
| `int8_t`, `float`, `double` | the `complex32` case hits a
`copy_mixed_arrays` specialization whose body is `throw 1;`, so it
throws first | no |
The throwing specializations are at `host_io.cpp:696-731`.
`std::complex<double>`
is explicitly instantiated for every one of these readers
(`host_io.cpp:4241`,
`4303`, `4361`, `4434`, `4504`, and the CSR/MCSR/BCSR equivalents), so
the path
is compiled into the shipping library. It is the ordinary
mixed-precision case
of loading a single-precision complex matrix into a double-precision
complex one.
I want to be straight that this is one specific type pairing rather than
"any
mismatched complex read". It is still a live memory-safety defect on a
supported
and instantiated path.
## Scope
Affected readers, one site each except `hyb` which has two (the COO
value array
and the ELL value array):
```
host_io.cpp:1022 read_matrix_csr_rocsparseio switch on file_val_type
host_io.cpp:1330 read_matrix_mcsr_rocsparseio switch on file_val_type
host_io.cpp:1681 read_matrix_bcsr_rocsparseio switch on file_val_type
host_io.cpp:1967 read_matrix_coo_rocsparseio switch on file_val_type
host_io.cpp:2231 read_matrix_dia_rocsparseio switch on file_val_type
host_io.cpp:2488 read_matrix_ell_rocsparseio switch on file_val_type
host_io.cpp:2858 read_matrix_hyb_rocsparseio switch on file_coo_val_type
host_io.cpp:2931 read_matrix_hyb_rocsparseio switch on file_ell_val_type
host_io.cpp:3123 read_matrix_dense_rocsparseio switch on file_val_type
```
All nine switches have identical shape. Every case group ends in `break`
except
`complex32`:
```
switch@996 on file_val_type:
break case int32+int64
break case int8
break case float32
break case float64
NO-EXIT case complex32
break case complex64
```
**`complex32` also appears 13 more times as a stacked case label sharing
a body
with its neighbours**, for example `host_io.cpp:955-962`:
```cpp
case rocsparseio_type_int8:
case rocsparseio_type_float32:
case rocsparseio_type_float64:
case rocsparseio_type_complex32:
case rocsparseio_type_complex64:
{
break;
}
```
Those are correct and are **not** touched. Only the nine cases that
carry their
own body are changed. A first pass of my own scan counted those 13 as
defects
before I distinguished stacked labels from bodies, so the number in this
PR is 9,
not 22.
## Fix
`break;` added to the nine `complex32` bodies. Nine lines, no other
change:
```diff
case rocsparseio_type_complex32:
{
copy_mixed_arrays(nnz, val[0], (const std::complex<float>*)tmp_val);
+ break;
}
```
## Verification
No AMD GPU was used, and none is needed: this is host-side matrix-file
I/O. I
built a reduced standalone reproducer modelling the buffer sizing, the 8
versus
16 byte type widths, the `copy_mixed_arrays` overload set including the
throwing
specializations, and the `switch` itself, on the reachable
`ValueType = std::complex<double>` instantiation. `nnz = 4`, so
`tmp_val` is 32
bytes and the fallthrough reads 64.
**Before, `clang++ -std=c++17 -g -O0 -fsanitize=address`:**
```
tmp_val = malloc(4 * 8) = 32 bytes
after complex32 case: val[0] = (1,10) (2,11) (3,12) (4,13) <-- correct
=================================================================
==32738==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000001c50 at pc 0x000103082c84 bp 0x00016d5d6120 sp 0x00016d5d58d0
READ of size 16 at 0x603000001c50 thread T0
#0 0x000103082c80 in __asan_memcpy+0x400 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3ec80)
#1 0x0001028290f0 in void copy_mixed_arrays<std::__1::complex<double>, std::__1::complex<double>>(unsigned long, std::__1::complex<double>*, std::__1::complex<double> const*) repro.cpp:53
ROCm#2 0x000102828b58 in main repro.cpp:129
ROCm#3 0x00018c759d50 in start+0x1c0c (dyld:arm64e+0x8d50)
0x603000001c50 is located 0 bytes after 32-byte region [0x603000001c30,0x603000001c50)
allocated by thread T0 here:
#0 0x000103085164 in malloc+0x78 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x41164)
#1 0x000102828980 in main repro.cpp:82
ROCm#2 0x00018c759d50 in start+0x1c0c (dyld:arm64e+0x8d50)
```
Note the frame: the over-read is attributed to the
`copy_mixed_arrays<complex<double>, complex<double>>` instantiation,
which is the
generic template reached only through the fallthrough. At `-O1` the loop
vectorises and ASan reports the same overflow as a single `READ of size
64`.
**Before, no sanitizer**, showing the second half of the defect:
```
tmp_val = malloc(4 * 8) = 32 bytes
after complex32 case: val[0] = (1,10) (2,11) (3,12) (4,13) <-- correct
after complex64 case: val[0] = (524288,1.04858e+06) (2.09715e+06,4.19431e+06) (0,0) (0,0) <-- clobbered
done
```
The correct values are replaced by the bit patterns of pairs of
`std::complex<float>` reinterpreted as single `std::complex<double>`
values,
trailed by the out-of-bounds tail.
**After, `-DFIXED`, ASan clean at both `-O0` and `-O1`:**
```
tmp_val = malloc(4 * 8) = 32 bytes
after complex32 case: val[0] = (1,10) (2,11) (3,12) (4,13) <-- correct
done
EXIT=0
```
### Why no compiler caught it
`-Wall -Wextra` are silent. `-Wimplicit-fallthrough` does flag it, but
clang does
not imply it from either, and rocALUTION's CMake never enables it.
Turning it on
for `projects/rocalution` would prevent recurrence, and I am happy to do
that as
a follow-up if you want it separated from this fix.
## A note on the unit test policy check
`tools/libraries_pr_bot/policy.yml` requires an accompanying test file
for `.cpp`
changes, with `exempt_paths: []`. I have not added one, and I would
rather explain
why than route around the check.
`projects/rocalution/clients/tests/` has 31 gtest files and **none of
them cover
file I/O at all**: `git grep -l
"ReadFileRSIO\|WriteFileRSIO\|rocsparseio" --
projects/rocalution/clients/` returns nothing. So there is no existing
test file
to extend, and a new one needs a rocsparseio fixture written at a
complex32 value
type, a new entry in `clients/tests/CMakeLists.txt`, and
`init_rocalution()`. I
cannot build or run the rocALUTION client suite here, and shipping
unexercised
CMake and gtest code alongside a memory-safety fix would make this PR
harder to
trust, not easier.
Two things I am happy to add, whichever you prefer:
1. A `WriteFileRSIO` then `ReadFileRSIO` round-trip test in a new
`clients/tests/test_host_io.cpp`, writing a small `complex<float>`
matrix and
reading it back into a `LocalMatrix<std::complex<double>>`, asserting
the
values match. That is the faithful regression test and it fails on
`develop`
under ASan. I would need someone to run it once, since I have no ROCm
runtime.
2. Enabling `-Wimplicit-fallthrough` for `projects/rocalution`, which
pins the
whole class rather than this instance.
I can also hand over the standalone reproducer if it is useful for the
review.
## Checklist
- Based on `develop` (`86a82b0`), single commit, DCO signed off.
- Re-verified against current `develop` HEAD rather than an earlier
snapshot: 9
affected sites, 13 stacked-label sites correctly excluded, all other
cases in
all nine switches confirmed to still end in `break`.
- Prior art checked: no open PR or issue mentions `host_io`,
`copy_mixed_arrays`
or `rocsparseio_type_complex32`, and `git log -S
'rocsparseio_type_complex32'`
shows no partial fix.
- `projects/rocalution` is **not** in the root `.pre-commit-config.yaml`
exclude
list, so `clang-format` 18.1.4, the pinned version, was run against
`projects/rocalution/.clang-format`. It reformats nothing beyond the
nine added
lines. No trailing whitespace, file still ends in a newline.
- Behaviour is unchanged for every configuration except the one that was
reading
out of bounds.
Signed-off-by: Aditya Singh <adisin650@gmail.com>1 parent 7bfd03f commit 8ee23cc
1 file changed
Lines changed: 9 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1022 | 1022 | | |
1023 | 1023 | | |
1024 | 1024 | | |
| 1025 | + | |
1025 | 1026 | | |
1026 | 1027 | | |
1027 | 1028 | | |
| |||
1330 | 1331 | | |
1331 | 1332 | | |
1332 | 1333 | | |
| 1334 | + | |
1333 | 1335 | | |
1334 | 1336 | | |
1335 | 1337 | | |
| |||
1681 | 1683 | | |
1682 | 1684 | | |
1683 | 1685 | | |
| 1686 | + | |
1684 | 1687 | | |
1685 | 1688 | | |
1686 | 1689 | | |
| |||
1967 | 1970 | | |
1968 | 1971 | | |
1969 | 1972 | | |
| 1973 | + | |
1970 | 1974 | | |
1971 | 1975 | | |
1972 | 1976 | | |
| |||
2231 | 2235 | | |
2232 | 2236 | | |
2233 | 2237 | | |
| 2238 | + | |
2234 | 2239 | | |
2235 | 2240 | | |
2236 | 2241 | | |
| |||
2488 | 2493 | | |
2489 | 2494 | | |
2490 | 2495 | | |
| 2496 | + | |
2491 | 2497 | | |
2492 | 2498 | | |
2493 | 2499 | | |
| |||
2858 | 2864 | | |
2859 | 2865 | | |
2860 | 2866 | | |
| 2867 | + | |
2861 | 2868 | | |
2862 | 2869 | | |
2863 | 2870 | | |
| |||
2931 | 2938 | | |
2932 | 2939 | | |
2933 | 2940 | | |
| 2941 | + | |
2934 | 2942 | | |
2935 | 2943 | | |
2936 | 2944 | | |
| |||
3123 | 3131 | | |
3124 | 3132 | | |
3125 | 3133 | | |
| 3134 | + | |
3126 | 3135 | | |
3127 | 3136 | | |
3128 | 3137 | | |
| |||
0 commit comments