Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917503002:
URLRequest fuzzer. (Closed)
DescriptionURLRequest fuzzer.
This fuzzer covers much of the network stack, with everything wired up.
Unfortunately, it does have a couple dependencies on the system it's
running on (uses NTLM/Neogiate platform stores and current time, not
sure what else).
It covers so much code that it's unlikely to be able to catch
everything, so most modules it should also be fuzzed independently.
BUG=602440
Committed: https://crrev.com/c951d41c1a0df97a0b6a17379f8f7c11989aa80f
Cr-Commit-Position: refs/heads/master@{#390440}
Patch Set 1 #Patch Set 2 : Cleanups #Patch Set 3 : Remove tab #
Total comments: 1
Patch Set 4 : Update other fuzzers (Lost them in a merge) #
Total comments: 55
Patch Set 5 : Response to comments #
Total comments: 2
Patch Set 6 : Response to comments #Patch Set 7 : self review #
Total comments: 2
Patch Set 8 : merge #
Total comments: 15
Patch Set 9 : Response to comments #Patch Set 10 : Minor cleanup #Patch Set 11 : Remove unnecessary const #Patch Set 12 : Add missing include #
Total comments: 4
Messages
Total messages: 42 (16 generated)
mmenke@chromium.org changed reviewers: + eroman@chromium.org
Eric: What do you think about this? It has a huge search space that it will probably never completely cover, but it can [theoretically] cover cases that I'm not sure more granular fuzzers could catch. Just consider that it covers 3 compression algorithms (Not SDCH, though), the in-memory cache (fairly limited coverage there, since it only sends one request - could do a cached redirect loop, I suppose, but no cached response bodies), cookie parsing, theoretically covers cookie sending, too, though would need to support SSL mock sockets to fully test that, some coverage of socket reuse logic, etc. Each fuzzer iteration is fairly slow (400 runs per sec), since we create a TestURLRequestContext each run. We can't really keep it around between tests - we'd have to flush the cache and the socket pools, for instance, and I'd be very concerned about missing stuff if we even tried.
https://codereview.chromium.org/1917503002/diff/40001/net/data/http/http.dict File net/data/http/http.dict (right): https://codereview.chromium.org/1917503002/diff/40001/net/data/http/http.dict... net/data/http/http.dict:54: "HTTP/1.1 302 Found\x0ALocation: http://lost/\x0A\x0A" Strangely, I have yet to see it generate a test case with a redirect.
Description was changed from ========== URLRequest fuzzer. This fuzzer covers much of the network stack, when all wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to cover everything, so most modules it covers should also be fuzzed independently. BUG=602440 ========== to ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to cover everything, so most modules it covers should also be fuzzed independently. BUG=602440 ==========
Description was changed from ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to cover everything, so most modules it covers should also be fuzzed independently. BUG=602440 ========== to ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to cover everything, so most modules it should also be fuzzed independently. BUG=602440 ==========
Description was changed from ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to cover everything, so most modules it should also be fuzzed independently. BUG=602440 ========== to ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to catch everything, so most modules it should also be fuzzed independently. BUG=602440 ==========
Cool, I think these are useful abstractions and test. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:34: if (remaining_data_.length() > 0) { nit: !x.empty() https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:35: unused_bits_ = remaining_data_.data()[remaining_data_.length() - 1]; Why does ConsumeBits() and ConsumeBytes() pull from different ends of the array? I would have envisioned layering this on top of the ConsumeBytes() primitive that pulls from the front: unused_bits_ = 0; num_unused_bits = 8 * ConsumeBytes(&unused_bits, 1); (Or based on current logic, just num_unused_bits = 8) https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:42: num_bits <= num_unused_bits_ ? num_bits : num_unused_bits_; Or how about: bits_to_consume = std::min(num_bits, num_unused_bits); https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:44: out = (out << bits_to_consume) + new_bits; Use | instead of +. It will be equivalent in this case, but since these are bit-level operations I find + confusing. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:15: // Utiliy class to break up fuzzer input for multiple consumers. Whenever run tyo: Utility https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:20: FuzzedDataProvider(const uint8_t* data, size_t size); Please describe ownership. (i.e. |data| must outlive |this|). https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:26: size_t ConsumeBytes(char* dest, size_t bytes); nit: input is uint8_t but here the output is char*. Should this be either uint8_t* for consistency, or void* for generality? https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:31: // calls), since the fuzzer generates data by manipulations at the byte level. Please define the range of num_bits. i.e. is it expected to be <= 32 (largest to fully return result), or is > 32 bits allowed. In the current implementation more than 32-bits can be consumed, however the result is truncated to just 32-bits, obviously. https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:41: // Use last byte to determine if the HttpProxyClientSocket should be told the Generalize this comment, or remove it. Where the data came from is an abstraction of the FuzzedDataProvider layer. https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:43: bool is_https_proxy = !(data_provider.ConsumeBits(8) & 1); Why not "!data_provider.ConsumeBits(1)" ? Are you concerned with changing the current corpus data? https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:37: remote_address_(IPEndPoint(IPAddress(127, 0, 0, 1), 80)), Or alternately IPAddress::IPv4Localhost() https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:57: sync = !!data_provider_->ConsumeBits(1); How about adding a wrapper method: bool FuzzedDatazProvider::ConsumeBool() { return !!ConsumeBits(1); } Since I could see this being useful in a number of places. I guess the downside is it makes it a bit less obvious how many bits are being used. i.e. it is less obvious why the following ConsumeBits(7) is awesome. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:58: result = data_provider_->ConsumeBits(7); Could combine this with the next line: result = std::min(buf_len, data_provider_->ConsumeBits(7)); although perhaps less clear given the style used here. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:106: sync = !!data_provider_->ConsumeBits(1); Same suggestiong (ConsumeBool()) https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:149: int sync = true; Why not a bool? https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:157: // Remove 6 bits, so a full byte as been consumed. If the connect is going as been --> has been. Also note I don't think it is worth documenting too much about the details on that a "full byte" was consumed. With the new design where the data provider saves the unused bits, at most the fuzz test wastes 7 bits, even when doing lots of small bit reads. So meh, consumers can just read how much data they want and call it a day :) https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:159: uint32_t error_bits = data_provider_->ConsumeBits(6); Why not just Consume these 6 bits when |is_error| ? https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:161: result = kConnectErrors[error_bits % arraysize(kConnectErrors)]; Note: would have been sufficient to consume 3 bits (since there are just 8 connect errors). Might be interesting to have a helper for that: // returns a number in range [0, max), consuming ceil(log_2 max) bits. FuzzedDataProvider::ConsumeNumberInRange(size_t max); https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:248: return kReadWriteErrors[data_provider_->ConsumeBits(8) % Ditto here -- 8 bits is more than we need. Not a problem per se, but does mean we are throwing away bits from the fuzzed data which could slow it down (although I am sure by a negligable amount). Could consider helper ConsumeNumberInRange() here too. (The corpus already won't be completely stable to changes made to kReadWriteErrors). https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket_factory.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:27: FailingUDPClientSocket() {} This looks boring. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:90: class FailingSSLClientSocket : public SSLClientSocket { Also bored. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:210: return make_scoped_ptr(new FailingUDPClientSocket()); The new hotness is WrapUnique(). The documentation doesn't explicitly say make_scoped_ptr is unique, but should probably use the new thing. and who knows, maybe by 2020 we will be able to use std::make_unique<>... https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:223: return std::move(socket); Remove the std::move(). Should be sufficient to just "return socket" because of RVO (which generally enables better optimizations). https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:231: return make_scoped_ptr(new FailingSSLClientSocket()); ditto, WrapUnique() https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket_factory.h (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.h:19: // sockets much bet the same, and in the same order (both on each socket, and much bet, aye? :) https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.h:24: // TODO(mmenke): Add support for both types of socket. Also TODO: add fuzzing for generation of valid cryptographically signed messages :). But yeah, i hear you. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.h:27: explicit FuzzedSocketFactory(FuzzedDataProvider* data_provider); mention ownership. I assume non-owned when we don't use std::unique_ptr<>, but still. https://codereview.chromium.org/1917503002/diff/60001/net/socket/socks_client... File net/socket/socks_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/socks_client... net/socket/socks_client_socket_fuzzer.cc:39: switch (data_provider.ConsumeBits(7) % 3) { Shrug, I would use the minimal amount of bits here. i.e. FuzzedDataProvider::GetNumberInRange() https://codereview.chromium.org/1917503002/diff/60001/net/url_request/url_req... File net/url_request/url_request_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/url_request/url_req... net/url_request/url_request_fuzzer.cc:32: url_request_context.CreateRequest(GURL("http://foo/"), neato! Some suggestions for the future: * Fuzz the protocol -- http, ftp, https * Fuzz the proxy configuration -- direct, proxy * Include upload data * Fuzz cancellation (especially within the Delegate in response to redirects and the like). * Fuzz deferring redirects and the like.
https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:43: bool is_https_proxy = !(data_provider.ConsumeBits(8) & 1); On 2016/04/22 22:07:10, eroman wrote: > Why not "!data_provider.ConsumeBits(1)" ? > > Are you concerned with changing the current corpus data? My concern is stability - the fuzzer works by modifying previous interesting inputs. If we remove things in multiples of 8, behavior tends to be more stable when the input is modified. Yes, the data may be used for completely different things, but in the case of multiple reads in a row, for example, the data is interpreted the exact same way. If we don't read in multiples of 8, the results of subsequent operations are basically completely random.
Responses to some comments I disagree with, I'll respond to the others and upload another CL later today. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:35: unused_bits_ = remaining_data_.data()[remaining_data_.length() - 1]; On 2016/04/22 22:07:10, eroman wrote: > Why does ConsumeBits() and ConsumeBytes() pull from different ends of the array? > I would have envisioned layering this on top of the ConsumeBytes() primitive > that pulls from the front: > > unused_bits_ = 0; > num_unused_bits = 8 * ConsumeBytes(&unused_bits, 1); > > (Or based on current logic, just num_unused_bits = 8) For consistency. Remember that the fuzzer takes previous inputs, and inserts data into them. If we have HTTP/1.1 200 OK Yada: Yada YaddaYadda: Yada Yada Yada Yada YadaYadaYadaYada <metadata> It becomes much easier for the fuzzer to much with the HTTP response while maintain the full response, than if we have: <read 8-bytes code>HTTP/1.1 <read 15-bytes code> 200 OK Yada: Y<read 13-bytes code>ada ... In the first case, all it needs to do is find a place to add a "SetCookies" header, for instance, and it's done (Admittedly, the request may start ending in a read error, or lose a byte or two off the end). In the second case, it has to add SetCookies in the right place, and add an exactly matching read length code as well. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:26: size_t ConsumeBytes(char* dest, size_t bytes); On 2016/04/22 22:07:10, eroman wrote: > nit: input is uint8_t but here the output is char*. Should this be either > uint8_t* for consistency, or void* for generality? Very few places in net use uint8_t - IOBuffer, in particular, uses char*. void* means we lose enforcement of type correctness. I think char* is the way to go here. If we decide to change IOBuffer, we can change this as well. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:159: uint32_t error_bits = data_provider_->ConsumeBits(6); On 2016/04/22 22:07:11, eroman wrote: > Why not just Consume these 6 bits when |is_error| ? See my other comments on how the fuzzer traverses input, above. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket_factory.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:27: FailingUDPClientSocket() {} On 2016/04/22 22:07:11, eroman wrote: > This looks boring. It was, very. https://codereview.chromium.org/1917503002/diff/60001/net/url_request/url_req... File net/url_request/url_request_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/url_request/url_req... net/url_request/url_request_fuzzer.cc:32: url_request_context.CreateRequest(GURL("http://foo/"), On 2016/04/22 22:07:11, eroman wrote: > neato! > > Some suggestions for the future: > * Fuzz the protocol -- http, ftp, https > * Fuzz the proxy configuration -- direct, proxy > * Include upload data > * Fuzz cancellation (especially within the Delegate in response to redirects > and the like). > * Fuzz deferring redirects and the like. Yea, I agree with all of these.
https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:35: unused_bits_ = remaining_data_.data()[remaining_data_.length() - 1]; On 2016/04/25 15:09:18, mmenke wrote: > On 2016/04/22 22:07:10, eroman wrote: > > Why does ConsumeBits() and ConsumeBytes() pull from different ends of the > array? > > I would have envisioned layering this on top of the ConsumeBytes() primitive > > that pulls from the front: > > > > unused_bits_ = 0; > > num_unused_bits = 8 * ConsumeBytes(&unused_bits, 1); > > > > (Or based on current logic, just num_unused_bits = 8) > > For consistency. Remember that the fuzzer takes previous inputs, and inserts > data into them. > > If we have > > HTTP/1.1 200 OK > Yada: Yada > YaddaYadda: Yada > > Yada Yada Yada > YadaYadaYadaYada > <metadata> > > It becomes much easier for the fuzzer to much with the HTTP response while > maintain the full response, than if we have: > > <read 8-bytes code>HTTP/1.1 <read 15-bytes code> 200 OK > Yada: Y<read 13-bytes code>ada > ... > > In the first case, all it needs to do is find a place to add a "SetCookies" > header, for instance, and it's done (Admittedly, the request may start ending in > a read error, or lose a byte or two off the end). In the second case, it has to > add SetCookies in the right place, and add an exactly matching read length code > as well. OK, if you have run tests and believe this to be a better formulation for input discovery, then great.
On 2016/04/25 16:07:12, eroman wrote: > https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... > File net/base/fuzzed_data_provider.cc (right): > > https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... > net/base/fuzzed_data_provider.cc:35: unused_bits_ = > remaining_data_.data()[remaining_data_.length() - 1]; > On 2016/04/25 15:09:18, mmenke wrote: > > On 2016/04/22 22:07:10, eroman wrote: > > > Why does ConsumeBits() and ConsumeBytes() pull from different ends of the > > array? > > > I would have envisioned layering this on top of the ConsumeBytes() primitive > > > that pulls from the front: > > > > > > unused_bits_ = 0; > > > num_unused_bits = 8 * ConsumeBytes(&unused_bits, 1); > > > > > > (Or based on current logic, just num_unused_bits = 8) > > > > For consistency. Remember that the fuzzer takes previous inputs, and inserts > > data into them. > > > > If we have > > > > HTTP/1.1 200 OK > > Yada: Yada > > YaddaYadda: Yada > > > > Yada Yada Yada > > YadaYadaYadaYada > > <metadata> > > > > It becomes much easier for the fuzzer to much with the HTTP response while > > maintain the full response, than if we have: > > > > <read 8-bytes code>HTTP/1.1 <read 15-bytes code> 200 OK > > Yada: Y<read 13-bytes code>ada > > ... > > > > In the first case, all it needs to do is find a place to add a "SetCookies" > > header, for instance, and it's done (Admittedly, the request may start ending > in > > a read error, or lose a byte or two off the end). In the second case, it has > to > > add SetCookies in the right place, and add an exactly matching read length > code > > as well. > > OK, if you have run tests and believe this to be a better formulation for input > discovery, then great. I haven't actually directly compared the two methods - I'll experiment with it a bit this afternoon.
Hrm...I just discovered that these tests are not running deterministically. Some non-determinism is kinda expected here, since disabling all the timeout timers and such is a bit excessive, but it's 100% reproducible. Every time I restart, using a previously generated corpus, it starts out with lower coverage than it had when it generated the corpus.
On 2016/04/25 19:41:07, mmenke wrote: > Hrm...I just discovered that these tests are not running deterministically. > > Some non-determinism is kinda expected here, since disabling all the timeout > timers and such is a bit excessive, but it's 100% reproducible. Every time I > restart, using a previously generated corpus, it starts out with lower coverage > than it had when it generated the corpus. So, measuring non-determinism as the "cov" and "bits" values of where one test run ends and the next starts, removing the shared MessageLoop gets rid of most of the discrepancy, but not quite all of it. Going to continue and investigate, and see if I can figure out if we're racing timers, file I/O, threads, or what.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
https://codereview.chromium.org/1917503002/diff/80001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/80001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:41: num_bits -= bits_to_add; Drive-by: I think bits_to_add can be greater than num_bits here, which will cause underflow. Maybe add DCHECK(0 <= out <= 1 << num_bits).
After some digging, it looks like the fuzzer always considers the 101st, 1001st, and 1024th iterations interesting. If I run for a number of iterations to create a corpus, and then just run on that corpus, the coverage difference between the two test runs (With no shared message loop) are all in STL containers: 3 different hit lines in vector, one in some map-ish class, and I've forgotten the other. My thinking is there's just some global that resizes on those iterations. It doesn't seem to happen on the other fuzzers I tried. Could be UMA's globals, or something similar. Don't think it's worth further investigation.
Patchset #6 (id:100001) has been deleted
Patchset #7 (id:140001) has been deleted
Eric: PTAL. I've switched to always consuming bytes in multiples of 8 for each call - don't think bits are so expensive that we need to avoid wasting them, and think this may give better behavior under input mutation. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:34: if (remaining_data_.length() > 0) { On 2016/04/22 22:07:10, eroman wrote: > nit: !x.empty() Done. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:35: unused_bits_ = remaining_data_.data()[remaining_data_.length() - 1]; On 2016/04/25 16:07:12, eroman wrote: > On 2016/04/25 15:09:18, mmenke wrote: > > On 2016/04/22 22:07:10, eroman wrote: > > > Why does ConsumeBits() and ConsumeBytes() pull from different ends of the > > array? > > > I would have envisioned layering this on top of the ConsumeBytes() primitive > > > that pulls from the front: > > > > > > unused_bits_ = 0; > > > num_unused_bits = 8 * ConsumeBytes(&unused_bits, 1); > > > > > > (Or based on current logic, just num_unused_bits = 8) > > > > For consistency. Remember that the fuzzer takes previous inputs, and inserts > > data into them. > > > > If we have > > > > HTTP/1.1 200 OK > > Yada: Yada > > YaddaYadda: Yada > > > > Yada Yada Yada > > YadaYadaYadaYada > > <metadata> > > > > It becomes much easier for the fuzzer to much with the HTTP response while > > maintain the full response, than if we have: > > > > <read 8-bytes code>HTTP/1.1 <read 15-bytes code> 200 OK > > Yada: Y<read 13-bytes code>ada > > ... > > > > In the first case, all it needs to do is find a place to add a "SetCookies" > > header, for instance, and it's done (Admittedly, the request may start ending > in > > a read error, or lose a byte or two off the end). In the second case, it has > to > > add SetCookies in the right place, and add an exactly matching read length > code > > as well. > > OK, if you have run tests and believe this to be a better formulation for input > discovery, then great. I have now run tests. I ran each method twice with the URLRequest fuzzer, once for 20,000 iterations, once for 200,000 iterations. Admittedly, doesn't really compare to what the fuzzer servers do. Both times getting the metadata from the end did better (~380 vs ~360 on the short run, ~1200 vs ~800 on the longer one). No doubt within the margin of error, for such a small sample set, but since the logic seems to support this approach, at least for how I'm using the fuzzers, I think this is good enough. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:42: num_bits <= num_unused_bits_ ? num_bits : num_unused_bits_; On 2016/04/22 22:07:10, eroman wrote: > Or how about: > bits_to_consume = std::min(num_bits, num_unused_bits); Now unused is a hard-coded 8, and "std::min(num_bits, 8u);" fails to compile, so I'm keeping it written out. static_cast<size_t>(8u) is just weird. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:44: out = (out << bits_to_consume) + new_bits; On 2016/04/22 22:07:10, eroman wrote: > Use | instead of +. It will be equivalent in this case, but since these are > bit-level operations I find + confusing. Done. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:15: // Utiliy class to break up fuzzer input for multiple consumers. Whenever run On 2016/04/22 22:07:10, eroman wrote: > tyo: Utility Done. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:20: FuzzedDataProvider(const uint8_t* data, size_t size); On 2016/04/22 22:07:10, eroman wrote: > Please describe ownership. (i.e. |data| must outlive |this|). Done. https://codereview.chromium.org/1917503002/diff/60001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.h:31: // calls), since the fuzzer generates data by manipulations at the byte level. On 2016/04/22 22:07:10, eroman wrote: > Please define the range of num_bits. > > i.e. is it expected to be <= 32 (largest to fully return result), or is > 32 > bits allowed. > > In the current implementation more than 32-bits can be consumed, however the > result is truncated to just 32-bits, obviously. Done. Added a CHECK, and documented it. https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:41: // Use last byte to determine if the HttpProxyClientSocket should be told the On 2016/04/22 22:07:10, eroman wrote: > Generalize this comment, or remove it. Where the data came from is an > abstraction of the FuzzedDataProvider layer. Done. https://codereview.chromium.org/1917503002/diff/60001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:43: bool is_https_proxy = !(data_provider.ConsumeBits(8) & 1); On 2016/04/22 22:59:32, mmenke wrote: > On 2016/04/22 22:07:10, eroman wrote: > > Why not "!data_provider.ConsumeBits(1)" ? > > > > Are you concerned with changing the current corpus data? > > My concern is stability - the fuzzer works by modifying previous interesting > inputs. If we remove things in multiples of 8, behavior tends to be more stable > when the input is modified. Yes, the data may be used for completely different > things, but in the case of multiple reads in a row, for example, the data is > interpreted the exact same way. If we don't read in multiples of 8, the results > of subsequent operations are basically completely random. I've updated ConsumeBits to always consume an exact number of bytes. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:37: remote_address_(IPEndPoint(IPAddress(127, 0, 0, 1), 80)), On 2016/04/22 22:07:10, eroman wrote: > Or alternately IPAddress::IPv4Localhost() Handy. Done. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:57: sync = !!data_provider_->ConsumeBits(1); On 2016/04/22 22:07:11, eroman wrote: > How about adding a wrapper method: > > bool FuzzedDatazProvider::ConsumeBool() { > return !!ConsumeBits(1); > } > > Since I could see this being useful in a number of places. > > I guess the downside is it makes it a bit less obvious how many bits are being > used. > i.e. it is less obvious why the following ConsumeBits(7) is awesome. Hrm...We could make consume bits always round to the nearest multiple of 8 bits, instead of trying to minimize number of consumed bits. WDYT? We could waste more data, but I don't think that's a huge deal. I've switched to doing that. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket.cc:58: result = data_provider_->ConsumeBits(7); On 2016/04/22 22:07:11, eroman wrote: > Could combine this with the next line: > > result = std::min(buf_len, data_provider_->ConsumeBits(7)); > > although perhaps less clear given the style used here. You mean std::min(static_cast<uint32_t>(buf_len), data_provider_->ConsumeBits(7)); Which is why I didn't do it - casts, and templates in general, are just ugly, though this does, admittedly, do the same cast implicitly. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket_factory.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:210: return make_scoped_ptr(new FailingUDPClientSocket()); On 2016/04/22 22:07:11, eroman wrote: > The new hotness is WrapUnique(). > > The documentation doesn't explicitly say make_scoped_ptr is unique, but should > probably use the new thing. > > and who knows, maybe by 2020 we will be able to use std::make_unique<>... Done. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:223: return std::move(socket); On 2016/04/22 22:07:11, eroman wrote: > Remove the std::move(). > > Should be sufficient to just "return socket" because of RVO (which generally > enables better optimizations). That doesn't work - their types are different (std::unique_ptr<FuzzedSocket> vs std::unique_ptr<StreamSocket>). Fails to build without the move. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.cc:231: return make_scoped_ptr(new FailingSSLClientSocket()); On 2016/04/22 22:07:11, eroman wrote: > ditto, WrapUnique() Done. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... File net/socket/fuzzed_socket_factory.h (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.h:19: // sockets much bet the same, and in the same order (both on each socket, and On 2016/04/22 22:07:11, eroman wrote: > much bet, aye? > :) Yes, yes. Much bet, very much bet. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.h:24: // TODO(mmenke): Add support for both types of socket. On 2016/04/22 22:07:11, eroman wrote: > Also TODO: add fuzzing for generation of valid cryptographically signed messages > :). But yeah, i hear you. Done. https://codereview.chromium.org/1917503002/diff/60001/net/socket/fuzzed_socke... net/socket/fuzzed_socket_factory.h:27: explicit FuzzedSocketFactory(FuzzedDataProvider* data_provider); On 2016/04/22 22:07:11, eroman wrote: > mention ownership. > > I assume non-owned when we don't use std::unique_ptr<>, but still. Done. https://codereview.chromium.org/1917503002/diff/60001/net/socket/socks_client... File net/socket/socks_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/60001/net/socket/socks_client... net/socket/socks_client_socket_fuzzer.cc:39: switch (data_provider.ConsumeBits(7) % 3) { On 2016/04/22 22:07:11, eroman wrote: > Shrug, I would use the minimal amount of bits here. i.e. > FuzzedDataProvider::GetNumberInRange() Done. https://codereview.chromium.org/1917503002/diff/80001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/80001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:41: num_bits -= bits_to_add; Oops, yea, you're right, good catch! I'd already fixed this in a refactor (Didn't find the bug, but noticed things were working), but great to know where the bug was.
https://codereview.chromium.org/1917503002/diff/160001/net/data/http/http.dict File net/data/http/http.dict (right): https://codereview.chromium.org/1917503002/diff/160001/net/data/http/http.dic... net/data/http/http.dict:96: "\x0AAge: 3153600000" This is 100 years. Figure that reduces variations in test behavior due to values indicating expiration one time, and not another, due to either test raciness or the system clock. https://codereview.chromium.org/1917503002/diff/160001/net/data/http/http.dic... net/data/http/http.dict:107: "\x0ACache-Control: must-revalidate" None of my runs remotely compares to what the test server does, but I found including the full strings makes things converge much faster, at least.
lgtm https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.cc:22: memcpy(dest, remaining_data_.data(), bytes_to_write); side-rant: unfortunately because the definition of memcpy() is complete rubbist, this can lead to undefined behavior if ever remaining_data_ was empty (in such a way that remaining_data_.data() is NULL. Because naturally, memcpy(X, NULL, 0) is undefined behavior. Which is a completely insane of course, but yay C? (for instance see https://gcc.gnu.org/gcc-4.9/porting_to.html and the inferences it makes from memmove as an example) I hate memcpy() anyway -- probably just make ConsumeBytes() return a string piece and let the consumer worry about copying? https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.cc:34: while ((range >> offset) > 0 && !remaining_data_.empty()) { much simpler than earlier impl! https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.cc:48: if (min == 0 && max == std::numeric_limits<uint32_t>::max()) nit: sufficient to test range == std::numeric_limits<uint32_t>::max() A little bit clearer anyway, since the division by 0 comes from overflow on (range + 1). https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.cc:64: return !ConsumeBits(1); So before in some places you were using !!ConsumeBits(1). Conceptually doesn't matter, except for what you want the default to be in the case where there is no data left (default true with this impl). https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:28: size_t ConsumeBytes(char* dest, size_t bytes); I think this interface would be easier/safer if it returned a base::StringPiece rather than copying to |dest|. We can get away with that since the current implementation just returns slices into the underlying data, which has to remain alive. For instance stuff that needs to make a copy could become: std::string data_read = foo->ConsumeBytes(10).as_string(); (Or use the helpers that append base::StringPiece to std::strings) Now stuff that just needs to pass around data can feed it without making a copy. base::StringPiece data_read = foo->ConsumeBytes(10); TestMyFoo(data_read); // didn't need to make a copy. Conceptually csharrison's fuzzer could have consumed this base::StringPiece interface (except in practice too many of our helpers takes const std::string& when they could just as easily be taking base::StringPiece, so would have probably had to copy to a std::string anyway). Even so, I think this is a bit less fiddly (code now needs to be careful that |dest| is at least |bytes| long). WDYT? https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:41: // Same as ConsumeBits(1), but returns a bool. Might want to mention returns |true| when there is no more data. https://codereview.chromium.org/1917503002/diff/180001/net/socket/fuzzed_sock... File net/socket/fuzzed_socket.cc (right): https://codereview.chromium.org/1917503002/diff/180001/net/socket/fuzzed_sock... net/socket/fuzzed_socket.cc:37: remote_address_(IPEndPoint(IPAddress(127, 0, 0, 1), 80)), I thought you were changing this to IPAddress::IPv4Localhost() ? Either approach is fine, just mentioning in case you didn't upload a patchset. https://codereview.chromium.org/1917503002/diff/180001/net/url_request/url_re... File net/url_request/url_request_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/180001/net/url_request/url_re... net/url_request/url_request_fuzzer.cc:41: base::RunLoop().Run(); Could you add a comment explaining that TestDelegate quits the loop on completion?
Thanks for all the feedback! https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.cc:22: memcpy(dest, remaining_data_.data(), bytes_to_write); On 2016/04/28 01:32:35, eroman wrote: > side-rant: unfortunately because the definition of memcpy() is complete rubbist, > this can lead to undefined behavior if ever remaining_data_ was empty (in such a > way that remaining_data_.data() is NULL. > > Because naturally, memcpy(X, NULL, 0) is undefined behavior. Which is a > completely insane of course, but yay C? (for instance see > https://gcc.gnu.org/gcc-4.9/porting_to.html and the inferences it makes from > memmove as an example) > > I hate memcpy() anyway -- probably just make ConsumeBytes() return a string > piece and let the consumer worry about copying? remaining_data_.data() cannot be NULL, I believe. It can only be of length 0. Anyhow, I've swtiched to returning a StringPiece, and now use std::copy in FuzzedSocket https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.cc:48: if (min == 0 && max == std::numeric_limits<uint32_t>::max()) On 2016/04/28 01:32:36, eroman wrote: > nit: sufficient to test range == std::numeric_limits<uint32_t>::max() > > A little bit clearer anyway, since the division by 0 comes from overflow on > (range + 1). Done. https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.cc:64: return !ConsumeBits(1); On 2016/04/28 01:32:36, eroman wrote: > So before in some places you were using !!ConsumeBits(1). > > Conceptually doesn't matter, except for what you want the default to be in the > case where there is no data left (default true with this impl). Ahh, good point. Changed, and added comment. https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:28: size_t ConsumeBytes(char* dest, size_t bytes); On 2016/04/28 01:32:36, eroman wrote: > I think this interface would be easier/safer if it returned a base::StringPiece > rather than copying to |dest|. > > We can get away with that since the current implementation just returns slices > into the underlying data, which has to remain alive. > > For instance stuff that needs to make a copy could become: > > std::string data_read = foo->ConsumeBytes(10).as_string(); > > (Or use the helpers that append base::StringPiece to std::strings) > > Now stuff that just needs to pass around data can feed it without making a copy. > > base::StringPiece data_read = foo->ConsumeBytes(10); > TestMyFoo(data_read); // didn't need to make a copy. > > Conceptually csharrison's fuzzer could have consumed this base::StringPiece > interface (except in practice too many of our helpers takes const std::string& > when they could just as easily be taking base::StringPiece, so would have > probably had to copy to a std::string anyway). > > Even so, I think this is a bit less fiddly (code now needs to be careful that > |dest| is at least |bytes| long). > > WDYT? SGTM. Could even provide both interfaces, but I'll stick with just the StringPiece version for now. https://codereview.chromium.org/1917503002/diff/180001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:41: // Same as ConsumeBits(1), but returns a bool. On 2016/04/28 01:32:36, eroman wrote: > Might want to mention returns |true| when there is no more data. Done (Though now it's false). https://codereview.chromium.org/1917503002/diff/180001/net/socket/fuzzed_sock... File net/socket/fuzzed_socket.cc (right): https://codereview.chromium.org/1917503002/diff/180001/net/socket/fuzzed_sock... net/socket/fuzzed_socket.cc:37: remote_address_(IPEndPoint(IPAddress(127, 0, 0, 1), 80)), On 2016/04/28 01:32:36, eroman wrote: > I thought you were changing this to IPAddress::IPv4Localhost() ? > Either approach is fine, just mentioning in case you didn't upload a patchset. Done. I did it earlier, but I reverted a bunch of files when debugging, and then forgot to un-revert some of them. :( https://codereview.chromium.org/1917503002/diff/180001/net/url_request/url_re... File net/url_request/url_request_fuzzer.cc (right): https://codereview.chromium.org/1917503002/diff/180001/net/url_request/url_re... net/url_request/url_request_fuzzer.cc:41: base::RunLoop().Run(); On 2016/04/28 01:32:36, eroman wrote: > Could you add a comment explaining that TestDelegate quits the loop on > completion? Done.
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/1917503002/#ps220001 (title: "Minor cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917503002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917503002/220001
[+mmoroz]: Hadn't realized you guys wanted to be CCed on all fuzzers - makes sense, though. This integration-y fuzzer covers much of the network stack. We want to cover the layers individually, and there are plenty of things this can't do, but this at least gives us high level coverage of a lot of code, and can catch some things other fuzzers may not be able to catch (socket reuse, for instance)
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/1917503002/#ps240001 (title: "Remove unnecessary const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917503002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917503002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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/1917503002/#ps260001 (title: "Add missing include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917503002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917503002/260001
https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:29: base::StringPiece ConsumeBytes(size_t bytes); FYI: In some follow-up changes I am reviewing, another useful API that has emerged is something along the lines of: base::StringPiece ConsumeRemainingBytes() size_t num_remaining_bytes(); For instance I suggested this in https://codereview.chromium.org/1929703003/#msg5, and I know Charles was thinking similarly in https://codereview.chromium.org/1919013003/diff/80001/net/base/fuzzed_data_pr...
https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:29: base::StringPiece ConsumeBytes(size_t bytes); On 2016/04/28 18:32:33, eroman wrote: > FYI: In some follow-up changes I am reviewing, another useful API that has > emerged is something along the lines of: > > base::StringPiece ConsumeRemainingBytes() > size_t num_remaining_bytes(); > > For instance I suggested this in > https://codereview.chromium.org/1929703003/#msg5, and I know Charles was > thinking similarly in > https://codereview.chromium.org/1919013003/diff/80001/net/base/fuzzed_data_pr... Yeah I was actually planning on adding this method now that this class dispenses StringPieces.
https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:29: base::StringPiece ConsumeBytes(size_t bytes); On 2016/04/28 18:32:33, eroman wrote: > FYI: In some follow-up changes I am reviewing, another useful API that has > emerged is something along the lines of: > > base::StringPiece ConsumeRemainingBytes() > size_t num_remaining_bytes(); > > For instance I suggested this in > https://codereview.chromium.org/1929703003/#msg5, and I know Charles was > thinking similarly in > https://codereview.chromium.org/1919013003/diff/80001/net/base/fuzzed_data_pr... I was thinking I'd probably need something like that at some point, too. Another thing that I was thinking may be useful is: base::StringPiece ConsumeLine(); Which just consumes bytes up to the next LF, for fuzzing things that don't expect to contain LFs that can be of arbitrary length (URLs, maybe?). Less finicky than separating out length and data into separate fields, too. Or could make it contain a character / list of characters to stop at, like strtok.
https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... File net/base/fuzzed_data_provider.h (right): https://codereview.chromium.org/1917503002/diff/260001/net/base/fuzzed_data_p... net/base/fuzzed_data_provider.h:29: base::StringPiece ConsumeBytes(size_t bytes); On 2016/04/28 18:38:29, mmenke wrote: > On 2016/04/28 18:32:33, eroman wrote: > > FYI: In some follow-up changes I am reviewing, another useful API that has > > emerged is something along the lines of: > > > > base::StringPiece ConsumeRemainingBytes() > > size_t num_remaining_bytes(); > > > > For instance I suggested this in > > https://codereview.chromium.org/1929703003/#msg5, and I know Charles was > > thinking similarly in > > > https://codereview.chromium.org/1919013003/diff/80001/net/base/fuzzed_data_pr... > > I was thinking I'd probably need something like that at some point, too. > > Another thing that I was thinking may be useful is: > > base::StringPiece ConsumeLine(); > > Which just consumes bytes up to the next LF, for fuzzing things that don't > expect to contain LFs that can be of arbitrary length (URLs, maybe?). Less > finicky than separating out length and data into separate fields, too. > > Or could make it contain a character / list of characters to stop at, like > strtok. sgtm
Message was sent while issue was closed.
Description was changed from ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to catch everything, so most modules it should also be fuzzed independently. BUG=602440 ========== to ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to catch everything, so most modules it should also be fuzzed independently. BUG=602440 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c951d41c1a0df97a0b6a17379f8f7c11989aa80f Cr-Commit-Position: refs/heads/master@{#390440}
Message was sent while issue was closed.
Description was changed from ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to catch everything, so most modules it should also be fuzzed independently. BUG=602440 ========== to ========== URLRequest fuzzer. This fuzzer covers much of the network stack, with everything wired up. Unfortunately, it does have a couple dependencies on the system it's running on (uses NTLM/Neogiate platform stores and current time, not sure what else). It covers so much code that it's unlikely to be able to catch everything, so most modules it should also be fuzzed independently. BUG=602440 Committed: https://crrev.com/c951d41c1a0df97a0b6a17379f8f7c11989aa80f Cr-Commit-Position: refs/heads/master@{#390440} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
