Review: JPEG robust to broken files

Intercept the internal JPEG library errors and report them.  Pay particular attention to things that might mean a corrupt JPEG file.

[jrobertray, 2011-01-13 18:44:54]

http://codereview.appspot.com/3978041/diff/1/src/jpeg.imageio/jpeginput.cpp
File src/jpeg.imageio/jpeginput.cpp (right):

http://codereview.appspot.com/3978041/diff/1/src/jpeg.imageio/jpeginput.cpp#n...
src/jpeg.imageio/jpeginput.cpp:91: char errbuf[JMSG_LENGTH_MAX];
errbuf unused here.

http://codereview.appspot.com/3978041/diff/1/src/jpeg.imageio/jpeginput.cpp#n...
src/jpeg.imageio/jpeginput.cpp:257: if (! close ()  ||
close() might be invoked before setjmp(), and close() may call into jpeg library
with default error handler installed.

http://codereview.appspot.com/3978041/diff/1/src/jpeg.imageio/jpeginput.cpp#n...
src/jpeg.imageio/jpeginput.cpp:298: jpeg_read_scanlines (&m_cinfo, (JSAMPLE
**)&data, 1);
Can close() be called externally while the default error handler is installed?

[larrygritz, 2011-01-14 01:51:15]

Take 2

[larrygritz, 2011-01-14 01:52:26]

Thanks for the review.  How's it look now?  It's a little complicated with
close(), but I think it's safe.

I definitely feel that this needs to live in the trunk for a while, I don't feel
good about porting it to 0.9 without a lot more testing.

[jrobertray, 2011-01-14 21:29:43]

Looks safer now. I've marked the places I found that would leave the custom
error handler in place. What is your motivation for wanting to re-install the
default error handler?

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cpp
File src/jpeg.imageio/jpeginput.cpp (right):

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:180: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:194: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:197: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:204: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:265: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:272: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:297: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:305: return false;
use_default_jpeg_error_handling will not be called.

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:305: return false;
Side note: does returning early from close() end up leaking a file descriptor
(m_fd), say if called via ~ImageInput?

http://codereview.appspot.com/3978041/diff/6001/src/jpeg.imageio/jpeginput.cp...
src/jpeg.imageio/jpeginput.cpp:307: }
use_default_jpeg_error_handling is not called before leaving block.

[larrygritz, 2011-01-15 01:09:11]

On 2011/01/14 21:29:43, jrobertray wrote:
> Looks safer now. I've marked the places I found that would leave the custom
> error handler in place. What is your motivation for wanting to re-install the
> default error handler?

Excellent question.  I don't know.  I think maybe I was tired when coding that
up, not thinking clearly.  Since each JPEG ImageInput has its own private jpeg
error manager, there aren't any thread issues.  I suppose we could install the
custom manager once and then be done with it.  Good point.  I'll go back in for
one more pass.


> Side note: does returning early from close() end up leaking a file descriptor
> (m_fd), say if called via ~ImageInput?

Um... I'll look at that.

[larrygritz, 2011-01-15 23:48:56]

Next try -- raytype()

[larrygritz, 2011-01-15 23:52:48]

On 2011/01/15 23:48:56, larrygritz wrote:
> Next try -- raytype()

Bad cut and paste!  Forget the "raytype" comment.  But I did just post a new
patch set.

I think I addressed jrobertray's comments -- now we use our custom handler for
all time, don't set it back to the default, which I agree was unnecessary.

While I was in there, I put an '#if 0' around some dubious code that would read
the remainder of the image upon close.  That's because we can't
"jpeg_finish_decompress" without reading all scanlines.  But who cares!  If
we're closing, we're done anyway, so just destroy it, don't "finish" it.

But there's a small chance that this will be bad, so again, I'll say that even
after committing this, I want it only in the trunk.  It just doesn't seem like a
good idea for such risky changes to go into the 0.9 release until it's widely,
thoroughly tested.

[larrygritz, 2011-01-20 06:18:59]

Any further comments about this, or is my current proposal satisfactory?

[jrobertray, 2011-01-20 20:02:56]

Regarding the matter of reading scanlines in close(), the jpeg library
offers jpeg_abort_decompress() as a way to free up internal memory
without decoding the rest of the file. Not calling it may leak
resources, but I haven't tested this.

Code looks good to me.

On Wed, Jan 19, 2011 at 10:18 PM,  <larrygritz@gmail.com> wrote:
> Any further comments about this, or is my current proposal satisfactory?
>
>
> http://codereview.appspot.com/3978041/
>