363986 - nsJPEGEncoder::ReadSegments() and nsPNGEncoder::ReadSegments() do not advance read pointer

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Matthew Gertner]

The above-mentioned method always reads from the beginning of the internal buffer. As a result, multiple calls to ReadSegments() (or Read()) will always return the same data.

Comment 1

[Matthew Gertner]

Stuart - I noticed that the PNG encoder does the same thing. Is this a real bug or am I misunderstanding something?

Comment 2

[David Marteau]

Attachment: Patch for png and jpeg encoders ReadSegments

Comment 3

[David Marteau]

Sorry,I forgot comments !

I confirm the bug. It brokes the use of buffered output stream  with buffer size smaller than the size of the available data from the encoder internal buffer.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 298902 [details] [diff] [review]
Patch for png and jpeg encoders ReadSegments

Stuart, could you give this a quick once-over?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 298902 [details] [diff] [review]
Patch for png and jpeg encoders ReadSegments

This fixed the png and jpeg encoders to work even if the data is not read all in one huge chunk.  It's a very safe patch.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 298902 [details] [diff] [review]
Patch for png and jpeg encoders ReadSegments

a=beltzner for 1.9

Comment 7

[Reed Loden [:reed]]

Checking in modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp;
/cvsroot/mozilla/modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp,v  <--  nsJPEGEncoder.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in modules/libpr0n/encoders/png/nsPNGEncoder.cpp;
/cvsroot/mozilla/modules/libpr0n/encoders/png/nsPNGEncoder.cpp,v  <--  nsPNGEncoder.cpp
new revision: 1.9; previous revision: 1.8
done

Comment 8

[Boris Zbarsky [:bzbarsky]]

It'd be good to get some tests in for this...

Comment 9

[Fredrik Larsson (:nossralf)]

I believe the fix for this bug is wrong and causes memory corruption, potentially(?) of an exploitable kind. It causes hard crashes on Linux under certain conditions.

Looking at how NS_CopySegmentToBuffer [1] uses the offset parameter, it adds offset to the *target* buffer into which data is written with memcpy, it doesn't change the offset in the *source* buffer from which data is read (which is what a patch needs to accomplish).

I ran into this on Linux for Prism, which uses the PNG encoder with buffered output streams (using the writeFrom method). As long as the buffered output stream's buffer is larger than the image data size, everything is fine, since the while loop in [2] only is run through once.

But if the output stream's buffer is smaller, the current way of using mImageBufferReadPoint in [3] (on line #487) means the memcpy in NS_CopySegmentToBuffer will overrun the target buffer and write to memory belonging to something else. (It will still also only write the first aCount bytes of the mImageBuffer repeatedly.)

That then also causes crashes when the buffered output stream's destructor is called and the buffer is de-allocated (glibc throws a hissy-fit).

A potential fix is to use pointer arithmetic to increase the source address which is handed to NS_CopySegmentToBuffer (and thereby memcpy) by adding mImageBufferReadPoint to the address of mImageBuffer in [3] on line #486.

At the very least, the offset to NS_CopySegmentToBuffer needs to always be 0, since the callers of Read or ReadSegments expect just that.

[1] http://mxr.mozilla.org/mozilla/source/xpcom/io/nsStreamUtils.cpp#663
[2] http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsBufferedStreams.cpp#616
[3] http://mxr.mozilla.org/mozilla/source/modules/libpr0n/encoders/png/nsPNGEncoder.cpp#473

Comment 10

[Fredrik Larsson (:nossralf)]

Attachment: valgrind output showing invalid memory writes

This is Valgrind output that illustrates how there are invalid writes when using a buffered output stream (buffer size 4096 bytes, image buffer size 21941 bytes) in conjunction with the PNG encoder, for the reasons described above.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  The offset should be the amount consumed by the writer during this ReadSegments call and we only call the writer once.

Fredrik, do you want to post a patch?  Matthew, how come your testing didn't catch this?

Ideally someone would also write some unit tests here to make sure the PNG and JPEG encoders behave correctly....

Comment 12

[Fredrik Larsson (:nossralf)]

Attachment: use pointer arithmetic to advance pointer to source buffer

This is what came to mind, though I'm not sure if it's breaking any rules for good form or similar.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 311808 [details] [diff] [review]
use pointer arithmetic to advance pointer to source buffer

That looks right to me.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)

> Fredrik, do you want to post a patch?  Matthew, how come your testing didn't
> catch this?

I think the tests failed to catch this because the buffer was larger than the image and only required 1 loop (1 ReadSegments call)

Comment 15

[Boris Zbarsky [:bzbarsky]]

That doesn't match comment 0, which talks about errors if multiple calls are made, nor does it match the discussion in the newsgroup that preceded this bug...

Comment 16

[Fredrik Larsson (:nossralf)]

(In reply to comment #15)
> That doesn't match comment 0, which talks about errors if multiple calls are
> made, nor does it match the discussion in the newsgroup that preceded this
> bug...
> 

The unit tests for the PNG encoder (and imgITools btw) use a binary input stream and readByteArray to compare the encoded data. ReadByteArray always allocates a buffer with the length matching the number of bytes that are to be read (in turn determined by a call to the encoder input stream's available method). That means that the whole image buffer is always read in one go, regardless of the size, which in turn means the bug never surfaces.

Subsequent calls will then succeed since the encoder's ReadSegments method always sets *_retval to 0 for bytes read and returns NS_OK once the whole buffer has been read. (The unit tests never do subsequent calls, but still.)

I've so far only seen it when one writes to a buffered output stream using the encoder's input stream as the source, and where the buffer is smaller than the available data. No unit test with that particular stream configuration exists.

On a side note, I built my local XULRunner with the last patch applied and it fixes all the problems I've yet to see related to this.

Comment 17

[Damon Sicore (:damons)]

Fixing the flags here.  Back to blocking.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, I understand why our unit tests didn't catch this, and we should add more tests.  I want to know why the tests the bug reporter presumably had which caught the original issue here didn't catch the problem...

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Well, I know Matthew was working on Windows and Mac, platforms that did not crash, for whatever reason. It became a visible issue, crashing, for Prism on Linux only.

Comment 20

[Boris Zbarsky [:bzbarsky]]

But the code as checked in would have produced the wrong data on all the platforms...

Comment 21

[Fredrik Larsson (:nossralf)]

(In reply to comment #20)
> But the code as checked in would have produced the wrong data on all the
> platforms...
> 

Indeed. I was going to write up a reduced test case and try it on WinXP, but never got around to it since Valgrind lead me straight to where the problem was.

In the case of Prism, the PNG encoder is only used on Linux (Win uses ICO and Mac ICNS). So that Prism only crashed on Linux makes perfect sense. I don't know if Matt ran in to this during work on Prism or not, though.

Comment 22

[Matthew Gertner]

Boris - I noticed the problem but I didn't submit, test or review the original patch.

Fredrik - great work tracking this down!

Comment 23

[Boris Zbarsky [:bzbarsky]]

Ah, so I should be asking David that question.  The question still stands...

I would like someone to create unit tests to test this stuff now to make sure it actually works.

Comment 24

[Fredrik Larsson (:nossralf)]

Attachment: possible unit test, first attempt

This unit test should cover this particular bug. (It fails with the first patch, passes with the current one.)

I stuck it in test_imgtools.js only because there are handy functions for comparing byte arrays and such there. Should perhaps belong in a separate file. (Also not sure if testing stuff in a loop is bad mojo, feedback is very welcome.)

It's the regular encoder data sanity check with the added step of writing to a storage stream through a buffered output stream with a tiny buffer, and comparing the resulting binary data with the known reference.

Comment 25

[David Marteau]

Ooops, Sorry for the wrong patch.
I have found that the code I used for my test was at the same time modified to be used on 1.8.1 branch and used a large buffer to prevent the crash. In that configuration the bug was not hit and the patch was not tested, sorry for that.

(In reply to comment #20)
> But the code as checked in would have produced the wrong data on all the
> platforms...
> 
In fact, all tests and usages of encoders I have found always a use buffer of a minimum size of the available data in the encoder internal buffer. That may be explain why this bug was never hit except in a few cases easily solved by using a larger buffer.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 312528 [details] [diff] [review]
possible unit test, first attempt

Fredrik, thanks for the test!  Looks great to me; should get a once-over from Stuart.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fredrik, can you also attach the images the test needs?

Comment 28

[Fredrik Larsson (:nossralf)]

(In reply to comment #27)
> Fredrik, can you also attach the images the test needs?
> 

Boris, I purposefully wrote it so it doesn't require any additional test images. It's essentially tests 1+3 and 7+9 as defined earlier in test_imgtools.js, with the added steps of using a storage stream and writing through a buffered output stream. It seemed unnecessary to introduce new images when there were suitable ones available already. Plus, this way any encoder/decoder error will be caught before this new test is even reached, which could make diagnosing easier.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  lxr is just lying; I forgot that it does that for binary files.

Checked in, with the test.

Comment 30

[Reed Loden [:reed]]

This is fixed now, right? If so, can this be resolved?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Er, I meant to do that, yes.