Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890193002:
Add a fuzzer for HttpProxyClientSocket. (Closed)
DescriptionAdd a fuzzer for HttpProxyClientSocket.
BUG=599582
Committed: https://crrev.com/8e9314bcf382b9df7caa3ab331e1b0090c27b62f
Cr-Commit-Position: refs/heads/master@{#387707}
Patch Set 1 #Patch Set 2 : Remove test code, dictionary #
Total comments: 15
Patch Set 3 : Response to comments #Patch Set 4 : NET_EXPORT_PRIVATE #
Messages
Total messages: 25 (11 generated)
Description was changed from ========== HttpProxyClientSocketFuzzer BUG=599582 ========== to ========== Add a fuzzer for HttpProxyClientSocket BUG=599582 ==========
Description was changed from ========== Add a fuzzer for HttpProxyClientSocket BUG=599582 ========== to ========== Add a fuzzer for HttpProxyClientSocket. BUG=599582 ==========
mmenke@chromium.org changed reviewers: + eroman@chromium.org
I've verified that with the right input, connect can succeed, both with and without an auth challenge. I tried adding a dictionary, but I could not get the fuzzer to actually use it, so I'll plan to upload a couple test cases instead.
lgtm https://codereview.chromium.org/1890193002/diff/20001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1890193002/diff/20001/net/BUILD.gn#newcode1985 net/BUILD.gn:1985: dict = "http/http_chunked_decoder_fuzzer.dict" do you think the problem is it isn't being used when run locally, or the construct here is broken? https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:30: // Fuzzer for HttpProxyClientSocket only tests establshing a connection when typo: establishing https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:46: is_https_proxy = !(data[size - 1] & 1); It might be interesting to add our own fuzzer support .h file which can do things like: is_https_proxy = ConsumeBits(1, &data, &size); Or fancier like: // Consume 1 byte of the fuzz data. int length = fuzz_data.ConsumeBits(6); bool sync = 1 == fuzz_data.ConsumeBits(1); bool error = 1 == fuzz_data.ConsumeBits(1); https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:51: scoped_ptr<net::FuzzedSocket> fuzzed_socket( I believe std::unique_ptr<> is the new hotness now. 9 out of 10 dentists recommend it. https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:52: new net::FuzzedSocket(data, size, bound_test_net_log.bound())); Assuming our style allows std::make_unique, this could now be: auto fuzzed_socket = std::make_unique<FuzzedSocket>(data, size, bound_test_net_log.bound()); (avoid typing net::FuzzedSocket twice). https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:73: socket_handle.release(), "Bond/007", net::HostPortPair("foo", 80), shaken not stirred https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:81: while (result == net::ERR_PROXY_AUTH_REQUESTED) { How many times will this loop?
https://codereview.chromium.org/1890193002/diff/20001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1890193002/diff/20001/net/BUILD.gn#newcode1985 net/BUILD.gn:1985: dict = "http/http_chunked_decoder_fuzzer.dict" On 2016/04/15 19:07:21, eroman wrote: > do you think the problem is it isn't being used when run locally, or the > construct here is broken? Oops, thought I removed this. I think it's broken, or I'm not making a dictionary correctly. I filed a bug against it - https://crbug.com/603964 Even if not broken, I'm not going to land something I can't/haven't run successfully locally, and without running it through a try job. https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:30: // Fuzzer for HttpProxyClientSocket only tests establshing a connection when On 2016/04/15 19:07:21, eroman wrote: > typo: establishing Done. https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:46: is_https_proxy = !(data[size - 1] & 1); On 2016/04/15 19:07:21, eroman wrote: > It might be interesting to add our own fuzzer support .h file which can do > things like: > > is_https_proxy = ConsumeBits(1, &data, &size); > > Or fancier like: > > // Consume 1 byte of the fuzz data. > int length = fuzz_data.ConsumeBits(6); > bool sync = 1 == fuzz_data.ConsumeBits(1); > bool error = 1 == fuzz_data.ConsumeBits(1); I'm actually planning on implementing a shared refcounted structure with this functionality (At least byte and string functionality), when I write fuzzers for URLRequest, so it can handle things like cross domain redirects, and non-reuseable sockets, which will require creating new sockets with the remaining bits. https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:51: scoped_ptr<net::FuzzedSocket> fuzzed_socket( On 2016/04/15 19:07:21, eroman wrote: > I believe std::unique_ptr<> is the new hotness now. > 9 out of 10 dentists recommend it. Dang dentists...Always messing with a good thing (Done.). https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:52: new net::FuzzedSocket(data, size, bound_test_net_log.bound())); On 2016/04/15 19:07:21, eroman wrote: > Assuming our style allows std::make_unique, this could now be: > > auto fuzzed_socket = std::make_unique<FuzzedSocket>(data, size, > bound_test_net_log.bound()); > > (avoid typing net::FuzzedSocket twice). I'm happy with the old ways...The ways of our great forecoders. (Actually, I just think having FuzzedSocket near the start of the line is easier to follow, so leaving as-is) https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:81: while (result == net::ERR_PROXY_AUTH_REQUESTED) { On 2016/04/15 19:07:21, eroman wrote: > How many times will this loop? Until it stops reading new auth challenges. If the credentials aren't valid, the server can respond with another challenge. Every time we consume a fair number of bytes (At least 60+, for the challenge), so I don't think that's going to be a problem. There is no re-challenge limit anywhere in net/, that I'm aware of, but I don't think the bots will find sending dozens of challenges that interesting.
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/1890193002/#ps40001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890193002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890193002/40001
lgtm https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket_fuzzer.cc (right): https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:46: is_https_proxy = !(data[size - 1] & 1); On 2016/04/15 19:23:52, mmenke wrote: > On 2016/04/15 19:07:21, eroman wrote: > > It might be interesting to add our own fuzzer support .h file which can do > > things like: > > > > is_https_proxy = ConsumeBits(1, &data, &size); > > > > Or fancier like: > > > > // Consume 1 byte of the fuzz data. > > int length = fuzz_data.ConsumeBits(6); > > bool sync = 1 == fuzz_data.ConsumeBits(1); > > bool error = 1 == fuzz_data.ConsumeBits(1); > > I'm actually planning on implementing a shared refcounted structure with this > functionality (At least byte and string functionality), when I write fuzzers for > URLRequest, so it can handle things like cross domain redirects, and > non-reuseable sockets, which will require creating new sockets with the > remaining bits. sounds good, I was envisioning something similar :) https://codereview.chromium.org/1890193002/diff/20001/net/http/http_proxy_cli... net/http/http_proxy_client_socket_fuzzer.cc:81: while (result == net::ERR_PROXY_AUTH_REQUESTED) { On 2016/04/15 19:23:52, mmenke wrote: > On 2016/04/15 19:07:21, eroman wrote: > > How many times will this loop? > > Until it stops reading new auth challenges. If the credentials aren't valid, > the server can respond with another challenge. Every time we consume a fair > number of bytes (At least 60+, for the challenge), so I don't think that's going > to be a problem. There is no re-challenge limit anywhere in net/, that I'm > aware of, but I don't think the bots will find sending dozens of challenges that > interesting. Good point - this necessarily terminates provided we consume at least 1 byte of data each time.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890193002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890193002/40001
Eric: Am I missing something obvious here? That one builder really does not want to link this fuzzer, but I can't figure out any difference between this and the SOCKS fuzzers I landed yesterday, other than that the socks files are non_nacl resources, which doesn't seem like it should matter.
On 2016/04/15 20:35:51, mmenke wrote: > Eric: Am I missing something obvious here? That one builder really does not > want to link this fuzzer, but I can't figure out any difference between this and > the SOCKS fuzzers I landed yesterday, other than that the socks files are > non_nacl resources, which doesn't seem like it should matter. Oh, no NET_EXPORT_PRIVATE....Nevermind. That does mean we're not testing it directly, though.
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/1890193002/#ps60001 (title: "NET_EXPORT_PRIVATE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890193002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890193002/60001
Message was sent while issue was closed.
Description was changed from ========== Add a fuzzer for HttpProxyClientSocket. BUG=599582 ========== to ========== Add a fuzzer for HttpProxyClientSocket. BUG=599582 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a fuzzer for HttpProxyClientSocket. BUG=599582 ========== to ========== Add a fuzzer for HttpProxyClientSocket. BUG=599582 Committed: https://crrev.com/8e9314bcf382b9df7caa3ab331e1b0090c27b62f Cr-Commit-Position: refs/heads/master@{#387707} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8e9314bcf382b9df7caa3ab331e1b0090c27b62f Cr-Commit-Position: refs/heads/master@{#387707}
Message was sent while issue was closed.
I checked logs for these fuzzers. Looks like they are stuck, they find only one testcase per hour (sometimes 2-3, but very barely). Do we have any testcases to use as seed corpus? We can store them inside the repo or just upload to: https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... and https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer...
Message was sent while issue was closed.
On 2016/05/04 15:17:43, mmoroz wrote: > I checked logs for these fuzzers. Looks like they are stuck, they find only one > testcase per hour (sometimes 2-3, but very barely). > > Do we have any testcases to use as seed corpus? We can store them inside the > repo or just upload to: > https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... > and > https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... Those links are for the socks proxy fuzzers (Which are, admittedly, for fairly simple code), which this CL is for the HTTP proxy fuzzer, which is a wee bit more complicated. Which fuzzer are you referring to? If you're talking about the socks/socks5 fuzzers, those are both simple enough cases that I don't think more is needed, and would expect fairly modest sized corpuses for both - in fact, if there's a way to tell the fuzzer infrastructure "Run this fuzzer a lot less often, as it doesn't cover as much", I think it would make a lot of sense for those two. If you're talking about the HTTP proxy client socket fuzzer, that fuzzer has a much larger space to explore, so issues with that one would be more concerning. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
