BUG: Bound the formatted write in znzprintf, and end the va_list on failure

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 $<TARGET_FILE:znzprintf_test> ${CMAKE_CURRENT_BINARY_DIR}/znzprintf_test.gz)
+endif()

znzlib/znzlib.c

@@ -297,18 +297,30 @@ 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) {
     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);
-       return retval;
+       va_end(va);
+       return -1;
+    }
+    written = vsnprintf(tmpstr,size,format,va);
+    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;
     }
-    vsprintf(tmpstr,format,va);
     retval=gzprintf(stream->zfptr,"%s",tmpstr);
     free(tmpstr);
   } else

znzlib/znzlib.h

@@ -145,14 +145,25 @@ 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);
 
 #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

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 <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#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;
+}