Latent memory leak in pngtest

[vesalvojdani]

A memory leak in pngtest (#265) was fixed (8439534) by freeing row buffer when libpng exits via longjmp. There is still the risk that row_buf is clobbered when jumping back. This is unlikely in the current implementation since reading and writing expose the address of the pointer to the library functions. But the manual also suggests that single lines can be read as follows:

diff --git a/pngtest.c b/pngtest.c
index f426d52ec..7607e778d 100644
--- a/pngtest.c
+++ b/pngtest.c
@@ -1496,7 +1496,7 @@ test_one_file(const char *inname, const char *outname)
              (unsigned long)png_get_rowbytes(read_ptr, read_info_ptr));
 
 #endif /* !SINGLE_ROWBUF_ALLOC */
-         png_read_rows(read_ptr, (png_bytepp)&row_buf, NULL, 1);
+         png_read_row(read_ptr, row_buf, NULL);
 
 #ifdef PNG_WRITE_SUPPORTED
 #ifdef PNGTEST_TIMING
@@ -1504,7 +1504,7 @@ test_one_file(const char *inname, const char *outname)
          t_decode += (t_stop - t_start);
          t_start = t_stop;
 #endif
-         png_write_rows(write_ptr, (png_bytepp)&row_buf, 1);
+         png_write_row(write_ptr, row_buf);
 #ifdef PNGTEST_TIMING
          t_stop = (float)clock();
          t_encode += (t_stop - t_start);

The above change seems innocent, but the memory leak reported in #265 can now be reproduced. Using the same file, I can see leaks with ASAN (export CC=/usr/bin/clang CFLAGS="-g -O2 -fsanitize=address"; note the -O2 here). I also see clobbering by adding debug statements before free and compiling with the default configure/make on an Ubuntu machine.

Since the pngtest.c file is referenced in the manual and serves as an example, consider making row_buf volatile or static, so that one does not run into this kind of a memory leak when making reasonable alterations to the example program.

[jbowler]

No a bug in libpng. This is a test program that only gets run while building libpng so memory leaks are irrelevant as, indeed, are any problems since pngtest is only run with very specific images (and immediately exits on error, causing the build to fail!)

[vesalvojdani]

This is not a concern for running the test, but pngtest.c is referenced multiple times in the manual as a demonstration of how to use the library. This usage pattern has caused leaks in clients, like imagemagick, so it would be helpful to either add a comment warning about this, or make it more robust for people who use it as a template.

[jbowler]

@vesalvojdani I take your point about it being a bad example but your suggested change does not seem to fix anything. This is covered by section 4.6.2.1 lines 11-14 of ANSI X3.159-1989 (identical to ISO C90, but I don't have a copy of that, see the end of this comment). @ctruta: the problem seems to have been introduced by the referenced PR. This is because row_buf is modified after the setjmp call but not marked volatile, before the PR it (apparently) was not used after the longjmp.

fpin and fpout were already protected by being declared static but I think that is wrong too; they don't need to be protected because they aren't changed!

So it really is an example; a bad example! I suggest replacing it in the manual with contrib/examples/pngpixel.c which discusses the minutiae of the issue (see the comments starting at main() in that file) and isn't littered with spurious #defines.

To fix the bug introduced by the previous (2019) change simply put volatile on the row_buf declaration on line 872. Ideally remove the static from fpin and fpout too; or can @ctruta or @vesalvojdani explain why those should be necessary?

@vesalvojdani you are indeed correct that taking the address of something often fixes the problem, but not in this case; at the point setjmp is called row_buf is NULL (always). The addressing operation only occurs later and, indeed, your change removes even that. My intuition is that what your change does is to cause row_buf to always be NULL after longjmp but it is certainly "indeterminate".

Note that contrib/libtests/pngpixel.c was carefully written to get this right even though the approach is not my preferred one; pngpixel uses the familiar C programmer single-function-with-many-lines style which is a big problem with setjmp/longjmp. simpleover.c and pngtopng.c avoid setjmp by using the "simplified" API, this is my recommended approach for app writers.

icctopng.c does use setjmp but it seems to be wrong too; missing volatile on info_ptr and result. example.c (and pngtest.c) avoid the problem by creating the info struct before the setjmp call which is counter-intuitive but I it works because the info struct creation routines take special care not to call png_error. contrib/libtests/pngimage.c does it in my preferred and now recommended way: Put the setjmp in an isolated function.

Note that pngvalid.c uses @ctruta's Try Catch stuff contrib/visupng/cexcept.h and may have problems; the 'display_init' structures are on the stack and only protected because they are address-taken before the Try. A better approach is the one used in libpng itself used for the simplified API which always calls via png_safe_execute, however this may be too difficult a programming technique for general use because it requires correct wrappering of the actual code inside png_safe_execute.

Note that the ANSI-C standard is not widely available and supposed copies on the web may be bogus (I found one such copy). This seems to be correct:

https://www.bsb.me.uk/ansi-c/ansi-c#sec_4_6_2

To be sure I'll copy and paste the paragraph with added emphasis (i.e. I put the bold and italic bits in):

All accessible objects have values as of the time longjmp was called, except that the values of objects of automatic storage duration that do not have volatile type and have been changed between the setjmp invocation and longjmp call are indeterminate.

[jbowler]

Try this:

https://github.com/jbowler/libpng/tree/pr496

It passes make check, make distcheck, cmake and contib/examples all compile without warnings (-Wall -Wextra to gcc). I have gcc 13.2.1

[jbowler]

@ctruta: [libpng18] Easy solution, remove pngtest :-)