Conversation
Closed PR commentHeads up that #26 will conflict with this one, and on a couple of details that may be worth folding in whenever this comes off WIP. The conflicts: #26 rewrites the Two things #26 found in code this PR touches:
On Not asking you to rebase across #26 — if it lands first I am happy to do that rebase myself. |
Closed PR CommentFollowed up on this properly rather than leaving it at "conflicts with #26". I audited every Two are genuine defects, and #26 now fixes both.
strcpy(command, "pigz");
strcat(command, " -n -f > \"");
strcat(command, nim->fname); /* unbounded */
strcat(command, "\"");
The other 145 calls are correct, which is why #26 does not rewrite them:
Rewriting 145 correct calls is 145 chances to get a length expression wrong, in code whose tests compare output byte for byte. I hit exactly that failure mode earlier in this work: a regex that looked right turned So #26 takes a different route to the same goal: This complements what you are doing rather than replacing it. Fortification catches a bad size at run time; Two things about the current implementation that would need sorting either way, repeating them here so they are not lost:
Happy to rebase #26 under this if you would rather land yours first — the two touch different lines apart from the |
|
@gdevenyi your LLM seems to be unaware that with Instead of making it redo everything I worked hard to do, could you instead use it to review the correctness of this PR? It seems to think 145 calls were correct, does it think they are still correct after the changes here? |
There was a problem hiding this comment.
Review by Claude Fable with effort high
Thanks for tackling this — the call-site conversions all look correct. I checked each fixed buffer against its declaration: extcopy[8] holds the 7-char extensions, extnii[8] + ".gz" fills exactly 8 with the NUL, magic[4] holds "n+1\0", and the strlen(...)+8 allocations cover extension + ".gz" + NUL. The QSTR macro change correctly adds memset to preserve the zero-padding strncpy used to provide.
The blockers are all in the new string_helper fallback (see line comments):
- The platform guard only compiles the fallback on glibc < 2.38, so MSVC/MinGW (no
__APPLE__, no__GLIBC__, nostrlcpy) get no definition at all and every call fails to link. Suggestcheck_symbol_exists(strlcpy string.h HAVE_STRLCPY)in CMake instead of enumerating platforms. string_helper.htests__GLIBC__before including any libc header, so the prototypes vanish if the header is included first.- The
strlcatfallback is unsafe underNDEBUGin a case where BSD's realstrlcatis safe.
Also noting (you already flagged the CMake wiring as unfinished): none of the modified .c files #include "string_helper.h" yet, so on pre-2.38 glibc the calls will be implicit declarations even once the file is compiled. Minor: the new files use tabs, // comments, and camelCase parameter names, which don't match the surrounding codebase.
| #ifndef __APPLE__ | ||
|
|
||
| // libc 2.38 and above provides strlcpy and strlcat, so don't redefine them. | ||
| #if defined (__GLIBC__) && ((__GLIBC__ < 2) || ((__GLIBC__ == 2) && (__GLIBC_MINOR__ < 38))) |
There was a problem hiding this comment.
This guard only provides the fallback when __GLIBC__ is defined and < 2.38. On MSVC/MinGW neither __APPLE__ nor __GLIBC__ is defined, so the whole block preprocesses away — and MSVC has no strlcpy, so every call site fails to link. This repo supports MSVC (there are _MSC_VER branches in doPigz).
Rather than enumerating platforms, detect the symbol in CMake:
include(CheckSymbolExists)
check_symbol_exists(strlcpy string.h HAVE_STRLCPY)and guard on #ifndef HAVE_STRLCPY. That also handles musl, the BSDs, and future libcs without further edits.
| #ifndef __APPLE__ | ||
|
|
||
| // libc 2.38 and above provides strlcpy and strlcat, so don't redefine them. | ||
| #if defined (__GLIBC__) && ((__GLIBC__ < 2) || ((__GLIBC__ == 2) && (__GLIBC_MINOR__ < 38))) |
There was a problem hiding this comment.
__GLIBC__ is only defined after a libc header has been included. If a .c file includes string_helper.h before <string.h>, this check sees __GLIBC__ undefined and the prototypes vanish even on old glibc. Add #include <string.h> at the top of this header, before the version check (or better, switch to a CMake HAVE_STRLCPY guard as suggested in the .c file).
|
|
||
| #include <stddef.h> | ||
|
|
||
| size_t strlcpy(char* restrict ioDestination, const char* restrict inSource, size_t inDestinationSize); |
There was a problem hiding this comment.
restrict is not a keyword in C++, and there's no extern "C" guard, so any C++ consumer (e.g. ITK) that includes this header will fail to compile. Wrap the declarations in extern "C" and hide restrict behind #ifdef __cplusplus.
| assert(destinationLength < inDestinationSize); | ||
|
|
||
| ioDestination += destinationLength; | ||
| inDestinationSize -= destinationLength; |
There was a problem hiding this comment.
If the destination is not NUL-terminated within inDestinationSize, the assert on line 41 is compiled out under NDEBUG, and this subtraction underflows to a huge value. The truncation branch below is then skipped and the memcpy writes past the buffer. OpenBSD's strlcat handles this case safely by returning size + strlen(src) without writing:
if (destinationLength >= inDestinationSize)
return inDestinationSize + sourceLength;All call sites in this PR are fine today, but a safety shim that can overflow where the real strlcat cannot defeats the purpose.
| strcat(command, "\""); | ||
| strlcpy(command, "pigz", sizeof(command)); | ||
| strlcat(command, " -n -f > \"", sizeof(command)); | ||
| strlcat(command, nim->fname, sizeof(command)); |
There was a problem hiding this comment.
Behavior change worth handling: the old strcat overflowed on a very long fname (~750+ chars); strlcat now silently truncates instead, which drops the closing quote and hands pigz a mangled command. Since strlcat returns the total length it tried to create, check it against sizeof(command) and fail (return -1) on truncation. Applies to all four copies (doPigz/doPigz2 in both nifti1_io.c and nifti2_io.c).
| strcat(elist[0], extgz); strcat(elist[1], extgz); strcat(elist[2], extgz); | ||
| strlcat(elist[0], extgz, 8); | ||
| strlcat(elist[1], extgz, 8); | ||
| strlcat(elist[2], extgz, 8); |
There was a problem hiding this comment.
The hardcoded 8 is correct today (extnii[8] + ".gz" fills it exactly), but sizeof(extnii) / sizeof(exthdr) / sizeof(extimg) would stay correct if the buffers ever change. Same in the nifti2_io.c copy.
axml_add_attrs() returns 1 when it cannot copy an attribute, but make_afni_xml() ignored the result, so the element was returned with a partial attribute list and nothing said so. Every failure the function reports is an allocation failure, so there is no case where continuing is right. Propagate it: epush() already treats a NULL from make_afni_xml() by entering a skip block, which is what the allocation-failure paths beside it do. axml_free_xml_t() releases what was built. It walks the attributes with attrs.length, which axml_add_attrs() lowers to the pair it failed on, so the half of that pair that was copied is freed and the entries past it are never read. That matters because the two arrays come from malloc(), not calloc(), so those entries hold uninitialized pointers.
nifti_read_n2_hdr() builds a nifti_image to convert an ASCII header and then released it with free(), which leaves its filename strings behind. No test reached that path, so the suite reported 362 of 362 either way. The fixture is the one the ASCII attribute test already uses, read through -disp_hdr2 rather than -disp_nim so the NIFTI-2 reader is the one exercised. The leak is visible to the memcheck and sanitizer legs; an ordinary build stays green with or without the fix.
znzread(), znzwrite() and nifti_read_buffer() all return size_t and
all returned -1 to report an error. In a size_t that value is
SIZE_MAX, larger than any length a caller can have asked for, so a
caller that tests the result with '<' reads the error as a complete
transfer:
ii = nifti_read_buffer(fp, nim->data, ntot, nim);
if( ii < ntot ){ ... } /* SIZE_MAX < ntot is false */
The visible effect is that a truncated image loads as if it were
whole: nifti_image_read() prints its short-read warning, ignores the
failure and returns an image whose tail is uninitialized heap.
FslReadVolumes() has the same problem one level up, dividing the
returned byte count by the volume size to report SIZE_MAX/volbytes
volumes read.
These now return 0, which every one of these callers already treats
as failure and which is the only value a size_t function has for
"nothing was transferred". The published return types in znzlib.h
and nifti1_io.h are unchanged.
The nifti2 equivalents return int64_t, where -1 is representable,
and are left alone.
Writes a 31x31x31 float image, copies it 1000 bytes short, and asserts nifti_image_read() returns NULL for the copy and a populated image for the original, so a blanket refusal cannot pass. Without the fix the truncated file comes back accepted with nvox=29791 and an uninitialized tail.
4f6573d to
b046eb1
Compare
afni_xml's parser holds sixteen levels of open elements in a fixed array and refuses to push a seventeenth by entering a skip block: epush() records the depth in xd->dskip and stops touching the stack until the matching pop. Two details of that bookkeeping are wrong. epush() overwrote dskip on every push past the limit, so the skip was recorded as starting at the innermost depth. epop() then cleared dskip before the stack was handled, so the pop that ends the skip fell through to "xd->stack[xd->depth-1] = NULL" and wrote a pointer eight bytes past the sixteen-entry array, which lives in the file-scope afni_xml_control. The next pop found dskip clear and dereferenced xd->stack[16]. epush() now keeps the outermost skip depth, since that is the depth whose pop ends the skip, and epop() clears dskip only after the element has been skipped. Depths within the limit are unaffected. The XML comes from the CIFTI extension of a NIfTI file, or from any file read by axml_read_file(), so the nesting is chosen by the input. Found by fuzzing axml_read_buf() with libFuzzer under AddressSanitizer.
process_popped_element() read xd->stack[xd->depth-1] and dereferenced it without a test. A slot is filled by the matching epush(), and an element that was skipped never fills one, so the read is only safe because the preceding commit keeps the skip running to the element that started it. Guard it rather than rely on that: check the depth is inside the stack, and that the slot holds a struct with a name, before the strcmp().
deep_nesting.xml nests twenty elements inside a CIFTI extension whose stack holds sixteen, which is the shape that walked off the end of xd->stack. The test drives it through cifti_tool and pins that the BrainModel sibling is still summarized, so a fix that refused the whole document would not pass.
A branch filter naming a branch that does not exist leaves the workflow configured but never run, which looks the same as one that runs and passes: no red check appears because no check appears at all. build.yml sat dead behind a filter naming main on a repository whose branch is master, and the only thing that found it was reading the file. The check judges a filter only when every entry is a literal name, so a release-* pattern or a branch that does not exist yet is left alone, and it reports a name that resolves to nothing rather than one that merely differs from the default.
cppcheck 2.19 nullPointerOutOfMemory: the uppercase copy of the name was written into an unchecked malloc result. Return -1, the same value the function already uses for an unrecognized name.
cppcheck 2.19 unreachableCode: FSLIOERR ends in exit(EXIT_FAILURE), so the return statements following it in FslReadAllVolumes and FslReadHeader can never run.
Fixes some clang-tidy bugprone-macro-parentheses warnings. Would be an issue where an argument that is an expression would evaluated different with different precedence.
Fixes many bugprone-implicit-widening-of-multiplication-result warnings. Here the multiplications were happening in small types (int, usually 32 bit) then stored in large types (size_t, usually 64 bit). The multiplication could have overflowed. Now the multiplication is done with large types and thus less likely to overflow.
disp_cifti_extension() searched for the CIFTI extension with
ext = nim->ext_list;
for( ind = 0; ind < nim->num_ext; ind++ )
if( ext->ecode == NIFTI_ECODE_CIFTI ) break;
ext is never advanced, so this tests the first extension num_ext times.
cifti_tool could only ever find a CIFTI extension that happened to be
first in the list; with any other extension ahead of it the tool
reported 'no CIFTI extension' for a file that has one. It now indexes
ext_list[ind] and leaves ext NULL when there is no match, which also
avoids the read past the end of the list that a bare ext++ would have
introduced.
The same function opened its output stream before the 'no CIFTI
extension' check and returned without closing it; the early return now
closes the stream like the normal path does.
cext_second_extension.nii carries a comment extension ahead of the CIFTI one. The test requires the extension's payload in the output and forbids the 'no CIFTI extension' message, and the existing unterminated-cext test still pins the first-position case, so neither a search that always matches nor one that never does would pass.
cdfbin() validates its `which` selector like this:
if(!(*which < 1 && *which > 4)) goto S30;
A value cannot be both less than 1 and greater than 4, so the condition
is always false, the negation is always true, and the jump to S30 is
always taken -- skipping the entire range check. gcc reports it as
-Wlogical-op, "logical 'and' of mutually exclusive tests is always
false".
The ten sibling functions in this file all spell the same guard with
`||`:
cdfbet 1527: if(!(*which < 1 || *which > 4)) goto S30;
cdfchi 2255: if(!(*which < 1 || *which > 3)) goto S30;
cdfchn 2553: if(!(*which < 1 || *which > 4)) goto S30;
... 7 more
so this is a single-character typo rather than an intentional deviation.
The effect is visible from the public API. cdfbin is declared in the
installed nifticdf.h, and its contract is that an out-of-range input sets
*status to -1 and *bound to the limit that was violated. Calling it with
which = 9:
before: status=999 bound=-999 (both left as the caller set them)
after: status=-1 bound=4 (the documented error return)
Before the fix the caller has no indication anything was wrong and the
function proceeds to compute with an unhandled selector.
The range check is reachable from the installed nifticdf.h, so assert the documented contract directly: which=9 and which=0 must set *status to -1 and *bound to the limit that was violated. A which=1 call is asserted to still return status 0, so a guard that rejected everything would not pass.
nifti_image_from_ascii() scans its input three times with
ii = sscanf( str+spos , "%1023s%n" , lhs , &nn ) ; spos += nn ;
if( ii == 0 || strcmp(lhs,"<nifti_image") != 0 ) return NULL ;
Two things are wrong with that pair of lines. sscanf() returns EOF,
not 0, when the input ends before anything is matched, so ii == 0 does
not detect the failure; and nn is added to spos before ii is looked
at, so the position advances by an indeterminate amount.
For a string holding nothing but whitespace the first scan returns
EOF, the test passes, and strcmp() reads the uninitialized 1024 byte
stack buffer. MemorySanitizer:
WARNING: MemorySanitizer: use-of-uninitialized-value
#0 nifti_image_from_ascii nifti2_io.c:8836
All three now test for the one successful conversion and only then
advance spos, which is what the surrounding code assumed. The two
inside the loop cannot fail today, because whitespace is skipped and
end of string is checked before each, and the in-tree readers gate on
has_ascii_header(), so the first is reached only through a direct call
to this published function. The same three lines exist in
nifti1_io.c and are corrected there too.
Found by fuzzing nifti_image_from_ascii() with libFuzzer under
MemorySanitizer.
19 -Wcast-align warnings, and behind them undefined behavior on any
target that cares about alignment.
modify_field() writes a value into a header field at a byte offset
parsed from a field table:
((short *)((char *)basep + field->offset))[fc] = (short)val;
field->offset is a byte offset into a packed on-disk header, so that
cast produces an address only correctly aligned by coincidence. The
same pattern appears for int, int64_t, float and double, in both tool
files. Each becomes a memcpy of the right width at the right byte
offset.
The second group reads a pointer back out of a structure through a
byte offset -- `sp = *(char **)((char *)str + fp->offset)` and the
nifti1_extension equivalents -- and becomes a memcpy into an aligned
local.
The third is nifti_header_version(), which cast its `const char * buf`
argument, a buffer straight off a file read with no alignment
guarantee, to both nifti_1_header * and nifti_2_header * and read
fields through them. It now copies into aligned locals first, exactly
the sizeof(nifti_1_header) bytes the function already checks are
present.
Verified by round-tripping int16, int32, int64, float32, float64 and
string fields through nifti_tool -mod_hdr2.
Nine -Wcast-qual warnings. Casting away const is how a read-only
contract turns into a write to memory the caller thought was safe, so
each was looked at rather than silenced.
fslio.c, FslGetFileType2()
mutablefslio = (FSLIO *)fslio; /* dodgy and will generate warnings */
mutablefslio->niftiptr->nifti_type = ...;
The comment is right that it looks dodgy, but the cast was never
needed. `const FSLIO * fslio` makes the *member* niftiptr const --
its type is `nifti_image * const` -- while what it points at stays
fully mutable. The assignment is legal as written, so the cast and
the local both go.
fslio.c FslWriteVolumes(), cifti axio_num_tokens(), text_to_i64(),
text_to_f64()
Pointers cast to char * and then only read: walked, indexed, or
handed to strtoll()/strtod(), which take const char *. Declared
const, casts removed.
cifti strip_whitespace()
Returned `(char *)str` on its early-exit paths, handing the caller a
writable pointer to the const string it passed in. The function is
static, so its return type is nobody else's business; it now returns
const char *.
afni_xml_tool.c and cifti_tool.c each define a file-scope `opts_t gopt` with external linkage, so the two share a symbol name and neither declares it in a header. Each is used only inside its own translation unit, always by address. Both files build executables rather than libraries, so `static` removes nothing from any shared library's exported symbols. Clears both -Wmissing-variable-declarations warnings.
The build instructions in README.md were two lines about 'make all',
which builds a subset of the tree with a Makefile nothing else in the
project uses. They now describe the CMake build the CI and the install
rules actually use, and note that the Makefile is unmaintained and does
not cover nifti2 or cifti.
A new section records how to run the sanitizers and valgrind, including
three things that each cost an afternoon to work out:
* valgrind's memcheck needs --trace-children=yes here, because most of
the tests are shell scripts that exec the tools. Without it valgrind
inspects the shell, sees nothing and reports a clean run;
* ctest -T memcheck exits 0 even when valgrind reports defects, so the
logs have to be read;
* valgrind refuses to start without the C library's debug symbols, and
on distributions that ship a stripped ld.so with no debuginfo package
-- Arch and its derivatives -- it cannot be run at all, DEBUGINFOD_URLS
included, because those builds are not on any debuginfod server. A
container recipe is given.
Every command in the new sections was run against this tree.
…plicit 152 of the -Wsign-conversion findings are an int or int64_t count reaching a size_t parameter of malloc(), calloc(), realloc(), memcpy(), memset(), strncpy(), znzread(), znzwrite() or fread(). Where a count is multiplied by a sizeof, the cast goes on the count rather than around the product, so the multiply happens at 64-bit width: (int)count * sizeof(T) can overflow before it is widened. The counts cannot be negative. nifti_update_dims_from_array() clamps every dim[i] to at least 1, nifti_datatype_sizes() leaves nbyper at 0 for an unknown datatype, and nifti_image_load() requires nbyper > 0 and nvox > 0.
The findings that are not allocation, copy or I/O lengths: file offsets, extension bookkeeping, and the ANALYZE orientation byte. znztell() returns a signed znz_off_t stored in size_t locals, so the znzseek() arguments are cast to znz_off_t and the arithmetic happens at the width the function takes. fslio's nvox is widened per operand so the product of seven int dimensions is computed at 64 bits. XML_Parse() takes an int length while blen is unsigned; the conversion is now at the call. The ANALYZE orientation byte is read through unsigned char * rather than a signed lvalue. That is value-preserving either way -- signed to unsigned char is defined modulo 256 -- so it states the intent only.
One change is a fix. nifti_read_buffer() passed (int)(ntot / nim->swapsize) to nifti_swap_Nbytes(), which takes a size_t count that was widened in 2010 because it "might not fit as int". The cast reinstated that truncation, leaving most of an image above 2 GiB unswapped. The cast is gone. nifti_image_read() took the file size through (size_t), which turns nifti_get_filesize()'s -1 error return into SIZE_MAX and slips past the guard below it. A signed temporary is used instead, matching what nifti2_io.c already does at the same place. The rest casts an int dimension to size_t at the point of use.
The old functions carry no destination bound and are rejected outright under -fbounds-safety. Each call site was converted to snprintf, strlcpy or strlcat with an explicit destination size. strncpy zero-fills the whole destination and strlcpy does not; the call sites relied on that only where the buffer came from calloc, except one that now zeroes explicitly. A few declarations moved to first use.
strlcpy and strlcat originate in OpenBSD and are now in the other BSDs, macOS, and glibc from 2.38. Supply implementations for the platforms that still have neither.
b046eb1 to
129a637
Compare
3 participants
@hjmjohnson this one needs careful checking.
Also it's not done yet, I have to do some cmake stuff to compile the new file.