Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377293002:
[libfuzzer] libpng_read_fuzzer: call png_set_user_limits() for MSan. (Closed)
Description[libfuzzer] libpng_read_fuzzer: call png_set_user_limits() for MSan.
To avoid OOM with MSan (crbug.com/648073). These values are recommended as
safe settings by https://github.com/glennrp/libpng/blob/libpng16/pngusr.dfa
R=aizatsky@chromium.org, msarett@chromium.org
BUG=648073
Committed: https://crrev.com/0ceee9d387bb17c24adefffd9927ebaf68a3df1f
Cr-Commit-Position: refs/heads/master@{#423208}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase onto fresh master checkout. #
Messages
Total messages: 14 (3 generated)
https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:49: #ifdef MEMORY_SANITIZER why not to make it unconditional? Seems like a good idea anyway. It is a matter of time it will OOM under any sanitizer, isn't it?
https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:49: #ifdef MEMORY_SANITIZER On 2016/09/29 17:14:40, aizatsky wrote: > why not to make it unconditional? Seems like a good idea anyway. It is a matter > of time it will OOM under any sanitizer, isn't it? I would like to agree, but: 1) we lose some coverage by enabling this: https://codereview.chromium.org/2383023002/diff2/1:20001/testing/libfuzzer/fu... 2) other builds are far enough from hitting OOM ("Peak RSS" column): https://cluster-fuzz.appspot.com/fuzzerstats?fuzzer_name=libfuzzer_libpng_rea... 3) we ship Chrome with default values from https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=210, so it makes sense to fuzz that version until we get blocked Also I have a feeling that we may not hit OOM with, let's say, 999999 x 999999 image, since there are other limits like https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=225 preventing us from allocating too large buffers https://cs.chromium.org/chromium/src/third_party/libpng/pngrutil.c?rcl=147520..., so for me it seems to be MSan only :(
lgtm https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:49: #ifdef MEMORY_SANITIZER On 2016/09/30 at 08:28:59, mmoroz wrote: > On 2016/09/29 17:14:40, aizatsky wrote: > > why not to make it unconditional? Seems like a good idea anyway. It is a matter > > of time it will OOM under any sanitizer, isn't it? > > I would like to agree, but: > 1) we lose some coverage by enabling this: https://codereview.chromium.org/2383023002/diff2/1:20001/testing/libfuzzer/fu... > > 2) other builds are far enough from hitting OOM ("Peak RSS" column): https://cluster-fuzz.appspot.com/fuzzerstats?fuzzer_name=libfuzzer_libpng_rea... > > 3) we ship Chrome with default values from https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=210, so it makes sense to fuzz that version until we get blocked > > > Also I have a feeling that we may not hit OOM with, let's say, 999999 x 999999 image, since there are other limits like https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=225 preventing us from allocating too large buffers https://cs.chromium.org/chromium/src/third_party/libpng/pngrutil.c?rcl=147520..., so for me it seems to be MSan only :( Understood. Please try to make it an if() statement rather than preprocessor then.
https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:49: #ifdef MEMORY_SANITIZER On 2016/09/30 17:51:52, aizatsky wrote: > On 2016/09/30 at 08:28:59, mmoroz wrote: > > On 2016/09/29 17:14:40, aizatsky wrote: > > > why not to make it unconditional? Seems like a good idea anyway. It is a > matter > > > of time it will OOM under any sanitizer, isn't it? > > > > I would like to agree, but: > > 1) we lose some coverage by enabling this: > https://codereview.chromium.org/2383023002/diff2/1:20001/testing/libfuzzer/fu... > > > > 2) other builds are far enough from hitting OOM ("Peak RSS" column): > https://cluster-fuzz.appspot.com/fuzzerstats?fuzzer_name=libfuzzer_libpng_rea... > > > > 3) we ship Chrome with default values from > https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=210, so > it makes sense to fuzz that version until we get blocked > > > > > > Also I have a feeling that we may not hit OOM with, let's say, 999999 x 999999 > image, since there are other limits like > https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=225 > preventing us from allocating too large buffers > https://cs.chromium.org/chromium/src/third_party/libpng/pngrutil.c?rcl=147520..., > so for me it seems to be MSan only :( > > > Understood. Please try to make it an if() statement rather than preprocessor > then. I've searched for something to do that, but haven't found a convenient way. Everything I tried depends on preprocessor and is undefined for non-MSan build. Also I've asked Dmitry and Alex from your team and they hadn't suggested any way to do that. Can I land this version?
On 2016/10/04 at 12:38:26, mmoroz wrote: > https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... > File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): > > https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... > testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:49: #ifdef MEMORY_SANITIZER > On 2016/09/30 17:51:52, aizatsky wrote: > > On 2016/09/30 at 08:28:59, mmoroz wrote: > > > On 2016/09/29 17:14:40, aizatsky wrote: > > > > why not to make it unconditional? Seems like a good idea anyway. It is a > > matter > > > > of time it will OOM under any sanitizer, isn't it? > > > > > > I would like to agree, but: > > > 1) we lose some coverage by enabling this: > > https://codereview.chromium.org/2383023002/diff2/1:20001/testing/libfuzzer/fu... > > > > > > 2) other builds are far enough from hitting OOM ("Peak RSS" column): > > https://cluster-fuzz.appspot.com/fuzzerstats?fuzzer_name=libfuzzer_libpng_rea... > > > > > > 3) we ship Chrome with default values from > > https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=210, so > > it makes sense to fuzz that version until we get blocked > > > > > > > > > Also I have a feeling that we may not hit OOM with, let's say, 999999 x 999999 > > image, since there are other limits like > > https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=225 > > preventing us from allocating too large buffers > > https://cs.chromium.org/chromium/src/third_party/libpng/pngrutil.c?rcl=147520..., > > so for me it seems to be MSan only :( > > > > > > Understood. Please try to make it an if() statement rather than preprocessor > > then. > > I've searched for something to do that, but haven't found a convenient way. Everything I tried depends on preprocessor and is undefined for non-MSan build. > > Also I've asked Dmitry and Alex from your team and they hadn't suggested any way to do that. > > Can I land this version? In compiler-rt we usually have conditional define and later: #ifdef MEMORY_SANITIZER #define MEMORY_SANITIZER_ENABLED 1 #else #define MEMORY_SANITIZER_ENABLED 0 And later you can do if (MEMORY_SANITIZER_ENABLED) and compiler will check everything. Feel free to submit this if such macro is not available.
lgtm
On 2016/10/04 20:48:40, aizatsky wrote: > On 2016/10/04 at 12:38:26, mmoroz wrote: > > > https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... > > File testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc (right): > > > > > https://codereview.chromium.org/2377293002/diff/1/testing/libfuzzer/fuzzers/l... > > testing/libfuzzer/fuzzers/libpng_read_fuzzer.cc:49: #ifdef MEMORY_SANITIZER > > On 2016/09/30 17:51:52, aizatsky wrote: > > > On 2016/09/30 at 08:28:59, mmoroz wrote: > > > > On 2016/09/29 17:14:40, aizatsky wrote: > > > > > why not to make it unconditional? Seems like a good idea anyway. It is a > > > matter > > > > > of time it will OOM under any sanitizer, isn't it? > > > > > > > > I would like to agree, but: > > > > 1) we lose some coverage by enabling this: > > > > https://codereview.chromium.org/2383023002/diff2/1:20001/testing/libfuzzer/fu... > > > > > > > > 2) other builds are far enough from hitting OOM ("Peak RSS" column): > > > > https://cluster-fuzz.appspot.com/fuzzerstats?fuzzer_name=libfuzzer_libpng_rea... > > > > > > > > 3) we ship Chrome with default values from > > > https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=210, > so > > > it makes sense to fuzz that version until we get blocked > > > > > > > > > > > > Also I have a feeling that we may not hit OOM with, let's say, 999999 x > 999999 > > > image, since there are other limits like > > > https://cs.chromium.org/chromium/src/third_party/libpng/pnglibconf.h?l=225 > > > preventing us from allocating too large buffers > > > > https://cs.chromium.org/chromium/src/third_party/libpng/pngrutil.c?rcl=147520..., > > > so for me it seems to be MSan only :( > > > > > > > > > Understood. Please try to make it an if() statement rather than preprocessor > > > then. > > > > I've searched for something to do that, but haven't found a convenient way. > Everything I tried depends on preprocessor and is undefined for non-MSan build. > > > > Also I've asked Dmitry and Alex from your team and they hadn't suggested any > way to do that. > > > > Can I land this version? > > In compiler-rt we usually have conditional define and later: > > #ifdef MEMORY_SANITIZER > #define MEMORY_SANITIZER_ENABLED 1 > #else > #define MEMORY_SANITIZER_ENABLED 0 > > > And later you can do > > if (MEMORY_SANITIZER_ENABLED) > > and compiler will check everything. Feel free to submit this if such macro is > not available. The only things I've found are: https://cs.chromium.org/chromium/src/third_party/llvm/include/llvm/Support/Co... and https://cs.chromium.org/chromium/src/third_party/webrtc/base/sanitizer.h?q=%2... sadly they neither of them seems to be a convenient option. Landing current version for now.
The CQ bit was checked by mmoroz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aizatsky@chromium.org Link to the patchset: https://codereview.chromium.org/2377293002/#ps20001 (title: "Rebase onto fresh master checkout.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [libfuzzer] libpng_read_fuzzer: call png_set_user_limits() for MSan. To avoid OOM with MSan (crbug.com/648073). These values are recommended as safe settings by https://github.com/glennrp/libpng/blob/libpng16/pngusr.dfa R=aizatsky@chromium.org, msarett@chromium.org BUG=648073 ========== to ========== [libfuzzer] libpng_read_fuzzer: call png_set_user_limits() for MSan. To avoid OOM with MSan (crbug.com/648073). These values are recommended as safe settings by https://github.com/glennrp/libpng/blob/libpng16/pngusr.dfa R=aizatsky@chromium.org, msarett@chromium.org BUG=648073 Committed: https://crrev.com/0ceee9d387bb17c24adefffd9927ebaf68a3df1f Cr-Commit-Position: refs/heads/master@{#423208} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0ceee9d387bb17c24adefffd9927ebaf68a3df1f Cr-Commit-Position: refs/heads/master@{#423208} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
