Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813693002:
Stop reporting OOM as errors in libpng fuzzers (Closed)
|
Created:
3 years, 8 months ago by scroggo_chromium Modified:
3 years, 8 months ago CC:
chromium-reviews, fuzzing_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop reporting OOM as errors in libpng fuzzers
Use a custom allocator to make malloc fail beyond an arbitrary max.
Set the maximum to match the default png_user_chunk_malloc_max.
BUG=673082
Review-Url: https://codereview.chromium.org/2813693002
Cr-Commit-Position: refs/heads/master@{#464972}
Committed: https://chromium.googlesource.com/chromium/src/+/645a85acd14bdef0471070b9f928b0c29c3d3dae
Patch Set 1 #
Total comments: 2
Patch Set 2 : Always limit malloc #Messages
Total messages: 20 (10 generated)
scroggo@chromium.org changed reviewers: + kcc@chromium.org, mmoroz@chromium.org, msarett@google.com
msarett@chromium.org changed reviewers: + msarett@chromium.org
lgtm
The CQ bit was checked by scroggo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:15: #ifdef MEMORY_SANITIZER Why #ifdef MEMORY_SANITIZER? Isn't this relevant to any other build mode?
https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:15: #ifdef MEMORY_SANITIZER On 2017/04/17 18:14:28, kcc2 wrote: > Why #ifdef MEMORY_SANITIZER? > Isn't this relevant to any other build mode? I would have thought so, too, but https://bugs.chromium.org/p/chromium/issues/detail?id=673082#c18 suggests we only set the limit when using MEMORY_SANITIZER. I supposed that to mean that this was only a problem when using msan - i.e. we successfully allocate this large amount otherwise. When I run locally, not using msan, we do not crash trying to decode the test image, but a LargeAlloc is reported.
On 2017/04/17 18:51:53, scroggo_chromium wrote: > https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... > File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): > > https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... > testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:15: #ifdef MEMORY_SANITIZER > On 2017/04/17 18:14:28, kcc2 wrote: > > Why #ifdef MEMORY_SANITIZER? > > Isn't this relevant to any other build mode? > > I would have thought so, too, but > https://bugs.chromium.org/p/chromium/issues/detail?id=673082#c18 suggests we > only set the limit when using MEMORY_SANITIZER. I supposed that to mean that > this was only a problem when using msan - i.e. we successfully allocate this > large amount otherwise. When I run locally, not using msan, we do not crash > trying to decode the test image, but a LargeAlloc is reported. msan consumes much more memory than e.g. ubsan build, and a bit more memory than asan build, so an input there msan OOMs *may* not OOM under asan or ubsan. But imho this kind of change should not be msan-specific at all.
On 2017/04/17 18:53:19, kcc2 wrote: > On 2017/04/17 18:51:53, scroggo_chromium wrote: > > > https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... > > File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): > > > > > https://codereview.chromium.org/2813693002/diff/1/testing/libfuzzer/fuzzers/l... > > testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:15: #ifdef MEMORY_SANITIZER > > On 2017/04/17 18:14:28, kcc2 wrote: > > > Why #ifdef MEMORY_SANITIZER? > > > Isn't this relevant to any other build mode? > > > > I would have thought so, too, but > > https://bugs.chromium.org/p/chromium/issues/detail?id=673082#c18 suggests we > > only set the limit when using MEMORY_SANITIZER. I supposed that to mean that > > this was only a problem when using msan - i.e. we successfully allocate this > > large amount otherwise. When I run locally, not using msan, we do not crash > > trying to decode the test image, but a LargeAlloc is reported. > > msan consumes much more memory than e.g. ubsan build, and a bit more memory than > asan build, > so an input there msan OOMs *may* not OOM under asan or ubsan. > But imho this kind of change should not be msan-specific at all. Uploaded patch set 2, which uses the custom allocator on all builds.
The CQ bit was checked by kcc@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org Link to the patchset: https://codereview.chromium.org/2813693002/#ps20001 (title: "Always limit malloc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492455758096680,
"parent_rev": "c11f0c9e5d3d186b06c279a13d2b94769b985d79", "commit_rev":
"645a85acd14bdef0471070b9f928b0c29c3d3dae"}
Message was sent while issue was closed.
Description was changed from ========== Stop reporting OOM as errors in libpng fuzzers Use a custom allocator to make malloc fail beyond an arbitrary max. Set the maximum to match the default png_user_chunk_malloc_max. BUG=673082 ========== to ========== Stop reporting OOM as errors in libpng fuzzers Use a custom allocator to make malloc fail beyond an arbitrary max. Set the maximum to match the default png_user_chunk_malloc_max. BUG=673082 Review-Url: https://codereview.chromium.org/2813693002 Cr-Commit-Position: refs/heads/master@{#464972} Committed: https://chromium.googlesource.com/chromium/src/+/645a85acd14bdef0471070b9f928... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/645a85acd14bdef0471070b9f928...
Message was sent while issue was closed.
Thanks Leon and Kostya! LGTM! |
