Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1834303002:
Rework the mime sniffer fuzzer. (Closed)
DescriptionRework the mime sniffer fuzzer.
In particular, make it fuzz the URL and content-type
header, and make it check the other top level mime
sniffing function.
Also, rename it so it's more clearly associated with
mime_sniffer.h.
BUG=598397
Committed: https://crrev.com/5552a6a020ac21565f4a92a36d545e8115c56132
Cr-Commit-Position: refs/heads/master@{#383597}
Patch Set 1 #Patch Set 2 : Update comment #
Total comments: 12
Patch Set 3 : Remove ShouldSniffMimeType #Patch Set 4 : Response to comments #Patch Set 5 : Update comment #
Messages
Total messages: 19 (5 generated)
mmenke@chromium.org changed reviewers: + eroman@chromium.org
Eric: Hrm...Didn't plan to send a bunch of reviews your way, just the way things worked out. If you want me to direct my fuzzer elsewhere, happy to, just seems like you've been thinking about fuzzing lately, so think you're better than a domain owner. I hadn't realized there was already a mime sniffer fuzzer when I started, because of the mismatched file name. After I found it, decided it was missing a fair number of branches. https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... File net/base/mime_sniffer_fuzzer.cc (right): https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:43: GURL url("https://unfortunate_site_that_relies_on_mime_sniffing/" + path); This is a bit unfortunate - it means that a lot of the "interesting" search space the fuzzer discovers will be related to GURL. :( We could factor out the code to grab the extension, and update all callsites or something, but I don't think that's worth the effort.
I'm not planning to do a ton of fuzzers all at once, thinking just 1-2 per week for a while. On 2016/03/28 20:20:59, mmenke wrote: > Eric: Hrm...Didn't plan to send a bunch of reviews your way, just the way > things worked out. If you want me to direct my fuzzer elsewhere, happy to, just > seems like you've been thinking about fuzzing lately, so think you're better > than a domain owner. > > I hadn't realized there was already a mime sniffer fuzzer when I started, > because of the mismatched file name. After I found it, decided it was missing a > fair number of branches. > > https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... > File net/base/mime_sniffer_fuzzer.cc (right): > > https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... > net/base/mime_sniffer_fuzzer.cc:43: GURL > url("https://unfortunate_site_that_relies_on_mime_sniffing/" + path); > This is a bit unfortunate - it means that a lot of the "interesting" search > space the fuzzer discovers will be related to GURL. :( > > We could factor out the code to grab the extension, and update all callsites or > something, but I don't think that's worth the effort.
Given that I wrote the original test, sending me the reviews is totally reasonable :)
On 2016/03/28 20:30:35, eroman wrote: > Given that I wrote the original test, sending me the reviews is totally > reasonable :) Ahh, didn't realize that. Had just assumed it was written by the cluster fuzz team...Though, erm...yea, I'm directing these towards you because I saw you added some fuzzers.
Description was changed from ========== Rework the mime sniffer fuzzer. In particular, make it fuzz the URL and content-type header, and make it check all 3 top level mime sniffing functions. Also, rename it so it's more clearly associated with mime_sniffer.h. BUG=598397 ========== to ========== Rework the mime sniffer fuzzer. In particular, make it fuzz the URL and content-type header, and make it check the other top level mime sniffing function. Also, rename it so it's more clearly associated with mime_sniffer.h. BUG=598397 ==========
> Ahh, didn't realize that. Had just assumed it was written by the cluster > fuzz team... Did you assume that because you thought it was a weak test written by someone without insight into how the network stack code works.... or because it was awesome and wow-ed you? https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... File net/base/mime_sniffer_fuzzer.cc (right): https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:17: // Finds the line break in |string_piece|, removes every up to and including the This comment is missing something. removes everything, or removes "every one" https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:18: // line break from |string_piece|, and returns all the Almost like you ended your thought before - then the pterodactyl safely landed optimus prime onto the millenium falcon. https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:19: std::string GetNextArgument(base::StringPiece* string_piece) { not a fan of name "string_piece" |input| would be more descriptive https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:47: net::ShouldSniffMimeType(url, mime_type_hint); This function has a dependence on the URL scheme, however we are only passing in https:// URLs here. Would it be better to feed it a URL whose scheme can vary? https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:50: net::SniffMimeType(input.data(), input.length(), url, mime_type_hint, I presume it may be the case that |!url.is_valid()|. Is this going to hit any DCHECKs in how |url| is consumed?
On 2016/03/28 21:00:26, eroman wrote: > > Ahh, didn't realize that. Had just assumed it was written by the cluster > > fuzz team... > > Did you assume that because you thought it was a weak test written by someone > without insight into how the network stack code works.... or because it was > awesome and wow-ed you? It was actually the file name mismatch...Which was awesome and wow-ed me? https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... File net/base/mime_sniffer_fuzzer.cc (right): https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:17: // Finds the line break in |string_piece|, removes every up to and including the On 2016/03/28 21:00:25, eroman wrote: > This comment is missing something. > > removes everything, or removes "every one" Oops. Done. Rewrote this method just a few times. Too many chefs, and all that...Even when I'm the only chef. :) https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:18: // line break from |string_piece|, and returns all the On 2016/03/28 21:00:26, eroman wrote: > Almost like you ended your thought before - > > > > then the pterodactyl safely landed optimus prime onto the millenium falcon. That's just silly. The pterodactyl was a Decepticon, and would never save Optimus Prime! https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:19: std::string GetNextArgument(base::StringPiece* string_piece) { On 2016/03/28 21:00:25, eroman wrote: > not a fan of name "string_piece" > > |input| would be more descriptive Done. https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:47: net::ShouldSniffMimeType(url, mime_type_hint); On 2016/03/28 21:00:26, eroman wrote: > This function has a dependence on the URL scheme, however we are only passing in > https:// URLs here. > > Would it be better to feed it a URL whose scheme can vary? I've switched this to take entire URL. I was taking the path just to reduce the GURL search space (No IDN, no file:// magic, for instance, which has some different rules), as I was horrified how many of the generated test cases were just probing GURL's logic. Even with just the path, we get hundreds of them. I don't think mime sniffing should ever depend on scheme in the future - we've pushed back against adding any more logic, and I think more dependencies on the URL is something we even more strongly don't want, but that could change, I suppose. I've also remove the call to ShouldSniffMimeType, and now just check the other two. Happy to add it back, it just doesn't seem to do anything exciting enough. https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:50: net::SniffMimeType(input.data(), input.length(), url, mime_type_hint, On 2016/03/28 21:00:26, eroman wrote: > I presume it may be the case that |!url.is_valid()|. Is this going to hit any > DCHECKs in how |url| is consumed? So I don't think that case will currently be hit, and the mime type logic doesn't care, anyways. Changing URL to be an arbitrary string would cause us to hit that case, but it still seems to be fine.
https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... File net/base/mime_sniffer_fuzzer.cc (right): https://codereview.chromium.org/1834303002/diff/20001/net/base/mime_sniffer_f... net/base/mime_sniffer_fuzzer.cc:47: net::ShouldSniffMimeType(url, mime_type_hint); On 2016/03/28 21:22:04, mmenke wrote: > On 2016/03/28 21:00:26, eroman wrote: > > This function has a dependence on the URL scheme, however we are only passing > in > > https:// URLs here. > > > > Would it be better to feed it a URL whose scheme can vary? > > I've switched this to take entire URL. > > I was taking the path just to reduce the GURL search space (No IDN, no file:// > magic, for instance, which has some different rules), as I was horrified how > many of the generated test cases were just probing GURL's logic. Even with just > the path, we get hundreds of them. I don't think mime sniffing should ever > depend on scheme in the future - we've pushed back against adding any more > logic, and I think more dependencies on the URL is something we even more > strongly don't want, but that could change, I suppose. > > I've also remove the call to ShouldSniffMimeType, and now just check the other > two. Happy to add it back, it just doesn't seem to do anything exciting enough. And just to confirm, running it now, 90%+ of the things it saves are it exploring the file:// and filesystem:// URL spaces, with only one line of data. We could force there to be at least two lfs, which would mean as it explores the URL space, there's a higher chance of it exploring other arguments as well, not sure it would get us much, though. It has discovered that .crx is magic, and one of the mime-type hints, at least, so it's not completely wasting its time. I find it interesting to see what it comes up with. Clearly, I need more excitement in my life. Maybe I'll take up watching youtube videos of people bungee jumping.
> It was actually the file name mismatch. I named the file sniff_mime_type_fuzzer.cc because it was testing the function "SniffMimeType", not because I accidentally got it wrong :) Naming the fuzzers based on the filename containing the function under test doesn't always work, since we sometimes require different test files for different functions. In this case for instance I would have kept the file named as it is, and then add a new one sniff_mime_type_from_local_data_fuzzer.cc for the second one, which presumably will converge on test cases more quickly. Shrug. LGTM
On 2016/03/28 22:28:09, eroman wrote: > > It was actually the file name mismatch. > > I named the file sniff_mime_type_fuzzer.cc because it was testing the function > "SniffMimeType", not because I accidentally got it wrong :) > > Naming the fuzzers based on the filename containing the function under test > doesn't always work, since we sometimes require different test files for > different functions. > > In this case for instance I would have kept the file named as it is, and then > add a new one sniff_mime_type_from_local_data_fuzzer.cc for the second one, > which presumably will converge on test cases more quickly. > > Shrug. > > LGTM I think we should try and standardize on sharing a prefix with the original file or something, just from a discoverability standpoint. It is a bit unfortunate that we can't put multiple fuzzers in one file.
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/1834303002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834303002/80001
I agree on both counts. This is kind of a crazy idea, but we could shove everything into one file named after the original source file, and then ifdef the different tests, and control which one is compiled by the target. Now that I wrote that out, it really does sound crazy... So yeah, maybe a practice of using the original file name and adding some suffix to disambiguate when needed is good.
On 2016/03/28 22:28:09, eroman wrote: > > It was actually the file name mismatch. > > I named the file sniff_mime_type_fuzzer.cc because it was testing the function > "SniffMimeType", not because I accidentally got it wrong :) > > Naming the fuzzers based on the filename containing the function under test > doesn't always work, since we sometimes require different test files for > different functions. > > In this case for instance I would have kept the file named as it is, and then > add a new one sniff_mime_type_from_local_data_fuzzer.cc for the second one, > which presumably will converge on test cases more quickly. > > Shrug. > > LGTM Oh, and I'm certainly fine with separate fuzzers, just not sure about the cost, and was thinking the interesting cases would be similar (And when I was testing ShouldSniffMimeType, I got something from sharing code, too). You're definitely right that it would converge more quickly.
Message was sent while issue was closed.
Description was changed from ========== Rework the mime sniffer fuzzer. In particular, make it fuzz the URL and content-type header, and make it check the other top level mime sniffing function. Also, rename it so it's more clearly associated with mime_sniffer.h. BUG=598397 ========== to ========== Rework the mime sniffer fuzzer. In particular, make it fuzz the URL and content-type header, and make it check the other top level mime sniffing function. Also, rename it so it's more clearly associated with mime_sniffer.h. BUG=598397 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Rework the mime sniffer fuzzer. In particular, make it fuzz the URL and content-type header, and make it check the other top level mime sniffing function. Also, rename it so it's more clearly associated with mime_sniffer.h. BUG=598397 ========== to ========== Rework the mime sniffer fuzzer. In particular, make it fuzz the URL and content-type header, and make it check the other top level mime sniffing function. Also, rename it so it's more clearly associated with mime_sniffer.h. BUG=598397 Committed: https://crrev.com/5552a6a020ac21565f4a92a36d545e8115c56132 Cr-Commit-Position: refs/heads/master@{#383597} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5552a6a020ac21565f4a92a36d545e8115c56132 Cr-Commit-Position: refs/heads/master@{#383597} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
