Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386343002:
Make sure the fuzzer read size does not go negative. (Closed)
DescriptionMake sure the fuzzer read size does not go negative.
When fuzzing the image formats, its possible to get a read request which
would go negative. Handle the request and return FALSE for the read.
BUG=chromium:621836
Committed: https://pdfium.googlesource.com/pdfium/+/fb403875dd1bbf830d9325f10e6a5650db30c6fd
Patch Set 1 #
Total comments: 2
Patch Set 2 : Review feedback #
Total comments: 4
Patch Set 3 : Review feedback #
Total comments: 2
Patch Set 4 : Fix #Messages
Total messages: 28 (15 generated)
dsinclair@chromium.org changed reviewers: + thestig@chromium.org, tsepez@chromium.org
PTAL.
The CQ bit was checked by dsinclair@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...
https://codereview.chromium.org/2386343002/diff/1/testing/libfuzzer/xfa_codec... File testing/libfuzzer/xfa_codec_fuzzer.h (right): https://codereview.chromium.org/2386343002/diff/1/testing/libfuzzer/xfa_codec... testing/libfuzzer/xfa_codec_fuzzer.h:54: if (offset > m_size) Shouldn't we check this first in the function? Should we also check if offset < 0?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dsinclair@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...
https://codereview.chromium.org/2386343002/diff/1/testing/libfuzzer/xfa_codec... File testing/libfuzzer/xfa_codec_fuzzer.h (right): https://codereview.chromium.org/2386343002/diff/1/testing/libfuzzer/xfa_codec... testing/libfuzzer/xfa_codec_fuzzer.h:54: if (offset > m_size) On 2016/10/03 21:16:50, Lei Zhang wrote: > Shouldn't we check this first in the function? Should we also check if offset < > 0? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2386343002/diff/20001/testing/libfuzzer/xfa_c... File testing/libfuzzer/xfa_codec_fuzzer.h (right): https://codereview.chromium.org/2386343002/diff/20001/testing/libfuzzer/xfa_c... testing/libfuzzer/xfa_codec_fuzzer.h:52: if (offset < 0 || offset > m_size) Erm, and also shouldn't this be: offset >= m_size https://codereview.chromium.org/2386343002/diff/20001/testing/libfuzzer/xfa_c... testing/libfuzzer/xfa_codec_fuzzer.h:52: if (offset < 0 || offset > m_size) And reading a size of 0 doesn't make sense either.
https://codereview.chromium.org/2386343002/diff/20001/testing/libfuzzer/xfa_c... File testing/libfuzzer/xfa_codec_fuzzer.h (right): https://codereview.chromium.org/2386343002/diff/20001/testing/libfuzzer/xfa_c... testing/libfuzzer/xfa_codec_fuzzer.h:52: if (offset < 0 || offset > m_size) On 2016/10/04 18:35:13, Lei Zhang wrote: > And reading a size of 0 doesn't make sense either. Done. https://codereview.chromium.org/2386343002/diff/20001/testing/libfuzzer/xfa_c... testing/libfuzzer/xfa_codec_fuzzer.h:52: if (offset < 0 || offset > m_size) On 2016/10/04 18:35:13, Lei Zhang wrote: > Erm, and also shouldn't this be: offset >= m_size Done.
The CQ bit was checked by dsinclair@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2386343002/#ps40001 (title: "Review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2386343002/diff/40001/testing/libfuzzer/xfa_c... File testing/libfuzzer/xfa_codec_fuzzer.h (right): https://codereview.chromium.org/2386343002/diff/40001/testing/libfuzzer/xfa_c... testing/libfuzzer/xfa_codec_fuzzer.h:52: if (offset <= 0 || offset >= m_size) No, 0 is a valid offset.
The CQ bit was unchecked by thestig@chromium.org
https://codereview.chromium.org/2386343002/diff/40001/testing/libfuzzer/xfa_c... File testing/libfuzzer/xfa_codec_fuzzer.h (right): https://codereview.chromium.org/2386343002/diff/40001/testing/libfuzzer/xfa_c... testing/libfuzzer/xfa_codec_fuzzer.h:52: if (offset <= 0 || offset >= m_size) On 2016/10/04 19:13:15, Lei Zhang wrote: > No, 0 is a valid offset. Bugger, mis-read your previous comment.
PTAL.
lgtm
The CQ bit was checked by dsinclair@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2386343002/#ps60001 (title: "Fix")
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.
Description was changed from ========== Make sure the fuzzer read size does not go negative. When fuzzing the image formats, its possible to get a read request which would go negative. Handle the request and return FALSE for the read. BUG=chromium:621836 ========== to ========== Make sure the fuzzer read size does not go negative. When fuzzing the image formats, its possible to get a read request which would go negative. Handle the request and return FALSE for the read. BUG=chromium:621836 Committed: https://pdfium.googlesource.com/pdfium/+/fb403875dd1bbf830d9325f10e6a5650db30... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://pdfium.googlesource.com/pdfium/+/fb403875dd1bbf830d9325f10e6a5650db30... |
