BUG: Bound the formatted write in znzprintf, and end the va_list on failure - #41
Conversation
|
Pushed a second commit. The first one bounds the write but still returned the Before, printing 2 MB through the 1000001-byte buffer: After: One correction so this is not ranked higher than it deserves: |
d86bbb0 to
8ca9baf
Compare
8ca9baf to
7b18b84
Compare
7b18b84 to
f6f6a4f
Compare
|
Rebased onto current Maintainer decision needed: this changes Rebase and commit-message cleanupThe ~29 shared-base commits are already on Message cleanup: removed the The first subject was shortened to Test added, and the red proof
Red, with only the Same revert under AddressSanitizer, showing the underlying overflow: Green with the fix, in both builds: Per the "a fix that makes something stop happening needs a companion assertion", the test also pins that an ordinary write still returns its byte count and that the file afterwards holds exactly those bytes, so a fix that simply refused every write would not pass. The test deliberately does not assert the NULL-stream return value, since that is the semantic change flagged above and should be your call, not the test's. |
57814cf to
962decd
Compare
znzprintf() built its output with vsprintf(), which has no bound; the buffer was sized strlen(format) + 1000000 under a comment reading "overkill I hope". A single %s argument longer than a megabyte writes past the end of the allocation. vsnprintf() with the size already being computed turns that overflow into a reported truncation. The same block also returned on calloc failure without calling va_end(), leaving the va_list unterminated. This is the only unbounded formatted write in the tree; there is no sprintf() and no gets() anywhere.
The preceding commit bounds the write but still returned the gzprintf count after a truncation, so a caller could not tell. It also returned 0 for an allocation failure, and the function returns 0 for a NULL stream. The printf family reports failure with a negative value; 0 is a successful empty write, so it cannot carry either meaning. All three now return -1, and a truncated result is not written at all: a partial record reported as a whole one is worse than nothing. znzprintf sits inside COMPILE_NIFTIUNUSED_CODE in both znzlib.c and znzlib.h and is absent from libznz.a in a default build, so this is a latent defect in code no released configuration compiles.
znzprintf has no caller in this tree and is compiled only under COMPILE_NIFTIUNUSED_CODE, so nothing reached it. The test builds its own copy of znzlib.c with that definition and drives a 2 MB argument through the 1000001 byte buffer. Without the bound the write overflows the allocation, and the call reports success; the test pins the negative return, and pins the ordinary write and the resulting file content so an over-broad fix that refuses everything does not pass.
znzflush() and znzeof() are defined inside COMPILE_NIFTIUNUSED_CODE and declared nowhere, unlike znzgets(), znzputc() and znzgetc() beside them. Nothing compiled that block until a test in this branch did, so the omission was invisible. They are declared where the rest of the block is declared, so the warning set the project already requires stays clean when the block is built.
962decd to
bb54abc
Compare
8449a1f
into
InsightSoftwareConsortium:master
2 participants
znzprintf() built its output with vsprintf(), which has no bound:
The comment is honest about what was protecting the buffer. A single %s
argument longer than a megabyte writes past the end of the allocation.
vsnprintf() with the size already being computed turns that from an
overflow into a truncation, and the truncation is now reported.
The same block also returned on calloc failure without calling va_end(),
leaving the va_list unterminated.
This is the only unbounded formatted write in the tree; there is no
sprintf() and no gets() anywhere, and 138 snprintf() calls, largely from
seanm's earlier work in upstream PRs #5 and #19.
Interface impact: none. On the union of all these changes, configured with
USE_FSL_CODE=ONandUSE_CIFTI_CODE=ON: all 448 exported symbols acrosslibniftiio,libnifti2,libznz,libfslio,libnifticdfandlibciftiare identical tomasterundernm -D --defined-only, and all ten installed headers are identical undergcc -E -P. Undergcc -dM -Eone macro definition differs, intentionally and only in text: #61 makesFSL_RADIOLOGICALread(-1)so it is safe inside an expression. Its value is still-1, checked by compiling against each installedfslio.hand printing it.Verification. This branch: builds with gcc 16.1.1,
ctestunchanged frommaster(2 of 345 fail onmasteritself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8,ctest345/345 under each, and the whole suite under valgrind memcheck with--trace-children=yesgives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9
-Wsign-conversion(5 infslio.cfor #22, 2 innifti2_io.cand 2 innifti_tester001.cfor #24) and 8-Wcalloc-transposed-argsinnifti_findhdrnameandnifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.One of a set of independent, single-purpose PRs. Each bases on
masterand can be merged on its own, in any order.The full set of PRs (35)
The union of all of them is on the fork as
all-changes, if you want to build and test the lot at once.CI and build
Configuration and documentation
Defects
Warning and check classes