Do not consume input of corrupt progressive JPEG

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.
  *
  * Portions are Copyright (C) 2001-6 mozilla.org
  *
  * Other contributors:
  *   Stuart Parmenter <stuart@mozilla.com>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ skipping 71 matching lines @@
 
 // JPEG only supports a denominator of 8.
 const unsigned scaleDenominator = 8;
 
 } // namespace
 
 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
 };
 
 enum jstate {
     JPEG_HEADER,                 // Reading JFIF headers
     JPEG_START_DECOMPRESS,
     JPEG_DECOMPRESS_PROGRESSIVE, // Output progressive pixels
     JPEG_DECOMPRESS_SEQUENTIAL,  // Output sequential pixels
     JPEG_DONE,
     JPEG_ERROR
 };
 
 enum yuv_subsampling {
     YUV_UNKNOWN,
     YUV_410,
     YUV_411,
     YUV_420,
     YUV_422,
     YUV_440,
     YUV_444
 };
 
 void init_source(j_decompress_ptr jd);
 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 {
     // public fields; must be first in this struct!
     struct jpeg_source_mgr pub;
 
     JPEGImageReader* decoder;
 };
 
 static unsigned readUint16(JOCTET* data, bool isBigEndian)
@@ skipping 176 matching lines @@
         , m_bufferLength(0)
         , m_bytesToSkip(0)
         , m_state(JPEG_HEADER)
         , m_samples(0)
 #if USE(QCMSLIB)
         , m_transform(0)
 #endif
     {
         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);
 
         ASSERT(!m_info.src);
         decoder_source_mgr* src = (decoder_source_mgr*)fastZeroedMalloc(sizeof(decoder_source_mgr));
         if (!src) {
             m_state = JPEG_ERROR;
             return;
         }
@@ skipping 154 matching lines @@
             }
         // FALL THROUGH
 
         case JPEG_START_DECOMPRESS:
             // Set parameters for decompression.
             // FIXME -- Should reset dct_method and dither mode for final pass
             // of progressive JPEG.
             m_info.dct_method = dctMethod();
             m_info.dither_mode = ditherMode();
             m_info.do_fancy_upsampling = doFancyUpsampling();
+            m_info.do_block_smoothing = true;
             m_info.enable_2pass_quant = false;
-            m_info.do_block_smoothing = true;
+            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
             // uses the IJG memory manager, it must be allocated before the call
             // to jpeg_start_compress().
             // FIXME: note that some output color spaces do not need the samples
             // buffer. Remove this allocation for those color spaces.
             {
                 int samplesWidth = (m_info.out_color_space == JCS_YCbCr) ? computeYUVSize(&m_info, 0, ImageDecoder::SizeForMemoryAllocation).width() : m_info.output_width;
                 m_samples = (*m_info.mem->alloc_sarray)(reinterpret_cast<j_common_ptr>(&m_info), JPOOL_IMAGE, samplesWidth * 4, 1);
@@ skipping 16 matching lines @@
                 // If we've completed image output...
                 ASSERT(m_info.output_scanline == m_info.output_height);
                 m_state = JPEG_DONE;
             }
         // FALL THROUGH
 
         case JPEG_DECOMPRESS_PROGRESSIVE:
             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));
 
                 for (;;) {
                     if (!m_info.output_scanline) {
                         int scan = m_info.input_scan_number;
 
                         // If we haven't displayed anything yet
                         // (output_scan_number == 0) and we have enough data for
                         // a complete scan, force output of the last full scan.
@@ skipping 95 matching lines @@
 
 #if USE(QCMSLIB)
     qcms_transform* m_transform;
 #endif
 };
 
 // Override the standard error method in the IJG JPEG decoder code.
 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)
 {
 }
 
 void skip_input_data(j_decompress_ptr jd, long num_bytes)
 {
     decoder_source_mgr *src = (decoder_source_mgr *)jd->src;
     src->decoder->skipBytes(num_bytes);
 }
 
@@ skipping 302 matching lines @@
     // has failed.
     if (!m_reader->decode(*m_data, onlySize) && isAllDataReceived())
         setFailed();
 
     // If decoding is done or failed, we don't need the JPEGImageReader anymore.
     if (isComplete(this, onlySize) || failed())
         m_reader.clear();
 }
 
 }