20.67 KB, text/plain
2.18 KB, patch
Stuart Parmenter: review+
|Details | Diff | Splinter Review|
2.43 KB, patch
Stuart Parmenter: review+
|Details | Diff | Splinter Review|
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.
Stuart - I noticed that the PNG encoder does the same thing. Is this a real bug or am I misunderstanding something?
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 on attachment 298902 [details] [diff] [review] Patch for png and jpeg encoders ReadSegments Stuart, could you give this a quick once-over?
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 on attachment 298902 [details] [diff] [review] Patch for png and jpeg encoders ReadSegments a=beltzner for 1.9
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
It'd be good to get some tests in for this...
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  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  only is run through once. But if the output stream's buffer is smaller, the current way of using mImageBufferReadPoint in  (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  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.  http://mxr.mozilla.org/mozilla/source/xpcom/io/nsStreamUtils.cpp#663  http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsBufferedStreams.cpp#616  http://mxr.mozilla.org/mozilla/source/modules/libpr0n/encoders/png/nsPNGEncoder.cpp#473
Created attachment 311801 [details] 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.
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....
Created attachment 311808 [details] [diff] [review] 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 on attachment 311808 [details] [diff] [review] use pointer arithmetic to advance pointer to source buffer That looks right to me.
(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)
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...
(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.
Fixing the flags here. Back to blocking.
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...
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.
But the code as checked in would have produced the wrong data on all the platforms...
(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.
Boris - I noticed the problem but I didn't submit, test or review the original patch. Fredrik - great work tracking this down!
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.
Created attachment 312528 [details] [diff] [review] 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.
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 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.
Fredrik, can you also attach the images the test needs?
(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.
Ah, I see. lxr is just lying; I forgot that it does that for binary files. Checked in, with the test.
This is fixed now, right? If so, can this be resolved?
Er, I meant to do that, yes.