Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1509163002:
fuzzer for net::HttpChunkedDecoder implemented (Closed)
Descriptionfuzzer for net::HttpChunkedDecoder implemented
R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org
BUG=566504
Committed: https://crrev.com/e52cf8a5b9a4bc3cd519d4b03be69345496b42fa
Cr-Commit-Position: refs/heads/master@{#364478}
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove unnecessary casts for data pointer #
Messages
Total messages: 17 (2 generated)
lgtm
https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:12: std::vector<char> buffer(data_ptr, data_ptr + size); everything here is non-const. Is there a chance FitlerBuf will change it?
On 2015/12/08 18:23:01, aizatsky wrote: > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:12: std::vector<char> > buffer(data_ptr, data_ptr + size); > everything here is non-const. Is there a chance FitlerBuf will change it? I see a memmove - https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_chun...
On 2015/12/08 18:27:08, aarya wrote: > On 2015/12/08 18:23:01, aizatsky wrote: > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): > > > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:12: std::vector<char> > > buffer(data_ptr, data_ptr + size); > > everything here is non-const. Is there a chance FitlerBuf will change it? > > I see a memmove - > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_chun... The data should be copied then.
On 2015/12/08 18:32:01, aizatsky wrote: > On 2015/12/08 18:27:08, aarya wrote: > > On 2015/12/08 18:23:01, aizatsky wrote: > > > > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > > File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): > > > > > > > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > > testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:12: > std::vector<char> > > > buffer(data_ptr, data_ptr + size); > > > everything here is non-const. Is there a chance FitlerBuf will change it? > > > > I see a memmove - > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_chun... > > The data should be copied then. Should I fix anything?
On 2015/12/09 09:42:56, mmoroz wrote: > On 2015/12/08 18:32:01, aizatsky wrote: > > On 2015/12/08 18:27:08, aarya wrote: > > > On 2015/12/08 18:23:01, aizatsky wrote: > > > > > > > > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > > > File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > > > testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:12: > > std::vector<char> > > > > buffer(data_ptr, data_ptr + size); > > > > everything here is non-const. Is there a chance FitlerBuf will change it? > > > > > > I see a memmove - > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_chun... > > > > The data should be copied then. > > Should I fix anything? Yes instead of directly using data, you need to make a copy of it before passing to FilterBuf. We should look at this for other fuzzers too in case they modify the data passed to them. This is problematic for several reasons like reproducing the testcase back, might be some other issues too since data is maintained by libfuzzer.
On 2015/12/09 15:56:58, aarya wrote: > On 2015/12/09 09:42:56, mmoroz wrote: > > On 2015/12/08 18:32:01, aizatsky wrote: > > > On 2015/12/08 18:27:08, aarya wrote: > > > > On 2015/12/08 18:23:01, aizatsky wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > > > > File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > > > > > testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:12: > > > std::vector<char> > > > > > buffer(data_ptr, data_ptr + size); > > > > > everything here is non-const. Is there a chance FitlerBuf will change > it? > > > > > > > > I see a memmove - > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_chun... > > > > > > The data should be copied then. > > > > Should I fix anything? > > Yes instead of directly using data, you need to make a copy of it before passing > to FilterBuf. We should look at this for other fuzzers too in case they modify > the data passed to them. This is problematic for several reasons like > reproducing the testcase back, might be some other issues too since data is > maintained by libfuzzer. Got it, but actually I do copy data into the vector and then pass to the decoder. Real input from the fuzzer is not modified, because constructor of vector copies data. Am I wrong? I used vector for that just to avoid manual allocation/freeing of the memory.
https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:11: char* data_ptr = reinterpret_cast<char*>(const_cast<unsigned char*>(data)); remove this const_cast since you are copying the data.
On 2015/12/09 18:11:07, aizatsky wrote: > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > File testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc (right): > > https://codereview.chromium.org/1509163002/diff/1/testing/libfuzzer/fuzzers/h... > testing/libfuzzer/fuzzers/http_chunked_decoder_fuzzer.cc:11: char* data_ptr = > reinterpret_cast<char*>(const_cast<unsigned char*>(data)); > remove this const_cast since you are copying the data. Oh, yes, it is redundant. Thanks!
lgtm
lgtm
The CQ bit was checked by aarya@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1509163002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1509163002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== fuzzer for net::HttpChunkedDecoder implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org BUG=566504 ========== to ========== fuzzer for net::HttpChunkedDecoder implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org BUG=566504 Committed: https://crrev.com/e52cf8a5b9a4bc3cd519d4b03be69345496b42fa Cr-Commit-Position: refs/heads/master@{#364478} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e52cf8a5b9a4bc3cd519d4b03be69345496b42fa Cr-Commit-Position: refs/heads/master@{#364478} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
