New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 752898 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

CHECK failure: data_position + length <= data_->size() in FastSharedBufferReader.cpp

Project Member Reported by ClusterFuzz, Aug 7 2017

Issue description

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

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5220363326980096


Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: msrchandra@chromium.org
Components: Blink>Image
Labels: Test-Predator-Wrong-CLs M-62
Owner: pilgrim@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL did not provide any possible suspects.
Using Code Search for the file, "FastSharedBufferReader.cpp" assigning to concern owner from GIT Blame.

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/d6f04a76bd3b49d538eadf08e97e64b800531ba0

@pilgrim -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.
Components: -Blink>Image Internals>Images>Codecs
Owner: ----
Status: Untriaged (was: Assigned)
Owner: scroggo@chromium.org
Status: Assigned (was: Untriaged)
This is unrelated to pilgrim's change. 
Status: Started (was: Assigned)
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?
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.
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/
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by ClusterFuzz, 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.
Project Member

Comment 9 by ClusterFuzz, Aug 19 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
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