[21.5] Fix crash in PNG initialiation [was: can't move cursor ...]

[Stephen J. Turnbull]

21.5

Vin, this presumably affects 21.4 too.  If you don't recall any
related reports (probably only comes up if PNG lib that gets found at
runtime differs from build-time link lib), I'll take my time....

Stephen J. Turnbull writes:

 > Does `configure --with-cppflags="-newflag $CPPFLAGS" ...' work in your
 > application?  I think that having the `--with-whateverflags' options
 > override the corresponding environment variables is easier to
 > explain.  It's certainly easier to code!

[[ This is in my queue but I am working on other stuff. ]]

 > Ron Isaacson writes:

 >  > #2 is definitely not our problem. :-) I think you need to ensure that
 >  > png_err_stct.setjmp_buffer is set to a valid address BEFORE passing
 >  > png_error_func as a parameter to anything. Otherwise, any runtime
 >  > error from png_create_read_struct will get you into the same boat.

OK, yes, this is really ugly.  (But then, when isn't a setjmp/longjmp
ugly?)  I think the attached patch should fix the problem, but I don't
yet have a test case and I've never patched code involving setjmp/
longjmp before so a second opinion would be welcome (and a third...).
(I think the relevant test case for the original bug can be easily
constructed by substituting "You're going down, sucker!" for
PNG_LIBPNG_VER_STRING, but I haven't done that yet.)

The first hunk is a little suspicious; for some reason
png_destroy_read_struct() wants access to the pointer variable
containing png_ptr, not just the pointer itself.  The only reason I
can think why it would want that is to NULL it, and I've done that, so
passing &tmp to png_destroy_read_struct() should be OK, I think.

The other changes are to first NULL out any unwind pointers to
structures that are being destroyed in case the libpng code longjmps,
then call libpng's finalizer; to move the setjmp call much earlier;
and to stuff the unwind structure with the the necessary data as soon
as we get it.

I guess I have to assume that none of the libpng functions will
implicitly call a finalizer....

I'll run with it a while before committing, but would really
appreciate comments.

2007-02-17  Stephen J. Turnbull  <stephen at xemacs.org>

	* glyphs-eimage.c (png_instantiate_unwind): Avoid recursion.
	(png_instantiate): Initialize setjmp_buffer early, and avoid
	recursive entry to error handler.

diff --git a/src/glyphs-eimage.c b/src/glyphs-eimage.c
index 49364ca..5184e2c 100644
--- a/src/glyphs-eimage.c
+++ b/src/glyphs-eimage.c
@@ -849,7 +849,13 @@ png_instantiate_unwind (Lisp_Object unwind_obj)
 
   free_opaque_ptr (unwind_obj);
   if (data->png_ptr)
-    png_destroy_read_struct (&(data->png_ptr), &(data->info_ptr), (png_infopp)NULL);
+    {
+      /* ensure we can't get here again */
+      png_structp tmp = data->png_ptr;
+      data->png_ptr = NULL;
+      png_destroy_read_struct (&tmp, &(data->info_ptr), (png_infopp)NULL);
+    }
+
   if (data->instream)
     retry_fclose (data->instream);
 
@@ -874,24 +880,36 @@ png_instantiate (Lisp_Object image_instance, Lisp_Object instantiator,
   png_structp png_ptr;
   png_infop info_ptr;
 
+  xzero (unwind);
+  record_unwind_protect (png_instantiate_unwind, make_opaque_ptr (&unwind));
+
+  if (setjmp (png_err_stct.setjmp_buffer))
+    {
+      /* Something blew up:
+	 just display the error (cleanup happens in the unwind) */
+      signal_image_error_2 ("Error decoding PNG",
+			     build_string(png_err_stct.err_str),
+			     instantiator);
+    }
+
   /* Initialize all PNG structures */
-  png_ptr = png_create_read_struct (PNG_LIBPNG_VER_STRING, (void*)&png_err_stct,
+  png_ptr = png_create_read_struct (PNG_LIBPNG_VER_STRING,
+				    (void *) &png_err_stct,
 				    png_error_func, png_warning_func);
   if (!png_ptr)
     signal_image_error ("Error obtaining memory for png_read", instantiator);
+  unwind.png_ptr = png_ptr;
+
   info_ptr = png_create_info_struct (png_ptr);
   if (!info_ptr)
     {
+      unwind.png_ptr = NULL;	/* avoid re-calling png_destroy_read_struct
+				   when unwinding */
       png_destroy_read_struct (&png_ptr, (png_infopp)NULL, (png_infopp)NULL);
       signal_image_error ("Error obtaining memory for png_read", instantiator);
     }
-
-  xzero (unwind);
-  unwind.png_ptr = png_ptr;
   unwind.info_ptr = info_ptr;
 
-  record_unwind_protect (png_instantiate_unwind, make_opaque_ptr (&unwind));
-
   /* This code is a mixture of stuff from Ben's GIF/JPEG stuff from
      this file, example.c from the libpng 0.81 distribution, and the
      pngtopnm sources. -WMP-
@@ -900,16 +918,6 @@ png_instantiate (Lisp_Object image_instance, Lisp_Object instantiator,
      and is no longer usable for previous versions. jh
   */
 
-  /* Set the jmp_buf return context for png_error ... if this returns !0, then
-     we ran into a problem somewhere, and need to clean up after ourselves. */
-  if (setjmp (png_err_stct.setjmp_buffer))
-    {
-      /* Something blew up: just display the error (cleanup happens in the unwind) */
-      signal_image_error_2 ("Error decoding PNG",
-			     build_string(png_err_stct.err_str),
-			     instantiator);
-    }
-
   /* Initialize the IO layer and read in header information */
   {
     Lisp_Object data = find_keyword_in_vector (instantiator, Q_data);