New issue
Advanced search Search tips

Issue 904613 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Null-dereference READ in SkDiscardableMemory::Create

Project Member Reported by ClusterFuzz, Nov 13

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5515976856305664

Fuzzer: libFuzzer_skia_image_decode_fuzzer
Job Type: windows_libfuzzer_chrome_asan
Platform Id: windows

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  SkDiscardableMemory::Create
  SkBitmapCache::Alloc
  SkImage_Lazy::getROPixels
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=windows_libfuzzer_chrome_asan&range=607281:607292

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

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing_on_windows.md for more information.
 
Components: Internals>Skia
Cc: -metzman@google.com metzman@chromium.org kjlubick@chromium.org
Status: Available (was: Untriaged)
I find the test case size (4B) very suspicious (probably a bug in fuzzer and not tested code).
This doesn't repro on OSS-Fuzz/Linux: https://oss-fuzz.com/v2/testcase-detail/5178066481971200?noredirect=1
Owner: scro...@google.com
Status: Assigned (was: Available)
Leon, can you try reproducing this on a windows machine?

out/Release/fuzz -t image_decode -b [file]

should be enough to reproduce (if the auto-detect doesn't work)
Project Member

Comment 4 by ClusterFuzz, Nov 14

Labels: Fuzz-Blocker ReleaseBlock-Beta M-72
This crash occurs very frequently on windows platform and is likely preventing the fuzzer skia_image_decode_fuzzer from making much progress. Fixing this will allow more bugs to be found.

Marking this bug as a blocker for next Beta release.

If this is incorrect, please add ClusterFuzz-Wrong label and remove the ReleaseBlock-Beta label.
Unfortunately, I don't have a Windows machine to test on :(

But I've looked at the file - we interpret it as a 1 x 43 WBMP. All it has is the width and height; there is no following data.

The SkWbmpCodec reports that it has decoded 0 rows, and then SkCodec fills all rows with transparent.

The null dereference is here:

SkDiscardableMemory* SkDiscardableMemory::Create(size_t bytes) {
  return new SkDiscardableMemoryChrome(
      base::DiscardableMemoryAllocator::GetInstance()
          ->AllocateLockedDiscardableMemory(bytes));

Which would seem to indicate that GetInstance() returned null? But if it were null, that should happen any time we run this test, unless the image itself is causing a problem. It's possible we're stomping memory when we try to initialize the pixels, though I don't see anything obvious in the code that we're doing wrong.

I've uploaded https://skia-review.googlesource.com/c/skia/+/171006. This makes it so that we don't try to fill the pixels on this particular image. If applying this patch fixes the problem, this would suggest that we are stomping memory. (It wouldn't fix the problem in general, though it might be something we should submit anyway.)
The patch doesn't seem to prevent the issue when I applied it locally.
Thanks for trying! Since it didn't make a difference, I don't see how the decode itself could be relevant.
>But if it were null, that should happen any time we run this test

I wouldn't be so shocked if that happened. The crash seems to repro no matter what the 4th byte is.
I'm more suspicious that this is a problem with the way I've built this fuzzer because of the fact that this bug occurs in chromium source code and not skia source.
> The crash seems to repro no matter what the 4th byte is.

Oh, I don't think I was clear - I meant that if GetInstance() was returning null, we should get a similar crash no matter what the image is - a valid image would cause the same problem, since we'd still have to call GetInstance().
>Oh, I don't think I was clear - I meant that if GetInstance() was returning null, we should get a similar crash no matter what the image is - a valid image would cause the same problem, since we'd still have to call GetInstance().

This does seem to be the case.

I was able to prevent this problem by setting a discardable memory allocator before fuzzing starts.
I'm going to do a bit more investigating and then put up a patch.
Cc: scro...@google.com
Components: -Internals>Skia Tools>Stability>libFuzzer
Owner: metzman@chromium.org
Ok, great, thanks.
 Issue 905275  has been merged into this issue.
Labels: -ReleaseBlock-Beta -M-72
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d0984eb5712e3c0e0eeba5d99d73b8414c02d4e

commit 8d0984eb5712e3c0e0eeba5d99d73b8414c02d4e
Author: Jonathan Metzman <metzman@chromium.org>
Date: Mon Nov 19 17:15:06 2018

[skia] Initialize allocator properly before fuzzing and move targets

Set the discardable memory allocator before fuzzing to avoid spurious
null derefs when discardable memory is allocated.
Also, move fuzzer build config to
//skia/tools/fuzzers/BUILD.gn

Bug: 903632,  904613 
Change-Id: Ib987333706309eb470bcb988c53ddeebf5f5851e
Reviewed-on: https://chromium-review.googlesource.com/c/1336430
Commit-Queue: Jonathan Metzman <metzman@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609339}
[modify] https://crrev.com/8d0984eb5712e3c0e0eeba5d99d73b8414c02d4e/skia/BUILD.gn
[add] https://crrev.com/8d0984eb5712e3c0e0eeba5d99d73b8414c02d4e/skia/tools/fuzzers/BUILD.gn
[add] https://crrev.com/8d0984eb5712e3c0e0eeba5d99d73b8414c02d4e/skia/tools/fuzzers/fuzzer_environment.cc

Project Member

Comment 15 by ClusterFuzz, Nov 19

ClusterFuzz has detected this issue as fixed in range 609334:609340.

Detailed report: https://clusterfuzz.com/testcase?key=5515976856305664

Fuzzer: libFuzzer_skia_image_decode_fuzzer
Job Type: windows_libfuzzer_chrome_asan
Platform Id: windows

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  SkDiscardableMemory::Create
  SkBitmapCache::Alloc
  SkImage_Lazy::getROPixels
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=windows_libfuzzer_chrome_asan&range=607281:607292
Fixed: https://clusterfuzz.com/revisions?job=windows_libfuzzer_chrome_asan&range=609334:609340

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

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing_on_windows.md 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 16 by ClusterFuzz, Nov 19

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 5515976856305664 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
The documentation for reproducing bugs on Windows was moved to: https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md

Sign in to add a comment