Do not consume input of corrupt progressive JPEG

Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp

Index: Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
diff --git a/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp b/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
index bb1db2b50ff9f8d441cea9d4e0a49abf0059a11a..3316317d5bd920c7506db194dc0d3f0126b8f340 100644
--- a/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
+++ b/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
@@ -89,6 +89,7 @@ namespace blink {
 
 struct decoder_error_mgr {
     struct jpeg_error_mgr pub; // "public" fields for IJG library
+    int num_corrupt_warnings;  // Counts corrupt warning messages
     jmp_buf setjmp_buffer;     // For handling catastropic errors
 };
 
@@ -116,6 +117,7 @@ boolean fill_input_buffer(j_decompress_ptr jd);
 void skip_input_data(j_decompress_ptr jd, long num_bytes);
 void term_source(j_decompress_ptr jd);
 void error_exit(j_common_ptr cinfo);
+void emit_message(j_common_ptr cinfo, int msg_level);
 
 // Implementation of a JPEG src object that understands our state machine
 struct decoder_source_mgr {
@@ -312,9 +314,11 @@ public:
     {
         memset(&m_info, 0, sizeof(jpeg_decompress_struct));
 
-        // Set up the normal JPEG error routines, then override error_exit.
+        // Set up the normal JPEG error routines and install overrides.
         m_info.err = jpeg_std_error(&m_err.pub);
+        m_err.pub.emit_message = emit_message;

[scroggo_chromium, 2015/06/15 15:57:06]

Besides error reporting, it looks like the behavior change is only for a
progressive image. Should we use the default emit_message for sequential jpegs?
(Or do you want consistent error reporting for both?)

On that note, should we also look for corrupt data in a sequential jpeg? (Or
should we wait until we see problems resulting from a corrupt sequential jpeg to
try to handle it?)

[Noel Gordon, 2015/06/18 12:10:07]

Good points.  Since we have corruption for sequential JPEG handled already per
https://crbug.com/398235, we don't really need an emit_error for them.  We do
for the progressive (aka multi-scan / buffered image) JPEG case, so in the next
patch set, I restricted adding the emit_message handler in that case only, so
the patch is a change in behavior for progressive JPEG only.

With that change, the test-case http://lcamtuf.coredump.cx/ffjpeg still shows
one rendering (good: that's the goal here).

         m_err.pub.error_exit = error_exit;
+        m_err.num_corrupt_warnings = 0;
 
         // Allocate and initialize JPEG decompression object.
         jpeg_create_decompress(&m_info);
@@ -489,8 +493,12 @@ public:
             m_info.dct_method = dctMethod();
             m_info.dither_mode = ditherMode();
             m_info.do_fancy_upsampling = doFancyUpsampling();
-            m_info.enable_2pass_quant = false;
             m_info.do_block_smoothing = true;
+            m_info.enable_2pass_quant = false;
+            m_info.enable_external_quant = false;

[scroggo_chromium, 2015/06/15 15:57:06]

Are these (setting to false and 0) necessary or just here for completeness? (It
seems like the code above to memset m_info to 0 already accomplished this. IIUC,
we should only reach here once, unless jpeg_start_decompress returned false; can
these be changed by jpeg_start_decompress?)

[Noel Gordon, 2015/06/18 12:10:07]

For completeness. Two variables related to buffered image mode decompression
were set (see the diff left).  I had to find all the others and wrote them down
here for completeness.  It's a reminder to me of what they are, but also that
these variables interact with one another in somewhat tricky ways (see
libjpeg.txt).
Correct.  Aside: and if we did come here twice, anyone notice a memory leak?
In general, they need to set before jpeg_start_decompress since it reads them
and updates internal libjpeg state, and resets some them in some circumstances.

You may change some of them after jpeg_start_decompress, but not all of them.

About the only interesting one to change after is do_block_smoothing.  For
example, you could disable it after some number of progressive frames had been
decoded, the idea being that the image has sufficient detail at that time to not
need block smoothing at all (block smoothing is a relatively costly operation).

A good example of the effect of block smoothing on the initial image frames is
http://pooyak.com/p/progjpeg, the initial frames are blurry (block smoothed).

An earlier version of this change did that, but I could not decide on which
frame number to use to disable block smoothing, so I left it out.

Anyho, TL,DR; they are here for completeness.  We could also just ASSERT them. 
Would you prefer that?

[scroggo_chromium, 2015/06/18 13:19:24]

Either way. I just wanted to make sure I understand why they were there. I think
the comment in patch set 2 makes it clear they should already be set.

+            m_info.enable_1pass_quant = false;
+            m_info.quantize_colors = false;
+            m_info.colormap = 0;
 
             // Make a one-row-high sample array that will go away when done with
             // image. Always make it big enough to hold an RGB row. Since this
@@ -527,6 +535,9 @@ public:
             if (m_state == JPEG_DECOMPRESS_PROGRESSIVE) {
                 int status;
                 do {
+                    decoder_error_mgr* err = reinterpret_cast_ptr<decoder_error_mgr *>(m_info.err);
+                    if (err->num_corrupt_warnings)
+                        break;
                     status = jpeg_consume_input(&m_info);
                 } while ((status != JPEG_SUSPENDED) && (status != JPEG_REACHED_EOI));
 
@@ -642,10 +653,27 @@ private:
 void error_exit(j_common_ptr cinfo)
 {
     // Return control to the setjmp point.
-    decoder_error_mgr *err = reinterpret_cast_ptr<decoder_error_mgr *>(cinfo->err);
+    decoder_error_mgr* err = reinterpret_cast_ptr<decoder_error_mgr *>(cinfo->err);
     longjmp(err->setjmp_buffer, -1);
 }
 
+void emit_message(j_common_ptr cinfo, int msg_level)
+{
+    if (msg_level >= 0)
+        return;
+
+    decoder_error_mgr* err = reinterpret_cast_ptr<decoder_error_mgr *>(cinfo->err);
+    err->pub.num_warnings++;
+
+    // Detect and count corrupt JPEG warning messages.
+    const char* warning = 0;
+    int code = err->pub.msg_code;
+    if (code > 0 && code <= err->pub.last_jpeg_message)
+        warning = err->pub.jpeg_message_table[code];
+    if (warning && !strncmp("Corrupt JPEG", warning, 12))
+        err->num_corrupt_warnings++;

[scroggo_chromium, 2015/06/15 15:57:05]

This appears to be used as a boolean. Do we need to count each one? (Or is that
for debugging purposes?)

[Noel Gordon, 2015/06/18 12:10:07]

For debug but also compat with the way libjpeg internally counts warnings:
err->num_warnings++; /me not wanting to fall too far from the tree.

[scroggo_chromium, 2015/06/18 13:19:24]

sgtm

+}
+
 void init_source(j_decompress_ptr)
 {
 }