CHECK failure: data_position + length <= data_->size() in FastSharedBufferReader.cpp |
|||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5220363326980096 Fuzzer: ochang_search_index_mutator Job Type: windows_asan_chrome_no_sandbox Platform Id: windows Crash Type: CHECK failure Crash Address: Crash State: data_position + length <= data_->size() in FastSharedBufferReader.cpp blink::FastSharedBufferReader::GetConsecutiveData blink::BMPImageReader::ProcessNonRLEData Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=441019:441022 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5220363326980096 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Aug 7 2017
,
Aug 7 2017
This is unrelated to pilgrim's change.
,
Aug 8 2017
FastSharedBufferReader::GetConsecutiveData expects that the caller has ensured that the values it is passing make sense. i.e. It should only be attempting to read data that is already in the SegmentReader. In this case, the values are bad (we try to read well beyond the end of the SegmentReader).
The problem appears to be a bad assumption in BMPImageReader::ReadCurrentPixel. Its comment states:
// Assumes decoded_offset has been set to the beginning of the current
// row.
But this assumption is untrue - decoded_offset (sic - should be decoded_offset_) is set to the beginning of the non-RLE encoded section, which in this case occurs in the middle of a row.
(FWIW, the CHECK stops us before we get into some bad behavior. In particular, in this loop in FastSharedBufferReader::GetConsecutiveData:
for (char* dest = buffer;;) {
size_t copy = std::min(length, segment_length_);
memcpy(dest, segment_, copy);
length -= copy;
if (!length)
return buffer;
// Continue reading the next segment.
dest += copy;
GetSomeDataInternal(data_position_ + copy);
}
segment_length_ is 0 and segment_ is nullptr. So we try to memcpy from nullptr, which is undefined. If that doesn't cause problems, we loop until length becomes 0, and length only changes by subtracting copy, which is 0. So if we're lucky and don't hit undefined behavior, we loop forever.)
https://chromium-review.googlesource.com/c/607290 fixes the bug, by changing the assumption and updating decoded_offset_.
What confuses me is that this has never come up before. Perhaps non-RLE runs that start in the middle of a row are very uncommon?
FWIW, Firefox reports that this file "contains errors". (I am not currently set up to debug Firefox, so I have not investigated what this error is.) My Linux image viewer has a slightly better message: "BMP image has bogus header data". Stepping through Blink doesn't give me any indication of what the error might be. Android *does* support decoding the image, however. Looking at its implementation, I see that interprets a "kJpeg_BmpCompressionMethod" with 24 bits per pixel as RLE, with other bits per pixel resulting in unsupported format. I don't see how it relates to JPEG - maybe that is the error that the other decoders see?
,
Aug 10 2017
Regarding your last two paragraphs: the reason this hasn't come up before is not because uncompressed sequences mid-row are uncommon, but because this bug only affects RLE24 BMPs. RLE4/RLE8 will always run through the paletted branch, which didn't have a bug. RLE24 BMPS, in turn, are very rare. They are supported only on OS/2 2.0+, using compression format 4. Windows, in turn, didn't define this compression format value until V4, at which point it used it for BI_JPEG -- a JPEG wrapped inside a BMP. (Don't ask.) So systems which naively read the compression format without checking if the header size indicates an OS/2 2.x BMP will assume this is JPEG-in-BMP and fail to decode it correctly. Real OS/2 RLE24 BMPs are probably just about nonexistent outside test suites, though I haven't checked for sure. Certainly most decoders (clearly, including Firefox' and your Linux image viewer's) won't support them.
,
Aug 14 2017
Thanks for the info!
> So systems which naively read the compression format without checking if the
> header size indicates an OS/2 2.x BMP will assume this is JPEG-in-BMP and fail
> to decode it correctly. Real OS/2 RLE24 BMPs are probably just about
> nonexistent outside test suites, though I haven't checked for sure. Certainly
> most decoders (clearly, including Firefox' and your Linux image viewer's) won't
> support them.
Do you remember where the test suite versions are? I found [1], but it doesn't have a test image in this format. (It *does* have an "attempt" to do JPEG in BMP, but it doesn't have 24 bits per pixel, so I don't think it matches this format.)
I'm guessing you do not, based on your comment in [2] ("There’s at least one other suite I’ve used which has some good OS/2 BMPs, but I can’t find it now"). I also looked at [3] and [4], which you reference, and neither of them contains such an image. Given that such a BMP is so rare (possibly only in test suites) and not supported by other viewers, should we consider dropping support for it?
(Just found another set of test images at [5], which also does not have an image that matches 24 bit RLE pretending to be a JPEG.)
[1] http://entropymine.com/jason/bmpsuite/
[2] https://blog.mozilla.org/nnethercote/2015/11/06/i-rewrote-firefoxs-bmp-decoder/
[3] https://dompdf.net/test/image_bmp.html
[4] https://sourceforge.net/projects/bmptestsuite/
[5] https://osdn.net/projects/sfnet_freeimage/downloads/Test%20Suite%20(graphics)/2.5.0/bmp.zip/
,
Aug 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a7a849bdd9e100b6e7cb713e2721750a24b0a128 commit a7a849bdd9e100b6e7cb713e2721750a24b0a128 Author: Leon Scroggins III <scroggo@google.com> Date: Fri Aug 18 21:39:38 2017 Fix decoding non-RLE runs in RLE BMPs Previously, BMPImageReader::ReadCurrentPixel assumed that decoded_offset_ pointed to the beginning of a row, and used coord_.X() to compute an offset from the row start. While this works for non-RLE runs that start a row, it is incorrect for those that start part-way into a row. In those cases, decoded_offset_ corresponds to the first pixel in the non-RLE run. Update decoded_offset_ for each non-RLE pixel so that ReadCurrentPixel can read from decoded_offset_. Bug: 752898 Change-Id: I18fcec180f59b6ebe0e599f97df3539d08e05126 Reviewed-on: https://chromium-review.googlesource.com/607290 Commit-Queue: Leon Scroggins <scroggo@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#495696} [add] https://crrev.com/a7a849bdd9e100b6e7cb713e2721750a24b0a128/third_party/WebKit/LayoutTests/images/resources/crbug752898.bmp [modify] https://crrev.com/a7a849bdd9e100b6e7cb713e2721750a24b0a128/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageDecoderTest.cpp [modify] https://crrev.com/a7a849bdd9e100b6e7cb713e2721750a24b0a128/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp [modify] https://crrev.com/a7a849bdd9e100b6e7cb713e2721750a24b0a128/third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h
,
Aug 19 2017
ClusterFuzz has detected this issue as fixed in range 495542:495750. Detailed report: https://clusterfuzz.com/testcase?key=5220363326980096 Fuzzer: ochang_search_index_mutator Job Type: windows_asan_chrome_no_sandbox Platform Id: windows Crash Type: CHECK failure Crash Address: Crash State: data_position + length <= data_->size() in FastSharedBufferReader.cpp blink::FastSharedBufferReader::GetConsecutiveData blink::BMPImageReader::ProcessNonRLEData Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=441019:441022 Fixed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=495542:495750 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5220363326980096 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Aug 19 2017
ClusterFuzz testcase 5220363326980096 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by msrchandra@chromium.org
, Aug 7 2017Components: Blink>Image
Labels: Test-Predator-Wrong-CLs M-62
Owner: pilgrim@chromium.org
Status: Assigned (was: Untriaged)