Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1836573002:
Add an HttpStreamParser fuzzer. (Closed)
DescriptionAdd an HttpStreamParser fuzzer.
BUG=598037
Committed: https://crrev.com/44e8e9c8b39cd6fb611da0f905846652b0752511
Cr-Commit-Position: refs/heads/master@{#383777}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Response to comments #Patch Set 3 : Add comment #
Total comments: 21
Patch Set 4 : Response to comments #Patch Set 5 : Release buffer pointer earlier #
Total comments: 9
Patch Set 6 : Response to comments #Patch Set 7 : Oops #Messages
Total messages: 27 (9 generated)
mmenke@chromium.org changed reviewers: + eroman@chromium.org
Eric: WDYT? I was considering targetting the HttpNetworkTransaction layer, since it's so closely tied to the HttpStreamParser, but decided this way saved the effort of creating an HttpNetworkSession, and doesn't miss out on all that much. Thinking I'll add 3 seed cases to the server: Responses with/without content-length, and chunked encoding - know we have chunked parser tests, so that is a bit redundant. After playing a bit with fuzzing, pretty impressed by how it explores the space (Contrary to my skeptical email - I hadn't realize it did more than send random stuff at us).
On 2016/03/25 20:06:36, mmenke wrote: > Eric: WDYT? I was considering targetting the HttpNetworkTransaction layer, > since it's so closely tied to the HttpStreamParser, but decided this way saved > the effort of creating an HttpNetworkSession, and doesn't miss out on all that > much. > > Thinking I'll add 3 seed cases to the server: Responses with/without > content-length, and chunked encoding - know we have chunked parser tests, so > that is a bit redundant. After playing a bit with fuzzing, pretty impressed by > how it explores the space (Contrary to my skeptical email - I hadn't realize it > did more than send random stuff at us). Other tests I'm thinking of: SDCH tests (And filter tests in general - not sure how beaten on brotli code is, and even gzip couldn't hurt), proxy socket tests, more integration-y FTP tests, maybe some around our file naming / unescaping black magic.
Description was changed from ========== Add an HttpStreamParser fuzzer. BUG=none ========== to ========== Add an HttpStreamParser fuzzer. BUG=598037 ==========
+cc mmoroz https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:29: extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { I was advised to include: #include <stddef.h> #include <stdint.h> In past reviews for these. High chance of them getting included transitively, but may as well for consistency with other fuzzers. https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:34: if (size > std::numeric_limits<int>::max()) (This certainly will not happen, there is a limit to the sizes used; but good to be defensive) https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:35: size = std::numeric_limits<int>::max(); Since you are using int throughout the rest, I would suggest: auto int_size = base::saturated_cast<int>(size); And then just use int_size later. https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:43: for (net::IoMode io_mode : kIoModes) { optional nit: auto ? https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:48: // Split the response into three reads of about equal size. Hopefully this Not sure if you have seen this: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_chun... It shows a technique for breaking it up into chunk sizes that are a function of the input, so they are not always the same size. https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:97: while (result != net::OK) { Can ReadResponseBody() not return an error in this test?
https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:29: extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { On 2016/03/25 20:43:18, eroman wrote: > I was advised to include: > > #include <stddef.h> > #include <stdint.h> > > In past reviews for these. > > High chance of them getting included transitively, but may as well for > consistency with other fuzzers. Done. https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:34: if (size > std::numeric_limits<int>::max()) On 2016/03/25 20:43:18, eroman wrote: > (This certainly will not happen, there is a limit to the sizes used; but good to > be defensive) No longer relevant (Though if it shouldn't happen, checks like that should be a CHECK instead) https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:35: size = std::numeric_limits<int>::max(); On 2016/03/25 20:43:18, eroman wrote: > Since you are using int throughout the rest, I would suggest: > auto int_size = base::saturated_cast<int>(size); > > And then just use int_size later. I was being paranoid: 2*size can overflow, even if this check passes. Anyhow, modified code so it no longer matters. https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:43: for (net::IoMode io_mode : kIoModes) { On 2016/03/25 20:43:18, eroman wrote: > optional nit: auto ? Done. I tend to avoid auto except for ugly types, to make code clearer, but since kIoModes was declared just above, and the name makes it clear, auto seems fine. https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:48: // Split the response into three reads of about equal size. Hopefully this On 2016/03/25 20:43:18, eroman wrote: > Not sure if you have seen this: > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_chun... > > It shows a technique for breaking it up into chunk sizes that are a function of > the input, so they are not always the same size. That doesn't quite work here. For correct chunk-encoded cases, for instance, the first four bytes here are "HTTP/", and the last are "0\r\n\r\n" (At least I think the last chunk has a double CRLF?). Admittedly, we may not need correct data to produce read length errors, but that does make me very paranoid. So instead, I've gone ahead and used the last bytes exclusively as read lengths + sync/async indicators. I do wonder how this sort of silliness can affect the fuzzer's search patterns. https://codereview.chromium.org/1836573002/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_fuzzer.cc:97: while (result != net::OK) { On 2016/03/25 20:43:18, eroman wrote: > Can ReadResponseBody() not return an error in this test? This is a bug. This should be while(result > 0). Yes, GetResult can return an error. The bug was actually the success case (net::OK is actually never returned, even on connection close, I believe, though we should exit the loop if that happens)
BTW cool, thanks for writing this! https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:34: extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { It would be helpful to have a high-level overview of what this fuzzer does here. There is good documentation inside about the individual steps taking (thanks!), but I still recommend a short overview of how the stream parser is being driven. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:36: base::MessageLoopForIO message_loop; I wonder if it would be faster to extract the message_loop to a singleton, since LLVMFuzzerTestOneInput() is going to be called repeatedly. Can explore that later I guess. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:44: // the size of type of each read. "size of type" ==> "size and type" https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:52: size_t read_seed = data[size - 1]; Max can advise on what works best for the fuzzer. I wonder if it would be advantageous to increase the ways in which the chunk sizes and types are determined. Right now if I understand, it is highly dependent on the last byte of the data, and then the rest follow from that. Some ideas (don't trust me though, listen to whatever Max suggests): * Pull this byte out of the data that is fed to parser so it can change independently, and can generate many permutations of chunks for the same data processed * Mix in data from other bytes of the input, to increase ways of modifying the chunking. Like perhaps the first byte, or the previous read_seed. XOR this with some other bytes from the data to increase the combinations reachable. Like for instance pulling a byte out of the input (not feeding to parser) and using that to influence this, or XORing with he previous read_seed. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:55: // High order byte determines IoMode. "high order byte" ==> "high order bit" https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:56: if (read_seed & 0x80) Just thinking out loud, but if we are generating lots of ASCII data this will be less likely to be hit. I wonder if taking the least significant bit instead would give more uniformity. For then getting the read_size doesn't really matter if that bit is re-used, can still find it as (0x3F & read_seed). https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:59: // Low order 6 bytes determine how many bytes are returned by the read. bytes ==> bits https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:71: if (size > 0) Isn't |size| going to be 0 after the while-loop above? Alternately for getting a single bit worth of data, Max suggested to me in the past this pattern: if (size > 1) { if ((data[0] ^ data[size - 1]) & 1) io_mode = net::SYNCHRONOUS; } https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:114: scoped_refptr<net::IOBufferWithSize> io_buffer( Would it make sense to allocate this once outside of the while-loop ?
https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:34: extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { On 2016/03/26 01:39:29, eroman wrote: > It would be helpful to have a high-level overview of what this fuzzer does here. > > There is good documentation inside about the individual steps taking (thanks!), > but I still recommend a short overview of how the stream parser is being driven. Added a short comment. Happy to add more, just not sure what else is worth saying. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:36: base::MessageLoopForIO message_loop; On 2016/03/26 01:39:28, eroman wrote: > I wonder if it would be faster to extract the message_loop to a singleton, since > LLVMFuzzerTestOneInput() is going to be called repeatedly. > > Can explore that later I guess. I think MessageLoops aren't too heavy weight? Only kernel object they need is a lock. Experimentally, making it a global made exec/s per sec go from a varying 1100-1200 runs/sec to a more consistent 1250. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:44: // the size of type of each read. On 2016/03/26 01:39:28, eroman wrote: > "size of type" ==> "size and type" Done. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:52: size_t read_seed = data[size - 1]; On 2016/03/26 01:39:29, eroman wrote: > Max can advise on what works best for the fuzzer. > > I wonder if it would be advantageous to increase the ways in which the chunk > sizes and types are determined. Right now if I understand, it is highly > dependent on the last byte of the data, and then the rest follow from that. > > Some ideas (don't trust me though, listen to whatever Max suggests): > > * Pull this byte out of the data that is fed to parser so it can change > independently, and can generate many permutations of chunks for the same data > processed > > * Mix in data from other bytes of the input, to increase ways of modifying the > chunking. Like perhaps the first byte, or the previous read_seed. > XOR this with some other bytes from the data to increase the combinations > reachable. Like for instance pulling a byte out of the input (not feeding to > parser) and using that to influence this, or XORing with he previous read_seed. You're misunderstanding the code. This code uses the last bytes exclusively for read sizes - they don't appear in any read. This avoids dual use data , which I'm highly skeptical of. Just consider a chunked-encoded HTTP stream. Both the first and last bytes are always the same. Could also use a hash, but I figure this is simpler. It does affect how we walk throguh the search space a bit, though. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:55: // High order byte determines IoMode. On 2016/03/26 01:39:29, eroman wrote: > "high order byte" ==> "high order bit" Done. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:56: if (read_seed & 0x80) On 2016/03/26 01:39:28, eroman wrote: > Just thinking out loud, but if we are generating lots of ASCII data this will be > less likely to be hit. > > I wonder if taking the least significant bit instead would give more uniformity. > > For then getting the read_size doesn't really matter if that bit is re-used, can > still find it as (0x3F & read_seed). Done. Since these bytes are not used as part of the body, not sure if they have an ASCII bias, though given the transforms the fuzzer applies, perhaps they do. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:59: // Low order 6 bytes determine how many bytes are returned by the read. On 2016/03/26 01:39:29, eroman wrote: > bytes ==> bits Done. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:71: if (size > 0) On 2016/03/26 01:39:28, eroman wrote: > Isn't |size| going to be 0 after the while-loop above? > > Alternately for getting a single bit worth of data, Max suggested to me in the > past this pattern: > > if (size > 1) { > if ((data[0] ^ data[size - 1]) & 1) > io_mode = net::SYNCHRONOUS; > } data[0] here is almost always 'H', and the last byte can have special properties now (Well...I guess not). Note that the above loop ends when there are 0 or 1 remaining bytes, since if there's 1, can't use it is a code for how many bytes to read. That having been said, because of the std::min above, it's usually 0. There's still some bits left over, though...Let's use them. https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:114: scoped_refptr<net::IOBufferWithSize> io_buffer( On 2016/03/26 01:39:29, eroman wrote: > Would it make sense to allocate this once outside of the while-loop ? Freeing it is more likely to catch a use-after-free, though, which is why I went with this approach. So a bit of a tradeoff of coverage vs performance here.
https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:71: if (size > 0) On 2016/03/28 15:26:26, mmenke wrote: > On 2016/03/26 01:39:28, eroman wrote: > > Isn't |size| going to be 0 after the while-loop above? > > > > Alternately for getting a single bit worth of data, Max suggested to me in the > > past this pattern: > > > > if (size > 1) { > > if ((data[0] ^ data[size - 1]) & 1) > > io_mode = net::SYNCHRONOUS; > > } > > data[0] here is almost always 'H', and the last byte can have special properties > now (Well...I guess not). Note that the above loop ends when there are 0 or 1 > remaining bytes, since if there's 1, can't use it is a code for how many bytes > to read. That having been said, because of the std::min above, it's usually 0. > There's still some bits left over, though...Let's use them. Erm...that "special properties / I guess not" was not well explained. For chunked-encoding, the final chunk is always 0\r\n\r\n, I believe (We may allow 0\n\n?). Of course, there can be data *after* the last chunk, so it may not have special properties afterall. Seems most robust just to use separate bytes only for the meta-data, though.
LGTM! I'm excited about adding other similar tests that fuzz from socket-level inputs https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:42: // Break the buffer into a sequence of variable sized sync and async optional nit: "sync and async reads of various sizes" https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:52: size_t read_seed = data[size - 1]; On 2016/03/28 15:26:26, mmenke wrote: > On 2016/03/26 01:39:29, eroman wrote: > > Max can advise on what works best for the fuzzer. > > > > I wonder if it would be advantageous to increase the ways in which the chunk > > sizes and types are determined. Right now if I understand, it is highly > > dependent on the last byte of the data, and then the rest follow from that. > > > > Some ideas (don't trust me though, listen to whatever Max suggests): > > > > * Pull this byte out of the data that is fed to parser so it can change > > independently, and can generate many permutations of chunks for the same data > > processed > > > > * Mix in data from other bytes of the input, to increase ways of modifying > the > > chunking. Like perhaps the first byte, or the previous read_seed. > > XOR this with some other bytes from the data to increase the combinations > > reachable. Like for instance pulling a byte out of the input (not feeding to > > parser) and using that to influence this, or XORing with he previous > read_seed. > > You're misunderstanding the code. This code uses the last bytes exclusively for > read sizes - they don't appear in any read. This avoids dual use data , which > I'm highly skeptical of. Just consider a chunked-encoded HTTP stream. Both the > first and last bytes are always the same. > > Could also use a hash, but I figure this is simpler. It does affect how we walk > throguh the search space a bit, though. I see, I had indeed read the code wrong. The new comments help; I think your approach is sound. https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:42: net::MockWrite writes[] = { Consider extracting this to a constant or function-level static. https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:64: size_t read_seed = data[size - 1]; Why size_t rather than uint8_t ? Is this so it plays nice with the subsequent std::min ? Fine either way, just curious. https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:71: // Use second bit determine if the last read, when the socket is closed, is bit determine --> bit to determine https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:72: // schronous. Only the second bit of the last time this loop runs matters. sycronous --> synchronous (and yes, misspelled the misspelling...)
On 2016/03/28 22:54:57, eroman wrote: > LGTM! > > I'm excited about adding other similar tests that fuzz from socket-level inputs I too am pretty excited about this stuff - I started out very skeptical. This test does not test sync/async deletion and cancellation. Wonder if we should think about adding that at some future point in time. Seems like it could get a little tricky to cover all cases (While reading headers, while reading body, etc, and we don't even know if we'll make it far enough to read the body). Anyhow, I'll address comments tomorrow, and leave it alone otherwise, for now, just musing on future potential improvements. > https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... > File net/http/http_stream_parser_fuzzer.cc (right): > > https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... > net/http/http_stream_parser_fuzzer.cc:42: // Break the buffer into a sequence of > variable sized sync and async > optional nit: > > "sync and async reads of various sizes" > > https://codereview.chromium.org/1836573002/diff/30001/net/http/http_stream_pa... > net/http/http_stream_parser_fuzzer.cc:52: size_t read_seed = data[size - 1]; > On 2016/03/28 15:26:26, mmenke wrote: > > On 2016/03/26 01:39:29, eroman wrote: > > > Max can advise on what works best for the fuzzer. > > > > > > I wonder if it would be advantageous to increase the ways in which the chunk > > > sizes and types are determined. Right now if I understand, it is highly > > > dependent on the last byte of the data, and then the rest follow from that. > > > > > > Some ideas (don't trust me though, listen to whatever Max suggests): > > > > > > * Pull this byte out of the data that is fed to parser so it can change > > > independently, and can generate many permutations of chunks for the same > data > > > processed > > > > > > * Mix in data from other bytes of the input, to increase ways of modifying > > the > > > chunking. Like perhaps the first byte, or the previous read_seed. > > > XOR this with some other bytes from the data to increase the combinations > > > reachable. Like for instance pulling a byte out of the input (not feeding to > > > parser) and using that to influence this, or XORing with he previous > > read_seed. > > > > You're misunderstanding the code. This code uses the last bytes exclusively > for > > read sizes - they don't appear in any read. This avoids dual use data , which > > I'm highly skeptical of. Just consider a chunked-encoded HTTP stream. Both > the > > first and last bytes are always the same. > > > > Could also use a hash, but I figure this is simpler. It does affect how we > walk > > throguh the search space a bit, though. > > I see, I had indeed read the code wrong. > > The new comments help; I think your approach is sound. > > https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... > File net/http/http_stream_parser_fuzzer.cc (right): > > https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... > net/http/http_stream_parser_fuzzer.cc:42: net::MockWrite writes[] = { > Consider extracting this to a constant or function-level static. > > https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... > net/http/http_stream_parser_fuzzer.cc:64: size_t read_seed = data[size - 1]; > Why size_t rather than uint8_t ? Is this so it plays nice with the subsequent > std::min ? Fine either way, just curious. > > https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... > net/http/http_stream_parser_fuzzer.cc:71: // Use second bit determine if the > last read, when the socket is closed, is > bit determine --> bit to determine > > https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... > net/http/http_stream_parser_fuzzer.cc:72: // schronous. Only the second bit of > the last time this loop runs matters. > sycronous --> synchronous > > > (and yes, misspelled the misspelling...)
Thanks for the review! Note that this doesn't check async writes, multiple writes, write failures, uploads, custom headers or upload write failures. Remote sites have basically free reign over uploads and (most) header, but I'm not sure there's much benefit to adding them here, from a "defense against attack (the dark arts?) standpoint", though may be useful from a more general bug testing standpoint. https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:42: net::MockWrite writes[] = { On 2016/03/28 22:54:57, eroman wrote: > Consider extracting this to a constant or function-level static. Note that we're actually calling the initializer here, so our presubmit checks may have issues with it. I've gone ahead and made it an inline constant, but not sure if compilers are smart enough to recognize it can be evaluated at compile time. https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:64: size_t read_seed = data[size - 1]; On 2016/03/28 22:54:57, eroman wrote: > Why size_t rather than uint8_t ? Is this so it plays nice with the subsequent > std::min ? Fine either way, just curious. That's exactly it. I prefer to avoid casts unless necessary. https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:71: // Use second bit determine if the last read, when the socket is closed, is On 2016/03/28 22:54:57, eroman wrote: > bit determine --> bit to determine Done. https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:72: // schronous. Only the second bit of the last time this loop runs matters. On 2016/03/28 22:54:57, eroman wrote: > sycronous --> synchronous > > > (and yes, misspelled the misspelling...) Done (And fixed the next sentence as well)
https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... File net/http/http_stream_parser_fuzzer.cc (right): https://codereview.chromium.org/1836573002/diff/70001/net/http/http_stream_pa... net/http/http_stream_parser_fuzzer.cc:42: net::MockWrite writes[] = { On 2016/03/29 16:38:37, mmenke wrote: > On 2016/03/28 22:54:57, eroman wrote: > > Consider extracting this to a constant or function-level static. > > Note that we're actually calling the initializer here, so our presubmit checks > may have issues with it. I've gone ahead and made it an inline constant, but > not sure if compilers are smart enough to recognize it can be evaluated at > compile time. Oops...SequencedSocketData doesn't like const arguments. I'll land without this change, may modify SequencedSocketData's constructor in a followup, but probably won't get to it.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1836573002/#ps80001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836573002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836573002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Nice one, thanks for implementing this! Do you have any seed corpus which would be useful for fuzzing? Would it be useful to have a dictionary with HTTP keywords and headers as well?
On 2016/03/29 17:23:08, mmoroz wrote: > Nice one, thanks for implementing this! Do you have any seed corpus which would > be useful for fuzzing? Would it be useful to have a dictionary with HTTP > keywords and headers as well? I'm thinking I'll upload 3 test cases: * No content-length header * Content-length header * Chunked-encoding. HttpStreamParser doesn't have a whole lot of header logic, beyond just parsing generic HTTP headers. May be one or two with special logic, which I can make sure I cover as well.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1836573002/#ps100001 (title: "Oops")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836573002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836573002/100001
Message was sent while issue was closed.
Description was changed from ========== Add an HttpStreamParser fuzzer. BUG=598037 ========== to ========== Add an HttpStreamParser fuzzer. BUG=598037 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add an HttpStreamParser fuzzer. BUG=598037 ========== to ========== Add an HttpStreamParser fuzzer. BUG=598037 Committed: https://crrev.com/44e8e9c8b39cd6fb611da0f905846652b0752511 Cr-Commit-Position: refs/heads/master@{#383777} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/44e8e9c8b39cd6fb611da0f905846652b0752511 Cr-Commit-Position: refs/heads/master@{#383777} |
