Dangers of setjmp()/longjmp()

Suppose that you want to use an external library (e.g., libpng or libjpeg) that, in terms of error handling, only gives you a choice between an approach based on setjmp()/longjmp(), or aborting the program. Of course, in robust programs, the first method should be used. But there are pitfalls inherently associated with using setjmp(). Let me illustrate one of them using ImageMagick as an example.

Once ImageMagick determines the dimensions of a PNG image that it reads, it allocates memory to store the pixels. The task is to free it if the image file is in fact broken. But the input image can be so broken that libpng calls longjmp() before telling the dimensions of the image. In this case, ImageMagick allocates nothing and thus has nothing to free. In short, the error path has to free memory if and only if it has been allocated before.

Sounds simple? Let's look at the code that seems to implement this logic:

static Image *ReadOnePNGImage(MngInfo *mng_info,
  const ImageInfo *image_info, ExceptionInfo *exception)
{
Image
  *image;

png_struct
  *ping;

unsigned char
  *png_pixels;

/* set up ping, do other things */

png_pixels=(unsigned char *) NULL;
if (setjmp(ping->jmpbuf))
  {
    /*
      PNG image is corrupt.
    */
    /* ... */
      if (png_pixels != (unsigned char *) NULL)
       png_pixels=(unsigned char *) RelinquishMagickMemory(png_pixels);
    /* ... */
    return(GetFirstImageInList(image));
  }

/* read png dimensions and other information - might call longjmp() */

if (num_passes > 1)
  png_pixels=(unsigned char *) AcquireQuantumMemory(image->rows,
    ping_info->rowbytes*sizeof(*png_pixels));
else
  png_pixels=(unsigned char *) AcquireQuantumMemory(ping_info->rowbytes,
    sizeof(*png_pixels));

/* read the scanlines - might call longjmp() */
/* do other useful things */
return (image);
}

At first, it seems that the code does exactly what is stated above: the bold statement frees the png_pixels pointer only if the memory has been allocated. But actually it doesn't work: the memory leaks if a broken PNG file is attempted to be processed. The library seems to forget after returning to setjmp() that a non-NULL value has been assigned to png_pixels. If one inserts printf() calls for debugging, they will print a non-NULL value after AcquireQuantumMemory(), but a NULL before the NULL check. So the memory is allocated and then not freed.

The explanation is that, after longjmp(), according to the ISO C standard (7.13.2.1, "The longjmp function"),

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

Indeed, the png_pixels variable is automatic, not volatile, and is changed between the calls to setjmp() and longjmp(), and thus the standard allows it to forget its value. GCC sometimes (but not always!) warns about such cases.

The simplest fix is usually to declare the variable as volatile. As for ImageMagick, they opted to fix the problem in a different way (but, if I read it correctly - with some dead code, and not for all variables), and version 6.5.4-6 is supposed to have no such memory leak.