juce_loadJPEGImageFromStream crashes

[grossebete]

Hi Jules,

juce_loadJPEGImageFromStream currently ignores all jpeg decoding errors silentlty.

This is very problematic in our code, which can be told to decode jpeg images on possibly very, very bad input streams (read: mp3 picture frames ).

When this happens, the jpglib routines continue execution on a very corrupted internal state (read: null pointers )

Furthermore, from what I read in the full jpglib source and usage examples, it seems like ERREXIT should not be silently ignored and c client code should use setjmp/longjmp handlers to resume execution in user code. For us C++ folks, this amounts to exception throwing and handling.

The following code is a rather simple fix that does just that.
It also contains a check that skips decoding of zero-sized images.

--- juce_JPEGLoaderBug/juce_JPEGLoader.cpp	Wed Oct 10 10:41:32 2007
+++ juce_JPEGLoaderBug/juce_JPEGLoader.patched.cpp	Mon Oct 29 18:09:45 2007
@@ -54,6 +54,20 @@

 //==============================================================================
+
+struct JPEGDecodingFailure {};
+
+void fatalErrorHandler (j_common_ptr cinfo)
+{
+    char errorBuffer [JMSG_LENGTH_MAX];
+    (*cinfo->err->format_message)(cinfo, errorBuffer);
+    jpeg_destroy(cinfo);
+
+    Logger::outputDebugString(String("JPEG decoding error: ") << errorBuffer);
+
+    throw JPEGDecodingFailure();
+}
+
 static void silentErrorCallback1 (j_common_ptr)
 {
 }
@@ -70,7 +84,7 @@
 {
     zerostruct (err);
 
-    err.error_exit = silentErrorCallback1;
+    err.error_exit = fatalErrorHandler;
     err.emit_message = silentErrorCallback2;
     err.output_message = silentErrorCallback1;
     err.format_message = silentErrorCallback3;
@@ -124,12 +138,22 @@
         jpegDecompStruct.src->next_input_byte   = (const unsigned char*) mb.getData();
         jpegDecompStruct.src->bytes_in_buffer   = mb.getSize();
 
+        try
+        {
         jpeg_read_header (&jpegDecompStruct, TRUE);
+        }
+        catch (JPEGDecodingFailure&)
+        {
+            return image;
+        }
 
         jpeg_calc_output_dimensions (&jpegDecompStruct);
 
         const int width = jpegDecompStruct.output_width;
         const int height = jpegDecompStruct.output_height;
+
+		if (width * height == 0)
+			return image;
 
         jpegDecompStruct.out_color_space = JCS_RGB;

Would it be a problem to commit something along those lines to the trunk?

Thanks.

[grossebete]

I hit ``post’’ a little too fast

The proper error handler setup should be:

    jpeg_std_error (&err);
    err.error_exit = fatalErrorHandler;
    err.emit_message = silentErrorCallback2;
    err.output_message = silentErrorCallback1;
    err.reset_error_mgr = silentErrorCallback1;

for the error message to be correctly formatted.

Also the catch clause should be pushed after the scope of the big if… since ERREXIT calls are scattered all over jpglib functions, not only the header decoding part.

Sorry for that.

[jules]

That’s great - thanks for getting that working!

One thing that seems a pity is to leak the jpegDecompStruct, by not calling jpeg_destroy_decompress if it fails… do you know if it’s safe to still call that when it has gone wrong?

[grossebete]

It should not be leaked, indeed, but I’m pretty sure it’s not, since the libjpg code contains this:

GLOBAL(void)
jpeg_destroy_decompress (j_decompress_ptr cinfo)
{
  jpeg_destroy((j_common_ptr) cinfo); /* use common routine */
}

and the fatalErrorHandler function calls jpeg_destroy.

Apparently, in libjpg, decoder and encoders are treated the same way, regarding dynamic allocations.

/* Routines that are to be used by both halves of the library are declared
 * to receive a pointer to this structure.  There are no actual instances of
 * jpeg_common_struct, only of jpeg_compress_struct and jpeg_decompress_struct.
 */
struct jpeg_common_struct {
  jpeg_common_fields;		/* Fields common to both master struct types */
  /* Additional fields follow in an actual jpeg_compress_struct or
   * jpeg_decompress_struct.  All three structs must agree on these
   * initial fields!  (This would be a lot cleaner in C++.)
   */
};

typedef struct jpeg_common_struct * j_common_ptr;
typedef struct jpeg_compress_struct * j_compress_ptr;
typedef struct jpeg_decompress_struct * j_decompress_ptr;

[grossebete]

Sorry, did not answer your other question.

The file ``example.c’’ from the official libjpg distribution contains this error handler:

METHODDEF(void)
my_error_exit (j_common_ptr cinfo)
{
  /* cinfo->err really points to a my_error_mgr struct, so coerce pointer */
  my_error_ptr myerr = (my_error_ptr) cinfo->err;

  /* Always display the message. */
  /* We could postpone this until after returning, if we chose. */
  (*cinfo->err->output_message) (cinfo);

  /* Return control to the setjmp point */
  longjmp(myerr->setjmp_buffer, 1);
}

and jumps to this point on return from longjmp:

  if (setjmp(jerr.setjmp_buffer)) {
    /* If we get here, the JPEG code has signaled an error.
     * We need to clean up the JPEG object, close the input file, and return.
     */
    jpeg_destroy_decompress(&cinfo);
    fclose(infile);
    return 0;
  }

So I guess it is safe to destroy the cinfo struct after decoding failed.

[jules]

oh yes, sorry - I didn’t notice that call there.

One other thing - I’m not too bothered about getting the error message, and was trying to avoid code bloat by not linking to jerror.c if possible. How badly do you need to get that error string?

[grossebete]

I won’t care that much about the jpglib error messages, once I’ll be done with this attached picture frame mess

Anyway, would it harm to have it, at least in debug builds ?

The format_message is only 40 lines long and does not depend on anything else but sprintf … maybe some copy/paste/reformat/preprocessor_conditionals could avoid injecting the whole jerror.c code into juce in debug builds, and bypass it entirely in release ones.

Thanks very much for asking, anyway

[jules]

It’s just that if you include code to get the message, your exe gets bloated with every possible error message, formatting functions, etc., which is a waste if it’s never needed.

[grossebete]

Yes, you’re right.

I did not see that JMESSAGE stuff and thought error messages were linked in anyway.

I guess I’ll be able to live without them after my next merge with the trunk

[jules]

Ok, I’ve checked in a version now that should do the trick. I’ve not got any images that are broken enough to test it though!

[grossebete]

Thanks!

I’d be glad to provide

Do you want one such image?

[grossebete]

Me again…

While torturing the jpeg decoder, it seems like I found another bug:

The decompStruct->src->bytes_in_buffer unsigned field is sometimes overflown by jpegSkip in juce_JPEGLoader.cpp, on some of my trashy images.

This also makes libjpg code crash.

This is how I fixed it:

static void jpegSkip (j_decompress_ptr decompStruct, long num) throw()
{
    decompStruct->src->next_input_byte += num;

    const long clampedNum = jmin(num, long(decompStruct->src->bytes_in_buffer));
    decompStruct->src->bytes_in_buffer -= clampedNum;
}

[jules]

Cool - thanks again!

[grossebete]

Hi Jules,

I noticed the error-skipping behavior in the png loader as well…

You disabled png_error and png_chunk_error.

This also makes the libpng decoding code crash on invalid input…

I solved this by modifying pngconf.h,

#define png_error(a, b) png_err(a)
#define png_chunk_error(a, b) png_err(a)

and used png_set_error_fn and the same logic as in the jpeg code for the error handling.

There’s just a little more cleanup to do in the catch block.

And it stopped crashing.

[jules]

ok, have a go of the code I’ve just checked in to see if it does the job…

[grossebete]

Yup. Fits the bill

Thanks !

[jules]