dev: Make the use of stdio FILE robust.

ABI changes:
    The three API functions png_init_io, png_begin_read_from_stdio and
    png_image_write_to_stdio now require the host fread, fwrite and
    fflush functions where appropriate in addition to the host FILE.

    Internally libpng uses the provided functions for all operations on
    the FILE; it does not use the implementations available when libpng
    was built.

API changes:
    The original APIs of the above three functions are now implemented
    as function-like macros which automatically pass the correct ISO-C
    functions.  This ensures that there are no net API changes and that
    the correct arguments are passed.

Build changes:
    The read stdio functionality is now dependent on SEQUENTIAL_READ
    rather than READ.  This is done for clarity; the progressive read
    mechanism does not use FILE.

Internal changes:
    png_struct::io_ptr has been supplemented with png_struct::stdio_ptr
    used when stdio is used directly by libpng for IO as opposed to
    caller-provided callbacks.

    Error checking has been added to detect mismatched use of the stdio
    (png_init_io etc) API with the callback (png_set_read_fn,
    png_set_write_fn APIs.)  Changing from one API to the other
    mid-stream should work but has not been tested and is not currently
    a documented option.

The changes address an issue which is frequently encountered on
Microsoft Windows systems because Windows NT DLLs each have their own
ISO-C hosted environment.  This means that a FILE from one DLL cannot be
used safely from a different DLL unless all access to the object is done
using functionality from the creating DLL.  The problem is more general;
passing objects across DLL or other boundaries is frequently supported
but direct access to those objects' internal structure in the receiving
environment is not safe.  Other such uses were address early on in the
development of libpng, this addresses the main, almost certainly, sole
remaining issue.

The idea of adding additional function pointers png_init_io was
suggested by github.com user pp383 in pull request pnggroup#208.

Signed-off-by: John Bowler <[email protected]>

png.c

@@ -669,26 +669,99 @@ png_get_io_ptr(png_const_structrp png_ptr)
    return png_ptr->io_ptr;
 }
 
-#if defined(PNG_READ_SUPPORTED) || defined(PNG_WRITE_SUPPORTED)
+#if defined(PNG_SEQUENTIAL_READ_SUPPORTED) || defined(PNG_WRITE_SUPPORTED)
 #  ifdef PNG_STDIO_SUPPORTED
 /* Initialize the default input/output functions for the PNG file.  If you
  * use your own read or write routines, you can call either png_set_read_fn()
  * or png_set_write_fn() instead of png_init_io().  If you have defined
  * PNG_NO_STDIO or otherwise disabled PNG_STDIO_SUPPORTED, you must use a
  * function of your own because "FILE *" isn't necessarily available.
  */
-void PNGAPI
-png_init_io(png_structrp png_ptr, png_FILE_p fp)
+void PNGAPI (
+png_init_io)(png_structrp png_ptr, FILE *fp,
+      size_t (*fread)(void *ptr, size_t size, size_t nmemb, FILE*),
+      size_t (*fwrite)(const void *ptr, size_t size, size_t nmemb, FILE*),
+      int (*fflush)(FILE*))
 {
    png_debug(1, "in png_init_io");
 
    if (png_ptr == NULL)
       return;
 
-   png_ptr->io_ptr = (png_voidp)fp;
-}
+   /* If SEQUENTIAL_READ is not supported and this IS a read png_struct
+    * png_init_io cannot be used (or, rather, it will not work).  Detect this
+    * early.
+    */
+#  ifndef PNG_SEQUENTIAL_READ_SUPPORTED
+      /* Read must be done using the progressive reader therefore: */
+      if ((png_ptr->mode & PNG_IS_READ_STRUCT) != 0)
+      {
+         png_app_error(png_ptr, "API: IO cannot be set with progressive read");
+         return;
+      }
+#  endif /* !SEQUENTIAL_READ */
+
+   /* First clear out any read/write functionality set by the caller. */
+   png_ptr->io_ptr = NULL;
+#  ifdef PNG_READ_SUPPORTED
+      png_ptr->read_data_fn = NULL;
+#  endif /* READ */
+#  ifdef PNG_WRITE_SUPPORTED
+      png_ptr->write_data_fn = NULL;
+#  endif /* WRITE */
+#  ifdef PNG_WRITE_FLUSH_SUPPPORTED
+      png_ptr->output_flush_fn = NULL;
 #  endif
 
+   /* Set up the stdio based read or write functions as appropriate. */
+#  ifdef PNG_SEQUENTIAL_READ_SUPPORTED
+      if ((png_ptr->mode & PNG_IS_READ_STRUCT) != 0)
+      {
+         png_ptr->fread = fread;
+         png_ptr->read_data_fn = png_stdio_read;
+      }
+      else
+      {
+         png_ptr->fread = NULL;
+      }
+#  else /* !SEQUENTIAL_READ */
+      PNG_UNUSED(fread)
+#  endif /* !SEQUENTIAL_READ */
+
+#  ifdef PNG_WRITE_SUPPORTED
+      if ((png_ptr->mode & PNG_IS_READ_STRUCT) == 0)
+      {
+         png_ptr->fwrite = fwrite;
+         png_ptr->write_data_fn = png_stdio_write;
+      }
+      else
+      {
+         png_ptr->fwrite = NULL;
+      }
+#  else /* !WRITE */
+      PNG_UNUSED(fwrite)
+#  endif /* !WRITE */
+
+#  ifdef PNG_WRITE_FLUSH_SUPPORTED
+      if ((png_ptr->mode & PNG_IS_READ_STRUCT) == 0)
+      {
+         png_ptr->fflush = fflush;
+         png_ptr->output_flush_fn = png_stdio_flush;
+      }
+      else
+      {
+         png_ptr->fflush = NULL;
+      }
+#  else /* !WRITE_FLUSH */
+      PNG_UNUSED(fflush)
+#  endif /* !WRITE_FLUSH */
+
+   png_ptr->stdio_ptr = fp;
+}
+#  endif /* STDIO */
+#endif /* SEQUENTIAL_READ || WRITE */
+
+#if defined(PNG_READ_SUPPORTED) || defined(PNG_WRITE_SUPPORTED)
 #  ifdef PNG_SAVE_INT_32_SUPPORTED
 /* PNG signed integers are saved in 32-bit 2's complement format.  ANSI C-90
  * defines a cast of a signed integer to an unsigned integer either to preserve
@@ -4556,15 +4629,14 @@ png_image_free_function(png_voidp argument)
 
    /* First free any data held in the control structure. */
 #  ifdef PNG_STDIO_SUPPORTED
-      if (cp->owned_file != 0)
+      if (cp->io_file != NULL)
       {
-         FILE *fp = png_voidcast(FILE*, cp->png_ptr->io_ptr);
-         cp->owned_file = 0;
+         FILE *fp = cp->io_file;
 
          /* Ignore errors here. */
          if (fp != NULL)
          {
-            cp->png_ptr->io_ptr = NULL;
+            cp->io_file = NULL;
             (void)fclose(fp);
          }
       }

png.h

@@ -1541,9 +1541,17 @@ PNG_REMOVED(209, void, png_set_filter_heuristics_fixed,
  */
 
 #ifdef PNG_STDIO_SUPPORTED
-/* Initialize the input/output for the PNG file to the default functions. */
-PNG_EXPORT(74, void, png_init_io, (png_structrp png_ptr, png_FILE_p fp));
-#endif
+#  if defined(PNG_SEQUENTIAL_READ_SUPPORTED) || defined(PNG_WRITE_SUPPORTED)
+/* Initialize the input/output for the PNG file to use stdio. */
+PNG_EXPORT(74, void, png_init_io, (png_structrp png_ptr, FILE *fp,
+      size_t (*fread)(void *ptr, size_t size, size_t nmemb, FILE*),
+      size_t (*fwrite)(const void *ptr, size_t size, size_t nmemb, FILE*),
+      int (*fflush)(FILE*)));
+
+#define png_init_io(png_ptr, fp)\
+   ((png_init_io)(png_ptr, fp, fread, fwrite, fflush))
+#endif /* SEQUENTIAL_READ || WRITE */
+#endif /* STDIO */
 
 /* Replace the (error and abort), and warning functions with user
  * supplied functions.  If no messages are to be printed you must still
@@ -2977,7 +2985,10 @@ PNG_EXPORT(234, int, png_image_begin_read_from_file, (png_imagep image,
     */
 
 PNG_EXPORT(235, int, png_image_begin_read_from_stdio, (png_imagep image,
-   FILE* file));
+   FILE* file, size_t (*fread)(void *ptr, size_t size, size_t nmemb, FILE*)));
+#define png_image_begin_read_from_stdio(image, file)\
+   ((png_image_begin_read_from_stdio)(image, file, fread))
+
    /* The PNG header is read from the stdio FILE object. */
 #endif /* STDIO */
 
@@ -3051,8 +3062,14 @@ PNG_EXPORT(239, int, png_image_write_to_file, (png_imagep image,
 
 PNG_EXPORT(240, int, png_image_write_to_stdio, (png_imagep image, FILE *file,
    int convert_to_8_bit, const void *buffer, png_int_32 row_stride,
-   const void *colormap));
+   const void *colormap,
+   size_t (*fwrite)(const void *ptr, size_t size, size_t nmemb, FILE*),
+   int (*fflush)(FILE*)));
    /* Write the image to the given (FILE*). */
+
+#define png_image_write_to_stdio(png_ptr, fp, cmap, cvt_to_8, buffer, stride)\
+   ((png_image_write_to_stdio)(png_ptr, fp, cmap, cvt_to_8, buffer, stride,\
+      fwrite, fflush))
 #endif /* SIMPLIFIED_WRITE_STDIO */
 
 /* With all write APIs if image is in one of the linear formats with 16-bit

pngpriv.h

@@ -849,23 +849,27 @@ PNG_INTERNAL_FUNCTION(void,png_zfree,(voidpf png_ptr, voidpf ptr),PNG_EMPTY);
  * PNGCBAPI at 1.5.0
  */
 
-PNG_INTERNAL_FUNCTION(void PNGCBAPI,png_default_read_data,(png_structp png_ptr,
-    png_bytep data, size_t length),PNG_EMPTY);
-
 #ifdef PNG_PROGRESSIVE_READ_SUPPORTED
 PNG_INTERNAL_FUNCTION(void PNGCBAPI,png_push_fill_buffer,(png_structp png_ptr,
     png_bytep buffer, size_t length),PNG_EMPTY);
-#endif
+#endif /* PROGRESSIVE_READ */
 
-PNG_INTERNAL_FUNCTION(void PNGCBAPI,png_default_write_data,(png_structp png_ptr,
+#ifdef PNG_STDIO_SUPPORTED
+#  ifdef PNG_SEQUENTIAL_READ_SUPPORTED
+PNG_INTERNAL_FUNCTION(void PNGCBAPI,png_stdio_read,(png_structp png_ptr,
+    png_bytep data, size_t length),PNG_EMPTY);
+#  endif /* SEQUENTIAL_READ */
+
+#ifdef PNG_WRITE_SUPPORTED
+PNG_INTERNAL_FUNCTION(void PNGCBAPI,png_stdio_write,(png_structp png_ptr,
     png_bytep data, size_t length),PNG_EMPTY);
+#endif /* WRITE */
 
 #ifdef PNG_WRITE_FLUSH_SUPPORTED
-#  ifdef PNG_STDIO_SUPPORTED
-PNG_INTERNAL_FUNCTION(void PNGCBAPI,png_default_flush,(png_structp png_ptr),
+PNG_INTERNAL_FUNCTION(void PNGCBAPI,png_stdio_flush,(png_structp png_ptr),
    PNG_EMPTY);
-#  endif
-#endif
+#endif /* WRITE_FLUSH */
+#endif /* STDIO */
 
 /* Reset the CRC variable */
 PNG_INTERNAL_FUNCTION(void,png_reset_crc,(png_structrp png_ptr),PNG_EMPTY);
@@ -1815,8 +1819,11 @@ typedef struct png_control
    png_const_bytep memory;          /* Memory buffer. */
    size_t          size;            /* Size of the memory buffer. */
 
+#  ifdef PNG_STDIO_SUPPORTED
+      FILE     *io_file;            /* FILE* opened by us, not user/app */
+#  endif
+
    unsigned int for_write       :1; /* Otherwise it is a read structure */
-   unsigned int owned_file      :1; /* We own the file in io_ptr */
 } png_control;
 
 /* Return the pointer to the jmp_buf from a png_control: necessary because C

pngread.c

@@ -75,12 +75,6 @@ png_create_read_struct_2,(png_const_charp user_png_ver, png_voidp error_ptr,
          if (png_ptr->target_state != 0U)
             png_set_option(png_ptr, PNG_TARGET_SPECIFIC_CODE, 1);
 #     endif
-
-      /* TODO: delay this, it can be done in png_init_io (if the app doesn't
-       * do it itself) avoiding setting the default function if it is not
-       * required.
-       */
-      png_set_read_fn(png_ptr, NULL, NULL);
    }
 
    return png_ptr;
@@ -1486,21 +1480,38 @@ png_image_read_header(png_voidp argument)
 }
 
 #ifdef PNG_STDIO_SUPPORTED
-int PNGAPI
-png_image_begin_read_from_stdio(png_imagep image, FILE* file)
+typedef struct
+{
+   png_structrp png_ptr;
+   FILE        *file;
+   size_t     (*fread)(void *ptr, size_t size, size_t nmemb, FILE*);
+} stdio_read_setup;
+
+static int
+setup_stdio_for_read(png_voidp parm)
+{
+   stdio_read_setup *read = png_voidcast(stdio_read_setup*, parm);
+   (png_init_io)(read->png_ptr, read->file, read->fread, NULL, NULL);
+   return 1;
+}
+
+int PNGAPI (
+png_image_begin_read_from_stdio)(png_imagep image, FILE* file,
+   size_t (*fread)(void *ptr, size_t size, size_t nmemb, FILE*))
 {
    if (image != NULL && image->version == PNG_IMAGE_VERSION)
    {
       if (file != NULL)
       {
-         if (png_image_read_init(image) != 0)
+         if (png_image_read_init(image))
          {
-            /* This is slightly evil, but png_init_io doesn't do anything other
-             * than this and we haven't changed the standard IO functions so
-             * this saves a 'safe' function.
-             */
-            image->opaque->png_ptr->io_ptr = file;
-            return png_safe_execute(image, png_image_read_header, image);
+            stdio_read_setup parm;
+            parm.png_ptr = image->opaque->png_ptr;
+            parm.file = file;
+            parm.fread = fread;
+
+            return png_safe_execute(image, setup_stdio_for_read, &parm) &&
+               png_safe_execute(image, png_image_read_header, image);
          }
       }
 
@@ -1523,20 +1534,13 @@ png_image_begin_read_from_file(png_imagep image, const char *file_name)
    {
       if (file_name != NULL)
       {
-         FILE *fp = fopen(file_name, "rb");
+         /* The file is stored in png_control::io_file and this means that it
+          * will passed to fclose on error:
+          */
+         FILE* fp = image->opaque->io_file = fopen(file_name, "rb");
 
          if (fp != NULL)
-         {
-            if (png_image_read_init(image) != 0)
-            {
-               image->opaque->png_ptr->io_ptr = fp;
-               image->opaque->owned_file = 1;
-               return png_safe_execute(image, png_image_read_header, image);
-            }
-
-            /* Clean up: just the opened file. */
-            (void)fclose(fp);
-         }
+            return png_image_begin_read_from_stdio(image, fp);
 
          else
             return png_image_error(image, strerror(errno));