Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919013003:
Add fuzzer to test Fuzz URLRequestDataJob (Closed)
|
Created:
4 years, 8 months ago by Charlie Harrison Modified:
4 years, 7 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, mmenke, Oliver Chang Base URL:
https://chromium.googlesource.com/chromium/src.git@url_request_fuzzer Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd fuzzer to test Fuzz URLRequestDataJob
BUG=602460
Committed: https://crrev.com/aa314dc5eead1a69177d91b70018b93535dbfe20
Cr-Commit-Position: refs/heads/master@{#390734}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Encapsulated logic in a singleton class. Rebased / updated for FuzzedDataProvider. #
Total comments: 6
Patch Set 3 : self review #
Total comments: 11
Patch Set 4 : Use URLRequest::Delegate #
Total comments: 12
Patch Set 5 : use vector<char> instead of constant size array #Patch Set 6 : Rebased on dependent patch and simplified #Patch Set 7 : another rebase #Patch Set 8 : rebase on #390645 #
Total comments: 1
Patch Set 9 : remove NET_EXPORT_PRIVATE #Patch Set 10 #
Messages
Total messages: 26 (9 generated)
csharrison@chromium.org changed reviewers: + eroman@chromium.org
eroman@, PTAL? Can you confirm that the static initialization trick is okay here? Without them the tests run ~3-4x slower. cc mmenke@ who I assume will be swamped with these reviews this week.
https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:20: std::string GenerateRandomRange(const char* bytes, size_t size) { Please document. This isn't necessarily generating valid range headers right? In that case, why generate the header here rather than have it discovered by the fuzzer? https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:35: // Fuzz different read lengths, different range requests, and different data. Please add a more general description of what the test does. Along the lines maybe of: Tests creating and reading to completion a URLRequest with fuzzed data: URLs, and custom Range headers. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:45: static char range_bytes[max_range_length]; Rather than N different statics, please extract this to a Singleton. The test could be a method on the Singleton for ease of accessing all these member then. I suggest mainly for readability, but depending how compiler generates the code might be faster too. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:56: job_factory.SetProtocolHandler( This for instance would just be part of the singleton's constructor. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:88: bytes_for_data_url -= 1; This seems like something that could be encapsulated into FuzzedDataProvider instead. i.e. would something like: ConsumeRemainingBytes() work? https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:97: request->SetExtraRequestHeaderByName("Range", range, true); Might want "no range header" to be a possible input as well. Not sure if invalid range headers are already treated in that manner.
I've updated the test to use the Singleton pattern, and tried to simplify it a bit and add better comments. Thanks for the review! https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:20: std::string GenerateRandomRange(const char* bytes, size_t size) { On 2016/04/25 at 23:51:30, eroman wrote: > Please document. > > This isn't necessarily generating valid range headers right? In that case, why generate the header here rather than have it discovered by the fuzzer? The goal is to give some structure so the fuzzer doesn't have to learn about range requests. This means the fuzzer learns about the real bugs in the range request implementation that isn't just valid header parsing. I'm fine to remove this though if you think just fuzzing the string is good enough. In the meantime I've added some comments. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:35: // Fuzz different read lengths, different range requests, and different data. On 2016/04/25 at 23:51:31, eroman wrote: > Please add a more general description of what the test does. Along the lines maybe of: > > Tests creating and reading to completion a URLRequest with fuzzed data: URLs, and custom Range headers. Done. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:45: static char range_bytes[max_range_length]; On 2016/04/25 at 23:51:30, eroman wrote: > Rather than N different statics, please extract this to a Singleton. > > The test could be a method on the Singleton for ease of accessing all these member then. > > I suggest mainly for readability, but depending how compiler generates the code might be faster too. Done. Good suggestion. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:56: job_factory.SetProtocolHandler( On 2016/04/25 at 23:51:30, eroman wrote: > This for instance would just be part of the singleton's constructor. Done. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:88: bytes_for_data_url -= 1; On 2016/04/25 at 23:51:30, eroman wrote: > This seems like something that could be encapsulated into FuzzedDataProvider instead. > i.e. would something like: > > ConsumeRemainingBytes() work? We'll still need to keep track of this length to make sure our reads will fully cover the remaining bytes. Otherwise we wouldn't know when to stop looping. I've added a method to on the data provider to make this easier. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:97: request->SetExtraRequestHeaderByName("Range", range, true); On 2016/04/25 at 23:51:30, eroman wrote: > Might want "no range header" to be a possible input as well. Not sure if invalid range headers are already treated in that manner. Done. https://codereview.chromium.org/1919013003/diff/20001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1919013003/diff/20001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:26: size_t FuzzedDataProvider::ConsumeRemainingBytes(char* dest) { This is wrong, because we could buffer overflow dest. https://codereview.chromium.org/1919013003/diff/20001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:45: num_bits = num_bits > bits_to_add ? num_bits - bits_to_add : 0; Temporary fix. https://codereview.chromium.org/1919013003/diff/20001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/20001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:5: #include <algorithm> Remove <algorithm> https://codereview.chromium.org/1919013003/diff/20001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:22: // |range_vals| array. Update comment. https://codereview.chromium.org/1919013003/diff/20001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:57: // amount of data read in each Read call is also fuzzed. Include that the IOBuffer size is fuzzed, and we randomly do not append a range header. https://codereview.chromium.org/1919013003/diff/20001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:59: // |-- data url --|-- read metadata --|-- range header --| This line is unclear because we now fuzz IOBuffer size and whether to use a range header.
https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:20: std::string GenerateRandomRange(const char* bytes, size_t size) { On 2016/04/26 12:58:40, csharrison wrote: > On 2016/04/25 at 23:51:30, eroman wrote: > > Please document. > > > > This isn't necessarily generating valid range headers right? In that case, why > generate the header here rather than have it discovered by the fuzzer? > > The goal is to give some structure so the fuzzer doesn't have to learn about > range requests. This means the fuzzer learns about the real bugs in the range > request implementation that isn't just valid header parsing. > > I'm fine to remove this though if you think just fuzzing the string is good > enough. In the meantime I've added some comments. I prefer having the fuzzer build the range request. My thinking is, it may come up with invalid combinations that trigger problems that we didn't think to test here. For instance what if it has weird bugs when dealing with hex digits, certain whitespaces, decimal numbers, etc. Rather than hardcoding what we think is the grammar (or which may be everything today), can just let the fuzzer discover it. I am not too concerned with having a larger space to explore, so long as it has a chance of discovering bugs. You would be surprised what it comes up with after running long enough :) Note that we already have separate tests for data:URL parsing itself, so I think the main value-added of this test is really checking the URLRequest interface and range stuff that we do for data:URLs. To be honest, I didn't even expect that range headers should work with data URLs! https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:21: static std::vector<char> range_vals{'\0', '-', '0', '1', '2', '3', '4', unfortunately our style guide doesn't allow this yet. https://codereview.chromium.org/1919013003/diff/40001/net/base/fuzzed_data_pr... File net/base/fuzzed_data_provider.cc (right): https://codereview.chromium.org/1919013003/diff/40001/net/base/fuzzed_data_pr... net/base/fuzzed_data_provider.cc:41: num_bits = num_bits > bits_to_add ? num_bits - bits_to_add : 0; Hmm yeah, there does seem to be a bug in the dependent CL! Hadn't noticed that when reviewing it :) https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:63: scoped_refptr<net::IOBuffer> buf(new net::IOBuffer((size_t)buf_size)); static_cast<size_t>(buf_size). https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:78: size_t read_length = provider.ConsumeValueInRange(0, 32); I wonder if a read length of 0 make sense. Maybe use 1 as the minimum? https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:91: GURL("data:" + data_string), net::DEFAULT_PRIORITY, &delegate_); What happens in the case of invalid GURLs? i.e. GURL::is_valid() https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:97: base::RunLoop().RunUntilIdle(); This is making some assumptions about how the code runs. Can you instead exercise the generic URLRequest API and check the return value of Read() and wait for the read to complete? https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:103: read_lengths_.clear(); I would actually put this at the start of the function instead, or make it a local variable.
PTAL. This is a little more complex now that the class is a URLRequest::Delegate. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:20: std::string GenerateRandomRange(const char* bytes, size_t size) { On 2016/04/27 00:43:01, eroman wrote: > On 2016/04/26 12:58:40, csharrison wrote: > > On 2016/04/25 at 23:51:30, eroman wrote: > > > Please document. > > > > > > This isn't necessarily generating valid range headers right? In that case, > why > > generate the header here rather than have it discovered by the fuzzer? > > > > The goal is to give some structure so the fuzzer doesn't have to learn about > > range requests. This means the fuzzer learns about the real bugs in the range > > request implementation that isn't just valid header parsing. > > > > I'm fine to remove this though if you think just fuzzing the string is good > > enough. In the meantime I've added some comments. > > I prefer having the fuzzer build the range request. > > My thinking is, it may come up with invalid combinations that trigger problems > that we didn't think to test here. For instance what if it has weird bugs when > dealing with hex digits, certain whitespaces, decimal numbers, etc. Rather than > hardcoding what we think is the grammar (or which may be everything today), can > just let the fuzzer discover it. > > I am not too concerned with having a larger space to explore, so long as it has > a chance of discovering bugs. You would be surprised what it comes up with after > running long enough :) > > Note that we already have separate tests for data:URL parsing itself, so I think > the main value-added of this test is really checking the URLRequest interface > and range stuff that we do for data:URLs. > > To be honest, I didn't even expect that range headers should work with data > URLs! Sounds good to me. I've removed this code. https://codereview.chromium.org/1919013003/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:21: static std::vector<char> range_vals{'\0', '-', '0', '1', '2', '3', '4', On 2016/04/27 00:43:01, eroman wrote: > unfortunately our style guide doesn't allow this yet. Removed. https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:63: scoped_refptr<net::IOBuffer> buf(new net::IOBuffer((size_t)buf_size)); On 2016/04/27 00:43:01, eroman wrote: > static_cast<size_t>(buf_size). Done. https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:78: size_t read_length = provider.ConsumeValueInRange(0, 32); On 2016/04/27 00:43:01, eroman wrote: > I wonder if a read length of 0 make sense. Maybe use 1 as the minimum? Done. https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:91: GURL("data:" + data_string), net::DEFAULT_PRIORITY, &delegate_); On 2016/04/27 00:43:01, eroman wrote: > What happens in the case of invalid GURLs? i.e. GURL::is_valid() I early returned if the url wasn't valid. Not sure if there are other cases we should be avoiding. https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:97: base::RunLoop().RunUntilIdle(); On 2016/04/27 00:43:01, eroman wrote: > This is making some assumptions about how the code runs. > > Can you instead exercise the generic URLRequest API and check the return value > of Read() and wait for the read to complete? Done. I made this class a URLRequest::Delegate to track the reads. https://codereview.chromium.org/1919013003/diff/40001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:103: read_lengths_.clear(); On 2016/04/27 00:43:01, eroman wrote: > I would actually put this at the start of the function instead, or make it a > local variable. Moved it to the top, as it is now referenced in various other methods.
lgtm https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:19: const size_t kMaxLengthForFuzzedData = 512; Can you remove this constant? I think instead what we an do is make data_bytes_ a std::vector<>, and before consuming the remaining data ensure that it is at least as large as the remaining data. This approach I believe has the advantage of scaling to however large the fuzzed input data is. From a performance prerspective since we are re-using the same std::vector<>, I expect our resizes will be cheap and re-use storage except when growing (i.e. amortized constant time over the lifetime of fuzzer). The constant below though seems fine. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:44: read_lengths_.clear(); optional: I seem to remember that resize(0) runs more quickly, since clear() tends to shrink the capacity too. I could be wrong; no change necessary. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:60: // Generate a sequence of reads sufficient to read the entire data url. nit: url --> URL throughout. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:71: GURL data_url = GURL(std::string(data_bytes_, data_size)); nit: GURL data_url(std::string(...)); https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:73: return 0; rather than return 0, I would suggest using some default data:URL value instead and proceeding. The advantage of this approach, is we will still be fuzzing the range request, so it gives the fuzzer more ways to manipulate data to test ranges. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:82: return 0; Same here -- might as well just proceed with no headers, and explore the data-url space. Although I guess if _neither_ were invalid then it is maybe pointless.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
[+ochang]: New fuzzer, FYI.
https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:19: const size_t kMaxLengthForFuzzedData = 512; On 2016/04/28 00:38:24, eroman wrote: > Can you remove this constant? > > I think instead what we an do is make data_bytes_ a std::vector<>, and before > consuming the remaining data ensure that it is at least as large as the > remaining data. > > This approach I believe has the advantage of scaling to however large the fuzzed > input data is. > From a performance prerspective since we are re-using the same std::vector<>, I > expect our resizes will be cheap and re-use storage except when growing (i.e. > amortized constant time over the lifetime of fuzzer). > > The constant below though seems fine. Done. Good idea. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:44: read_lengths_.clear(); On 2016/04/28 00:38:24, eroman wrote: > optional: > > I seem to remember that resize(0) runs more quickly, since clear() tends to > shrink the capacity too. I could be wrong; no change necessary. Keeping this as is for clarity. From what I read on various SO posts I doubt it will make a big difference. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:60: // Generate a sequence of reads sufficient to read the entire data url. On 2016/04/28 00:38:24, eroman wrote: > nit: url --> URL throughout. Done. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:71: GURL data_url = GURL(std::string(data_bytes_, data_size)); On 2016/04/28 00:38:24, eroman wrote: > nit: > > GURL data_url(std::string(...)); Done. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:73: return 0; On 2016/04/28 00:38:24, eroman wrote: > rather than return 0, I would suggest using some default data:URL value instead > and proceeding. The advantage of this approach, is we will still be fuzzing the > range request, so it gives the fuzzer more ways to manipulate data to test > ranges. Done. https://codereview.chromium.org/1919013003/diff/60001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:82: return 0; On 2016/04/28 00:38:24, eroman wrote: > Same here -- might as well just proceed with no headers, and explore the > data-url space. Although I guess if _neither_ were invalid then it is maybe > pointless. Done.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919013003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919013003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919013003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919013003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/...)
eroman@, mmenke@ can I have a hand parsing the win compile errors? I've not seen "inconsistent dll linkage" error before. https://codereview.chromium.org/1919013003/diff/140001/net/url_request/url_re... File net/url_request/url_request_test_util.h (left): https://codereview.chromium.org/1919013003/diff/140001/net/url_request/url_re... net/url_request/url_request_test_util.h:142: class TestDelegate : public URLRequest::Delegate { Looks like I don't need this anymore, will remove.
Try removing your latest changes to url_request_test_util.h which marked things as NET_EXPORT_PRIVATE. url_request_fuzzer.cc already makes use of TestURLRequestContext without needing to export it, so your change shouldn't need to either.
The CQ bit was checked by csharrison@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/1919013003/#ps180001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919013003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919013003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/aa314dc5eead1a69177d91b70018b93535dbfe20 Cr-Commit-Position: refs/heads/master@{#390734}
Message was sent while issue was closed.
Description was changed from ========== Add fuzzer to test Fuzz URLRequestDataJob BUG=602460 ========== to ========== Add fuzzer to test Fuzz URLRequestDataJob BUG=602460 Committed: https://crrev.com/aa314dc5eead1a69177d91b70018b93535dbfe20 Cr-Commit-Position: refs/heads/master@{#390734} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
