From 9bf7a569f9433e52518d832b1621aa5e8d2680e3 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 14 Aug 2026 23:12:34 -0400 Subject: [PATCH 1/4] BUG: Bound the formatted write in znzprintf, and end the va_list 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. (cherry picked from commit 57a61a7fd296a6f99fb3bb0f9a5b1a0a92ad94b5) --- znzlib/znzlib.c | 8 ++++++-- znzlib/znzlib.h | 7 +++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/znzlib/znzlib.c b/znzlib/znzlib.c index 52b5665d..64c80da1 100644 --- a/znzlib/znzlib.c +++ b/znzlib/znzlib.c @@ -302,13 +302,17 @@ int znzprintf(znzFile stream, const char *format, ...) #ifdef HAVE_ZLIB if (stream->zfptr!=NULL) { size_t size; /* local to HAVE_ZLIB block */ - size = strlen(format) + 1000000; /* overkill I hope */ + int written; + size = strlen(format) + 1000000; /* still generous, but now a bound */ tmpstr = (char *)calloc(1, size); if( tmpstr == NULL ){ fprintf(stderr,"** ERROR: znzprintf failed to alloc %zu bytes\n", size); + va_end(va); return retval; } - vsprintf(tmpstr,format,va); + written = vsnprintf(tmpstr,size,format,va); + if( written < 0 || (size_t)written >= size ) + fprintf(stderr,"** ERROR: znzprintf output truncated at %zu bytes\n", size-1); retval=gzprintf(stream->zfptr,"%s",tmpstr); free(tmpstr); } else diff --git a/znzlib/znzlib.h b/znzlib/znzlib.h index ff031687..5b21a4f0 100644 --- a/znzlib/znzlib.h +++ b/znzlib/znzlib.h @@ -150,9 +150,16 @@ ZNZ_API int znzputc(int c, znzFile file); ZNZ_API int znzgetc(znzFile file); #if !defined(WIN32) +/* the attribute lets the caller's format be checked, which is what makes + the internal vsnprintf on it acceptable to -Wformat-nonliteral */ +#if defined(__GNUC__) || defined(__clang__) +ZNZ_API int znzprintf(znzFile stream, const char *format, ...) + __attribute__((format(printf, 2, 3))); +#else ZNZ_API int znzprintf(znzFile stream, const char *format, ...); #endif #endif +#endif /*=================*/ #ifdef __cplusplus From 714d2dad43dd578dda2d609758e35bb1f0de4744 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Fri, 18 Sep 2026 22:01:29 -0400 Subject: [PATCH 2/4] BUG: Report a znzprintf failure as a failure 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. (cherry picked from commit ac7ec091d1a87813822f1f40c379c292f17623eb) --- znzlib/znzlib.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/znzlib/znzlib.c b/znzlib/znzlib.c index 64c80da1..05d87e27 100644 --- a/znzlib/znzlib.c +++ b/znzlib/znzlib.c @@ -297,7 +297,9 @@ int znzprintf(znzFile stream, const char *format, ...) int retval=0; char *tmpstr; va_list va; - if (stream==NULL) { return 0; } + /* the printf family reports failure with a negative value; 0 means an + empty write succeeded, so it cannot be used for the failures below */ + if (stream==NULL) { return -1; } va_start(va, format); #ifdef HAVE_ZLIB if (stream->zfptr!=NULL) { @@ -308,11 +310,17 @@ int znzprintf(znzFile stream, const char *format, ...) if( tmpstr == NULL ){ fprintf(stderr,"** ERROR: znzprintf failed to alloc %zu bytes\n", size); va_end(va); - return retval; + return -1; } written = vsnprintf(tmpstr,size,format,va); - if( written < 0 || (size_t)written >= size ) + if( written < 0 || (size_t)written >= size ){ + /* writing the truncated text would put a partial record in the + file and report it as a complete one, so write nothing */ fprintf(stderr,"** ERROR: znzprintf output truncated at %zu bytes\n", size-1); + free(tmpstr); + va_end(va); + return -1; + } retval=gzprintf(stream->zfptr,"%s",tmpstr); free(tmpstr); } else From 6493aef7feeee44ee8e4f529356d48feef2f6ffc Mon Sep 17 00:00:00 2001 From: "Hans J. Johnson" Date: Tue, 22 Sep 2026 08:36:48 -0500 Subject: [PATCH 3/4] ENH: Cover the znzprintf truncation path 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. (cherry picked from commit 0350b30e729cda08615b3c9ee2f0b95a810ef107) --- znzlib/CMakeLists.txt | 12 ++++++++ znzlib/znzprintf_test.c | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 znzlib/znzprintf_test.c diff --git a/znzlib/CMakeLists.txt b/znzlib/CMakeLists.txt index 10239be6..5be28397 100644 --- a/znzlib/CMakeLists.txt +++ b/znzlib/CMakeLists.txt @@ -27,3 +27,15 @@ if(BUILD_SHARED_LIBS) target_compile_definitions(${NIFTI_ZNZLIB_NAME} INTERFACE ZNZ_USE_SHARED) endif() install_nifti_target(${NIFTI_ZNZLIB_NAME}) + +if(NIFTI_BUILD_TESTING AND ZLIB_FOUND AND NOT WIN32) + # znzprintf is only compiled under COMPILE_NIFTIUNUSED_CODE, and not at + # all on Windows, so the test builds its own copy of znzlib.c with that + # definition and is registered only where the function exists. + add_executable(znzprintf_test znzprintf_test.c znzlib.c) + target_compile_definitions(znzprintf_test PRIVATE COMPILE_NIFTIUNUSED_CODE) + target_include_directories(znzprintf_test PRIVATE ${CMAKE_CURRENT_LIST_DIR} ${ZLIB_INCLUDE_DIR}) + target_link_libraries(znzprintf_test PRIVATE ${NIFTI_ZLIB_LIBRARIES}) + add_test(NAME znzprintf_truncation + COMMAND $ ${CMAKE_CURRENT_BINARY_DIR}/znzprintf_test.gz) +endif() diff --git a/znzlib/znzprintf_test.c b/znzlib/znzprintf_test.c new file mode 100644 index 00000000..24b6c321 --- /dev/null +++ b/znzlib/znzprintf_test.c @@ -0,0 +1,67 @@ +/* Exercises znzprintf(), which is compiled only under + COMPILE_NIFTIUNUSED_CODE and has no caller inside this tree. */ + +#include +#include +#include + +#include "znzlib.h" + +#define BIG_LEN 2000000 + +static const char expected[] = "abc\n"; + +int main(int argc, char *argv[]) +{ + const char *path; + znzFile zf; + char *big; + int normal; + int truncated; + int status = 0; + size_t got; + char readback[64]; + + if( argc < 2 ){ fprintf(stderr,"usage: %s OUTFILE.gz\n", argv[0]); return 1; } + path = argv[1]; + + zf = znzopen(path, "wb", 1); + if( zf == NULL ){ fprintf(stderr,"** cannot open %s\n", path); return 1; } + + normal = znzprintf(zf, "%s", expected); + if( normal != (int)strlen(expected) ){ + fprintf(stderr,"** FAIL: ordinary write returned %d, expected %d\n", + normal, (int)strlen(expected)); + status = 1; + } + + big = (char *)malloc(BIG_LEN + 1); + if( big == NULL ){ fprintf(stderr,"** cannot allocate\n"); znzclose(zf); return 1; } + memset(big, 'x', BIG_LEN); + big[BIG_LEN] = '\0'; + + truncated = znzprintf(zf, "%s", big); + if( truncated >= 0 ){ + fprintf(stderr,"** FAIL: truncating write returned %d, expected a " + "negative value\n", truncated); + status = 1; + } + free(big); + + if( znzclose(zf) != 0 ){ fprintf(stderr,"** cannot close %s\n", path); return 1; } + + zf = znzopen(path, "rb", 1); + if( zf == NULL ){ fprintf(stderr,"** cannot reopen %s\n", path); return 1; } + memset(readback, 0, sizeof(readback)); + got = znzread(readback, 1, sizeof(readback) - 1, zf); + znzclose(zf); + + if( got != strlen(expected) || strcmp(readback, expected) != 0 ){ + fprintf(stderr,"** FAIL: file holds %zu bytes, expected only the %d " + "bytes of the ordinary write\n", got, (int)strlen(expected)); + status = 1; + } + + if( status == 0 ) printf("znzprintf test passed\n"); + return status; +} From 68a4bf13af77b1e0be2f43d8f041639bd7161b94 Mon Sep 17 00:00:00 2001 From: "Hans J. Johnson" Date: Tue, 22 Sep 2026 10:09:08 -0500 Subject: [PATCH 4/4] COMP: Declare the two znzlib functions that no header declares 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. (cherry picked from commit 8449a1f579045ff4a3792aa1193879dc4ccdd182) --- znzlib/znzlib.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/znzlib/znzlib.h b/znzlib/znzlib.h index 5b21a4f0..866fc01c 100644 --- a/znzlib/znzlib.h +++ b/znzlib/znzlib.h @@ -145,6 +145,10 @@ ZNZ_API int znzputs(const char *str, znzFile file); #ifdef COMPILE_NIFTIUNUSED_CODE ZNZ_API char * znzgets(char* str, int size, znzFile file); +ZNZ_API int znzflush(znzFile file); + +ZNZ_API int znzeof(znzFile file); + ZNZ_API int znzputc(int c, znzFile file); ZNZ_API int znzgetc(znzFile file);