Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10527004:
Make dns_fuzz_stub read and parse JSON test case (Closed)
|
Created:
8 years, 6 months ago by Deprecated (see juliatuttle) Modified:
8 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, cbentzel+watch_chromium.org, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake dns_fuzz_stub read and parse JSON test case
BUG=130751
TEST=Adhoc; converted sample test case to JSON, and stub still works
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141244
Patch Set 1 #
Total comments: 11
Patch Set 2 : #
Total comments: 6
Patch Set 3 : Print #EOF when DNS fuzz stub succeeds #Patch Set 4 : sigh. that was for a different issue. #Patch Set 5 : #Messages
Total messages: 16 (0 generated)
https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... File net/tools/dns_fuzz_stub/dns_fuzz_stub.cc (right): https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:15: #include "base/memory/scoped_ptr.h" fix ordering https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:89: resp_buf->push_back(static_cast<char>(byte_int)); The behavior of this cast is not defined by the standard. ("implementation-defined") One option: reinterpret_cast<char&>(static_cast<uint8>(byte_int))
https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... File net/tools/dns_fuzz_stub/dns_fuzz_stub.cc (right): https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:89: resp_buf->push_back(static_cast<char>(byte_int)); On 2012/06/04 20:26:24, szym wrote: > The behavior of this cast is not defined by the standard. > ("implementation-defined") One option: > > reinterpret_cast<char&>(static_cast<uint8>(byte_int)) Sorry, that won't work directly, but you get my point. Perhaps I'm overly paranoid. This code is not meant to run outside of our context anyway.
http://codereview.chromium.org/10527004/diff/1/net/tools/dns_fuzz_stub/dns_fu... File net/tools/dns_fuzz_stub/dns_fuzz_stub.cc (right): http://codereview.chromium.org/10527004/diff/1/net/tools/dns_fuzz_stub/dns_fu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:45: Value* value = base::JSONReader::Read(json); Use a scoped_ptr here for the returned value. Optional exercise, not for this CL: Convert jsonreader to return scoped_ptr<Value> instead. http://codereview.chromium.org/10527004/diff/1/net/tools/dns_fuzz_stub/dns_fu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:53: CHECK(value->GetAsDictionary(&dict)); Is this true? What if the file just consisted of a "2" http://codereview.chromium.org/10527004/diff/1/net/tools/dns_fuzz_stub/dns_fu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:56: if (!dict->GetInteger("id", &id_int) || !FitsUint16(id_int)) { Although boolean short-circuiting should make this work, combining the part about setting id_int and passing it into FitsUint16 in the same expression makes me concerned. I'd prefer to split this into two separate expressions.
PTAL. https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... File net/tools/dns_fuzz_stub/dns_fuzz_stub.cc (right): https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:15: #include "base/memory/scoped_ptr.h" On 2012/06/04 20:26:24, szym wrote: > fix ordering Done. https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:45: Value* value = base::JSONReader::Read(json); On 2012/06/04 23:49:23, cbentzel wrote: > Use a scoped_ptr here for the returned value. > > Optional exercise, not for this CL: Convert jsonreader to return > scoped_ptr<Value> instead. Done. https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:53: CHECK(value->GetAsDictionary(&dict)); On 2012/06/04 23:49:23, cbentzel wrote: > Is this true? What if the file just consisted of a "2" It's actually not; the spec requires that it be a dictionary *or an array*. (Just the number "2" would not be valid.) I fixed it. https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:56: if (!dict->GetInteger("id", &id_int) || !FitsUint16(id_int)) { On 2012/06/04 23:49:23, cbentzel wrote: > Although boolean short-circuiting should make this work, combining the part > about setting id_int and passing it into FitsUint16 in the same expression makes > me concerned. I'd prefer to split this into two separate expressions. Done. https://chromiumcodereview.appspot.com/10527004/diff/1/net/tools/dns_fuzz_stu... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:89: resp_buf->push_back(static_cast<char>(byte_int)); On 2012/06/04 20:26:24, szym wrote: > The behavior of this cast is not defined by the standard. > ("implementation-defined") One option: > > reinterpret_cast<char&>(static_cast<uint8>(byte_int)) We decided to stick with this, since it works and getting it right in a portable fashion is insanely tricky.
lgtm
https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... File net/tools/dns_fuzz_stub/dns_fuzz_stub.cc (right): https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:58: if (!dict->GetInteger("id", &id_int)) { Nit: You could write a helper function such as bool GetUint16Value(DictionaryValue* dict, const char* key, uint16* value); Since only used twice (for "id" and "qtype") I'm not too concerned about this, but would push if you added a third time. https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:68: if (!dict->GetStringASCII("qname", qname)) { Should you validate that the qname is well formed? https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:103: resp_buf->push_back(static_cast<char>(resp_byte_int)); Should this be a uint8 vector?
https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... File net/tools/dns_fuzz_stub/dns_fuzz_stub.cc (right): https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:68: if (!dict->GetStringASCII("qname", qname)) { On 2012/06/06 14:50:56, cbentzel wrote: > Should you validate that the qname is well formed? If it isn't, the stub will fail in line 140. https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:103: resp_buf->push_back(static_cast<char>(resp_byte_int)); On 2012/06/06 14:50:56, cbentzel wrote: > Should this be a uint8 vector? IOBuffer holds char (which makes me a bit uneasy), but perhaps it's better to static_cast<uint8> here and then reinterpret_cast<char*> or just memcpy in line 145.
LGTM https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... File net/tools/dns_fuzz_stub/dns_fuzz_stub.cc (right): https://chromiumcodereview.appspot.com/10527004/diff/7001/net/tools/dns_fuzz_... net/tools/dns_fuzz_stub/dns_fuzz_stub.cc:103: resp_buf->push_back(static_cast<char>(resp_byte_int)); On 2012/06/06 14:53:57, szym wrote: > On 2012/06/06 14:50:56, cbentzel wrote: > > Should this be a uint8 vector? > > IOBuffer holds char (which makes me a bit uneasy), but perhaps it's better to > static_cast<uint8> here and then reinterpret_cast<char*> or just memcpy in line > 145. Let's keep as char. Perhaps we should change IOBuffer instead.
Am I good to commit this?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10527004/7001
Try job failure for 10527004-7001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10527004/7001
Try job failure for 10527004-7001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10527004/7001
Change committed as 141244 |
