Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14049018:
Add simple non-response-based question parsing for mDNS passive listening (Closed)
|
Created:
7 years, 8 months ago by Noam Samuel Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd simple non-response-based question parsing for mDNS passive listening
Added method InitParseNoQuery to begin parsing a DNS response even if there is
no matching query. This is useful in mDNS passive listening (for example, cacheing the results of unsolicited announcements as described in section 8.3 of http://www.rfc-editor.org/rfc/rfc6762.txt)
BUG=233821
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195937
Patch Set 1 : #Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #
Total comments: 13
Patch Set 6 : Respond to reviewer comments #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Messages
Total messages: 31 (0 generated)
Hey, I'm going through the DNS record parsing code and seeing if there are any obvious features that need to be added for mDNS and DNS-SD. I think this one will definitely be needed since mDNS requires parsing DNS "responses" with no question section.
Added support for pointers (compresssed domains) in question section. On 2013/04/15 20:49:56, Noam Samuel (chromium) wrote: > Hey, I'm going through the DNS record parsing code and seeing if there are any > obvious features that need to be added for mDNS and DNS-SD. I think this one > will definitely be needed since mDNS requires parsing DNS "responses" with no > question section.
https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:180: bool DnsResponse::InitParseNoQuery(int nbytes) { nit: you may consider naming this InitParse() as well. https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:235: size_t DnsResponse::GetQuestionSectionSize() const { nit: You may consider changing this function to GetResponseOffset(), this will simplify your InitParseNoQuery() a bit https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:243: while (pos < data_size && data[pos]) { data[pos] -> data[pos] != 0 you can make it more explicit and easy to read with () && () https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:248: if (static_cast<unsigned>(data[pos]) >= 192) { could you please make a new variable usigned txt_len = static_cast<unsigned>(data[pos]); https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:261: if (static_cast<int>(data[pos] + pos + 1 > data_size)) why do you need static_cast here? btw, you may move this check to the outside of this loop https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:264: pos += data[pos]+1; need spaces aroud + https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:267: pos += 5; // 2 for qtype, 2 for qclass, 1 for null char Comment is a bit misleading, I think it should be // 1 for 0-length text, 2 for qtype, 2 for qclass
https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:180: bool DnsResponse::InitParseNoQuery(int nbytes) { On 2013/04/16 21:55:22, gene wrote: > nit: you may consider naming this InitParse() as well. Done. https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:235: size_t DnsResponse::GetQuestionSectionSize() const { On 2013/04/16 21:55:22, gene wrote: > nit: You may consider changing this function to GetResponseOffset(), this will > simplify your InitParseNoQuery() a bit Done. https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:243: while (pos < data_size && data[pos]) { On 2013/04/16 21:55:22, gene wrote: > data[pos] -> data[pos] != 0 > you can make it more explicit and easy to read with () && () Done. https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:261: if (static_cast<int>(data[pos] + pos + 1 > data_size)) On 2013/04/16 21:55:22, gene wrote: > why do you need static_cast here? > btw, you may move this check to the outside of this loop Done. https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:264: pos += data[pos]+1; On 2013/04/16 21:55:22, gene wrote: > need spaces aroud + Done. https://codereview.chromium.org/14049018/diff/4001/net/dns/dns_response.cc#ne... net/dns/dns_response.cc:267: pos += 5; // 2 for qtype, 2 for qclass, 1 for null char On 2013/04/16 21:55:22, gene wrote: > Comment is a bit misleading, I think it should be > // 1 for 0-length text, 2 for qtype, 2 for qclass Done.
almost there :) https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:185: parser_ = DnsRecordParser(io_buffer_->data(), nit: you can fit this in one line now https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:187: GetResponseOffset()); do you want to fail InitParse if GetResponseOffset() returns 0?
https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:7: #include <climits> you probably don't need this any more
https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:7: #include <climits> On 2013/04/17 02:05:38, gene wrote: > you probably don't need this any more Done. https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:185: parser_ = DnsRecordParser(io_buffer_->data(), On 2013/04/17 02:04:02, gene wrote: > nit: you can fit this in one line now Done. https://codereview.chromium.org/14049018/diff/11001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:187: GetResponseOffset()); On 2013/04/17 02:04:02, gene wrote: > do you want to fail InitParse if GetResponseOffset() returns 0? Yes.
lgtm
+szym Does this look good? It's part of a wider effort to add DNS-SD/mDNS support to chrome.
Needs work. https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:234: size_t DnsResponse::GetResponseOffset() const { Use DnsRecordParser instead. ReadName with |out| set NULL simply skips the name. I suggest you add: bool DnsRecordParser::SkipQuestion() { size_t consumed = ReadName(cur_, NULL); if (!consumed) return false; cur_ += consumed + 2 * sizeof(uint16); // QTYPE + QCLASS; if (cur_ > io_buffer_->size()) return false; } Then InitParseWithoutQuery is: parser_ = DnsRecordParser(io_buffer_->data(), nbytes, sizeof(dns_protocol::Header)); unsigned qdcount = base::NetToHost16(header()->qdcount); for (unsigned i = 0; i < qdcount; ++i) { if (!parser_.SkipQuestion()) return false; } return true; https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:249: continue; This is an error. At this point you should be done with the name, so this should be "break". https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h File net/dns/dns_response.h (right): https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... net/dns/dns_response.h:120: bool InitParse(int nbytes); I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since the semantics are substantially different from InitParse. The comment should also indicate that the parser is still positioned after the question section. https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... net/dns/dns_response.h:140: size_t GetResponseOffset() const; "Response" is confusing. (This is DnsResponse.) I suggest "answer" in reference to "answer section". That said, I don't think this is needed, see comments in .cc https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... File net/dns/dns_response_unittest.cc (right): https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:376: 0x00, This is not a valid DNS response. The pointer is the last label. There's no extra null-label after it. https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:412: // TODO(noamsml): simplify test I suggest you reuse the response packets in dns_test_util.h It makes sense to add another test for the cases of qdcount == 0 and qdcount > 1.
Also - I'd like to see compile time flags (mainly in net/net.gyp) so embedders of the network library that will not use mDNS do not need to build or increase library size. On Wed, Apr 17, 2013 at 6:11 PM, <szym@chromium.org> wrote: > Needs work. > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc > File net/dns/dns_response.cc (right): > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... > net/dns/dns_response.cc:234: size_t DnsResponse::GetResponseOffset() > const { > Use DnsRecordParser instead. ReadName with |out| set NULL simply skips > the name. > > I suggest you add: > bool DnsRecordParser::SkipQuestion() { > size_t consumed = ReadName(cur_, NULL); > if (!consumed) > return false; > cur_ += consumed + 2 * sizeof(uint16); // QTYPE + QCLASS; > if (cur_ > io_buffer_->size()) > return false; > } > > Then InitParseWithoutQuery is: > > parser_ = DnsRecordParser(io_buffer_->data(), nbytes, > sizeof(dns_protocol::Header)); > unsigned qdcount = base::NetToHost16(header()->qdcount); > for (unsigned i = 0; i < qdcount; ++i) { > if (!parser_.SkipQuestion()) > return false; > } > return true; > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... > net/dns/dns_response.cc:249: continue; > This is an error. At this point you should be done with the name, so > this should be "break". > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h > File net/dns/dns_response.h (right): > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... > net/dns/dns_response.h:120: bool InitParse(int nbytes); > I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since > the semantics are substantially different from InitParse. > > The comment should also indicate that the parser is still positioned > after the question section. > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... > net/dns/dns_response.h:140: size_t GetResponseOffset() const; > "Response" is confusing. (This is DnsResponse.) I suggest "answer" in > reference to "answer section". > > That said, I don't think this is needed, see comments in .cc > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > File net/dns/dns_response_unittest.cc (right): > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > net/dns/dns_response_unittest.cc:376: 0x00, > This is not a valid DNS response. The pointer is the last label. There's > no extra null-label after it. > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > net/dns/dns_response_unittest.cc:412: // TODO(noamsml): simplify test > I suggest you reuse the response packets in dns_test_util.h > It makes sense to add another test for the cases of qdcount == 0 and > qdcount > 1. > > https://codereview.chromium.org/14049018/
See disable_ftp_support for an example. Although in this case I think it should default to off and get enabled when a flag is set. On Wed, Apr 17, 2013 at 7:27 PM, Chris Bentzel <cbentzel@chromium.org> wrote: > Also - I'd like to see compile time flags (mainly in net/net.gyp) so > embedders of the network library that will not use mDNS do not need to > build or increase library size. > > On Wed, Apr 17, 2013 at 6:11 PM, <szym@chromium.org> wrote: >> Needs work. >> >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc >> File net/dns/dns_response.cc (right): >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... >> net/dns/dns_response.cc:234: size_t DnsResponse::GetResponseOffset() >> const { >> Use DnsRecordParser instead. ReadName with |out| set NULL simply skips >> the name. >> >> I suggest you add: >> bool DnsRecordParser::SkipQuestion() { >> size_t consumed = ReadName(cur_, NULL); >> if (!consumed) >> return false; >> cur_ += consumed + 2 * sizeof(uint16); // QTYPE + QCLASS; >> if (cur_ > io_buffer_->size()) >> return false; >> } >> >> Then InitParseWithoutQuery is: >> >> parser_ = DnsRecordParser(io_buffer_->data(), nbytes, >> sizeof(dns_protocol::Header)); >> unsigned qdcount = base::NetToHost16(header()->qdcount); >> for (unsigned i = 0; i < qdcount; ++i) { >> if (!parser_.SkipQuestion()) >> return false; >> } >> return true; >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... >> net/dns/dns_response.cc:249: continue; >> This is an error. At this point you should be done with the name, so >> this should be "break". >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h >> File net/dns/dns_response.h (right): >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... >> net/dns/dns_response.h:120: bool InitParse(int nbytes); >> I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since >> the semantics are substantially different from InitParse. >> >> The comment should also indicate that the parser is still positioned >> after the question section. >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... >> net/dns/dns_response.h:140: size_t GetResponseOffset() const; >> "Response" is confusing. (This is DnsResponse.) I suggest "answer" in >> reference to "answer section". >> >> That said, I don't think this is needed, see comments in .cc >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... >> File net/dns/dns_response_unittest.cc (right): >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... >> net/dns/dns_response_unittest.cc:376: 0x00, >> This is not a valid DNS response. The pointer is the last label. There's >> no extra null-label after it. >> >> https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... >> net/dns/dns_response_unittest.cc:412: // TODO(noamsml): simplify test >> I suggest you reuse the response packets in dns_test_util.h >> It makes sense to add another test for the cases of qdcount == 0 and >> qdcount > 1. >> >> https://codereview.chromium.org/14049018/
https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h File net/dns/dns_response.h (right): https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... net/dns/dns_response.h:120: bool InitParse(int nbytes); On 2013/04/17 22:11:12, szym wrote: > I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since the > semantics are substantially different from InitParse. > > The comment should also indicate that the parser is still positioned after the > question section. After some more thinking, I suggest InitParseIgnoreQuery. I'd also be OK if you used InitParse here and InitParseMatchQuery for the other method.
Another example is ENABLE_BUILT_IN_DNS . On Wed, Apr 17, 2013 at 7:28 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > See disable_ftp_support for an example. Although in this case I think > it should default to off and get enabled when a flag is set. > > On Wed, Apr 17, 2013 at 7:27 PM, Chris Bentzel <cbentzel@chromium.org> > wrote: > > Also - I'd like to see compile time flags (mainly in net/net.gyp) so > > embedders of the network library that will not use mDNS do not need to > > build or increase library size. > > > > On Wed, Apr 17, 2013 at 6:11 PM, <szym@chromium.org> wrote: > >> Needs work. > >> > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc > >> File net/dns/dns_response.cc (right): > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... > >> net/dns/dns_response.cc:234: size_t DnsResponse::GetResponseOffset() > >> const { > >> Use DnsRecordParser instead. ReadName with |out| set NULL simply skips > >> the name. > >> > >> I suggest you add: > >> bool DnsRecordParser::SkipQuestion() { > >> size_t consumed = ReadName(cur_, NULL); > >> if (!consumed) > >> return false; > >> cur_ += consumed + 2 * sizeof(uint16); // QTYPE + QCLASS; > >> if (cur_ > io_buffer_->size()) > >> return false; > >> } > >> > >> Then InitParseWithoutQuery is: > >> > >> parser_ = DnsRecordParser(io_buffer_->data(), nbytes, > >> sizeof(dns_protocol::Header)); > >> unsigned qdcount = base::NetToHost16(header()->qdcount); > >> for (unsigned i = 0; i < qdcount; ++i) { > >> if (!parser_.SkipQuestion()) > >> return false; > >> } > >> return true; > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... > >> net/dns/dns_response.cc:249: continue; > >> This is an error. At this point you should be done with the name, so > >> this should be "break". > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h > >> File net/dns/dns_response.h (right): > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... > >> net/dns/dns_response.h:120: bool InitParse(int nbytes); > >> I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since > >> the semantics are substantially different from InitParse. > >> > >> The comment should also indicate that the parser is still positioned > >> after the question section. > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... > >> net/dns/dns_response.h:140: size_t GetResponseOffset() const; > >> "Response" is confusing. (This is DnsResponse.) I suggest "answer" in > >> reference to "answer section". > >> > >> That said, I don't think this is needed, see comments in .cc > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > >> File net/dns/dns_response_unittest.cc (right): > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > >> net/dns/dns_response_unittest.cc:376: 0x00, > >> This is not a valid DNS response. The pointer is the last label. There's > >> no extra null-label after it. > >> > >> > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > >> net/dns/dns_response_unittest.cc:412: // TODO(noamsml): simplify test > >> I suggest you reuse the response packets in dns_test_util.h > >> It makes sense to add another test for the cases of qdcount == 0 and > >> qdcount > 1. > >> > >> https://codereview.chromium.org/14049018/ >
Should I wrap this functionality in #ifndefs? Or does this apply only for mDNS-specific code? On 2013/04/17 23:33:00, szym wrote: > Another example is ENABLE_BUILT_IN_DNS . > > > On Wed, Apr 17, 2013 at 7:28 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > > > See disable_ftp_support for an example. Although in this case I think > > it should default to off and get enabled when a flag is set. > > > > On Wed, Apr 17, 2013 at 7:27 PM, Chris Bentzel <mailto:cbentzel@chromium.org> > > wrote: > > > Also - I'd like to see compile time flags (mainly in net/net.gyp) so > > > embedders of the network library that will not use mDNS do not need to > > > build or increase library size. > > > > > > On Wed, Apr 17, 2013 at 6:11 PM, <mailto:szym@chromium.org> wrote: > > >> Needs work. > > >> > > >> > > >> > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc > > >> File net/dns/dns_response.cc (right): > > >> > > >> > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... > > >> net/dns/dns_response.cc:234: size_t DnsResponse::GetResponseOffset() > > >> const { > > >> Use DnsRecordParser instead. ReadName with |out| set NULL simply skips > > >> the name. > > >> > > >> I suggest you add: > > >> bool DnsRecordParser::SkipQuestion() { > > >> size_t consumed = ReadName(cur_, NULL); > > >> if (!consumed) > > >> return false; > > >> cur_ += consumed + 2 * sizeof(uint16); // QTYPE + QCLASS; > > >> if (cur_ > io_buffer_->size()) > > >> return false; > > >> } > > >> > > >> Then InitParseWithoutQuery is: > > >> > > >> parser_ = DnsRecordParser(io_buffer_->data(), nbytes, > > >> sizeof(dns_protocol::Header)); > > >> unsigned qdcount = base::NetToHost16(header()->qdcount); > > >> for (unsigned i = 0; i < qdcount; ++i) { > > >> if (!parser_.SkipQuestion()) > > >> return false; > > >> } > > >> return true; > > >> > > >> > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... > > >> net/dns/dns_response.cc:249: continue; > > >> This is an error. At this point you should be done with the name, so > > >> this should be "break". > > >> > > >> > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h > > >> File net/dns/dns_response.h (right): > > >> > > >> > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... > > >> net/dns/dns_response.h:120: bool InitParse(int nbytes); > > >> I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since > > >> the semantics are substantially different from InitParse. > > >> > > >> The comment should also indicate that the parser is still positioned > > >> after the question section. > > >> > > >> > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... > > >> net/dns/dns_response.h:140: size_t GetResponseOffset() const; > > >> "Response" is confusing. (This is DnsResponse.) I suggest "answer" in > > >> reference to "answer section". > > >> > > >> That said, I don't think this is needed, see comments in .cc > > >> > > >> > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > > >> File net/dns/dns_response_unittest.cc (right): > > >> > > >> > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > > >> net/dns/dns_response_unittest.cc:376: 0x00, > > >> This is not a valid DNS response. The pointer is the last label. There's > > >> no extra null-label after it. > > >> > > >> > > > https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... > > >> net/dns/dns_response_unittest.cc:412: // TODO(noamsml): simplify test > > >> I suggest you reuse the response packets in dns_test_util.h > > >> It makes sense to add another test for the cases of qdcount == 0 and > > >> qdcount > 1. > > >> > > >> https://codereview.chromium.org/14049018/ > >
Frankly, in this cl, I'm ok with these two simple methods being always compiled. On Apr 17, 2013 9:03 PM, <noamsml@chromium.org> wrote: > Should I wrap this functionality in #ifndefs? Or does this apply only for > mDNS-specific code? > > On 2013/04/17 23:33:00, szym wrote: > >> Another example is ENABLE_BUILT_IN_DNS . >> > > > On Wed, Apr 17, 2013 at 7:28 PM, Chris Bentzel <cbentzel@chromium.org >> >wrote: >> > > > See disable_ftp_support for an example. Although in this case I think >> > it should default to off and get enabled when a flag is set. >> > >> > On Wed, Apr 17, 2013 at 7:27 PM, Chris Bentzel >> > <mailto:cbentzel@chromium.org> > >> > wrote: >> > > Also - I'd like to see compile time flags (mainly in net/net.gyp) so >> > > embedders of the network library that will not use mDNS do not need to >> > > build or increase library size. >> > > >> > > On Wed, Apr 17, 2013 at 6:11 PM, <mailto:szym@chromium.org> wrote: >> > >> Needs work. >> > >> >> > >> >> > >> >> > https://codereview.chromium.**org/14049018/diff/18001/net/** >> dns/dns_response.cc<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc> >> > >> File net/dns/dns_response.cc (right): >> > >> >> > >> >> > >> > > https://codereview.chromium.**org/14049018/diff/18001/net/** > dns/dns_response.cc#newcode234<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#newcode234> > >> > >> net/dns/dns_response.cc:234: size_t DnsResponse::** >> GetResponseOffset() >> > >> const { >> > >> Use DnsRecordParser instead. ReadName with |out| set NULL simply >> skips >> > >> the name. >> > >> >> > >> I suggest you add: >> > >> bool DnsRecordParser::SkipQuestion(**) { >> > >> size_t consumed = ReadName(cur_, NULL); >> > >> if (!consumed) >> > >> return false; >> > >> cur_ += consumed + 2 * sizeof(uint16); // QTYPE + QCLASS; >> > >> if (cur_ > io_buffer_->size()) >> > >> return false; >> > >> } >> > >> >> > >> Then InitParseWithoutQuery is: >> > >> >> > >> parser_ = DnsRecordParser(io_buffer_->**data(), nbytes, >> > >> sizeof(dns_protocol::Header)); >> > >> unsigned qdcount = base::NetToHost16(header()->**qdcount); >> > >> for (unsigned i = 0; i < qdcount; ++i) { >> > >> if (!parser_.SkipQuestion()) >> > >> return false; >> > >> } >> > >> return true; >> > >> >> > >> >> > >> > > https://codereview.chromium.**org/14049018/diff/18001/net/** > dns/dns_response.cc#newcode249<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#newcode249> > >> > >> net/dns/dns_response.cc:249: continue; >> > >> This is an error. At this point you should be done with the name, so >> > >> this should be "break". >> > >> >> > >> >> > https://codereview.chromium.**org/14049018/diff/18001/net/** >> dns/dns_response.h<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h> >> > >> File net/dns/dns_response.h (right): >> > >> >> > >> >> > >> > > https://codereview.chromium.**org/14049018/diff/18001/net/** > dns/dns_response.h#newcode120<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#newcode120> > >> > >> net/dns/dns_response.h:120: bool InitParse(int nbytes); >> > >> I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since >> > >> the semantics are substantially different from InitParse. >> > >> >> > >> The comment should also indicate that the parser is still positioned >> > >> after the question section. >> > >> >> > >> >> > >> > > https://codereview.chromium.**org/14049018/diff/18001/net/** > dns/dns_response.h#newcode140<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#newcode140> > >> > >> net/dns/dns_response.h:140: size_t GetResponseOffset() const; >> > >> "Response" is confusing. (This is DnsResponse.) I suggest "answer" in >> > >> reference to "answer section". >> > >> >> > >> That said, I don't think this is needed, see comments in .cc >> > >> >> > >> >> > >> > > https://codereview.chromium.**org/14049018/diff/18001/net/** > dns/dns_response_unittest.cc<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unittest.cc> > >> > >> File net/dns/dns_response_unittest.**cc (right): >> > >> >> > >> >> > >> > > https://codereview.chromium.**org/14049018/diff/18001/net/** > dns/dns_response_unittest.cc#**newcode376<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unittest.cc#newcode376> > >> > >> net/dns/dns_response_unittest.**cc:376: 0x00, >> > >> This is not a valid DNS response. The pointer is the last label. >> There's >> > >> no extra null-label after it. >> > >> >> > >> >> > >> > > https://codereview.chromium.**org/14049018/diff/18001/net/** > dns/dns_response_unittest.cc#**newcode412<https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unittest.cc#newcode412> > >> > >> net/dns/dns_response_unittest.**cc:412: // TODO(noamsml): simplify >> test >> > >> I suggest you reuse the response packets in dns_test_util.h >> > >> It makes sense to add another test for the cases of qdcount == 0 and >> > >> qdcount > 1. >> > >> >> > >> https://codereview.chromium.**org/14049018/<https://codereview.chromium.org/1... >> > >> > > > > https://codereview.chromium.**org/14049018/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:234: size_t DnsResponse::GetResponseOffset() const { On 2013/04/17 22:11:12, szym wrote: > Use DnsRecordParser instead. ReadName with |out| set NULL simply skips the name. > > I suggest you add: > bool DnsRecordParser::SkipQuestion() { > size_t consumed = ReadName(cur_, NULL); > if (!consumed) > return false; > cur_ += consumed + 2 * sizeof(uint16); // QTYPE + QCLASS; > if (cur_ > io_buffer_->size()) > return false; > } > > Then InitParseWithoutQuery is: > > parser_ = DnsRecordParser(io_buffer_->data(), nbytes, > sizeof(dns_protocol::Header)); > unsigned qdcount = base::NetToHost16(header()->qdcount); > for (unsigned i = 0; i < qdcount; ++i) { > if (!parser_.SkipQuestion()) > return false; > } > return true; > Done. https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:249: continue; Made irrelevant. On 2013/04/17 22:11:12, szym wrote: > This is an error. At this point you should be done with the name, so this should > be "break". https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h File net/dns/dns_response.h (right): https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... net/dns/dns_response.h:120: bool InitParse(int nbytes); On 2013/04/17 23:30:27, szym wrote: > On 2013/04/17 22:11:12, szym wrote: > > I suggest keeping it distinct, e.g., InitParseWithoutQuery(...) since the > > semantics are substantially different from InitParse. > > > > The comment should also indicate that the parser is still positioned after the > > question section. > > After some more thinking, I suggest InitParseIgnoreQuery. > I'd also be OK if you used InitParse here and InitParseMatchQuery for the other > method. Done. https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response.h#ne... net/dns/dns_response.h:140: size_t GetResponseOffset() const; Made irrelevant. On 2013/04/17 22:11:12, szym wrote: > "Response" is confusing. (This is DnsResponse.) I suggest "answer" in reference > to "answer section". > > That said, I don't think this is needed, see comments in .cc https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... File net/dns/dns_response_unittest.cc (right): https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:376: 0x00, On 2013/04/17 22:11:12, szym wrote: > This is not a valid DNS response. The pointer is the last label. There's no > extra null-label after it. Done. https://codereview.chromium.org/14049018/diff/18001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:412: // TODO(noamsml): simplify test On 2013/04/17 22:11:12, szym wrote: > I suggest you reuse the response packets in dns_test_util.h > It makes sense to add another test for the cases of qdcount == 0 and qdcount > > 1. Done.
LGTM with nits. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:199: parser_ = DnsRecordParser(); // Make parser invalid again nit: '.' at end of sentence. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:325: Ditto. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.h File net/dns/dns_response.h (right): https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.h#ne... net/dns/dns_response.h:70: // Skip a question section "Returns true if succeeded". nit: '.' at the end of a sentence. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.h#ne... net/dns/dns_response.h:168: Remove the line. Lint might erroneously complain. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... File net/dns/dns_response_unittest.cc (right): https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:343: 0xc0, 0x18, // pointer to "chromium.org" nit: align comment with the rest. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:348: 0xc0, 0x0c, // NAME is a pointer to name in Question section. nit: align comment with the rest. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:568: Ditto.
Before submitting, please make sure to add reference to existing BUG= in the CL description.
https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:325: On 2013/04/18 19:12:34, szym wrote: > Ditto. Done. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.h File net/dns/dns_response.h (right): https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.h#ne... net/dns/dns_response.h:70: // Skip a question section On 2013/04/18 19:12:34, szym wrote: > "Returns true if succeeded". > > nit: '.' at the end of a sentence. Done. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response.h#ne... net/dns/dns_response.h:168: On 2013/04/18 19:12:34, szym wrote: > Remove the line. Lint might erroneously complain. Done. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... File net/dns/dns_response_unittest.cc (right): https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:343: 0xc0, 0x18, // pointer to "chromium.org" On 2013/04/18 19:12:34, szym wrote: > nit: align comment with the rest. Done. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:348: 0xc0, 0x0c, // NAME is a pointer to name in Question section. On 2013/04/18 19:12:34, szym wrote: > nit: align comment with the rest. Done. https://codereview.chromium.org/14049018/diff/31001/net/dns/dns_response_unit... net/dns/dns_response_unittest.cc:568: On 2013/04/18 19:12:34, szym wrote: > Ditto. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/14049018/36001
Retried try job too often on win7_aura for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/14049018/36001
Hey, I stopped this commit and added in one small change that requires extra review. The change removes buggy/unsafe behavior when parsing malformed packets. On 2013/04/22 21:08:53, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/noamsml%40chromium.org/14049018/36001
lgtm On 2013/04/23 00:04:27, Noam Samuel (chromium) wrote: > Hey, I stopped this commit and added in one small change that requires extra > review. The change removes buggy/unsafe behavior when parsing malformed packets. > > On 2013/04/22 21:08:53, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/noamsml%2540chromium.org/14049018/36001
https://codereview.chromium.org/14049018/diff/59001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/59001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:135: const char* next = cur_ + consumed + 2 * sizeof(uint16); Please, leave the comment QTYPE + QCLASS explaining what sizeof(uint16) stands for. https://codereview.chromium.org/14049018/diff/59001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:136: if (static_cast<unsigned>(next - packet_) > length_) This comparison is unsafe, it should instead be: if (next > packet_ + length)
https://codereview.chromium.org/14049018/diff/59001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/59001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:135: const char* next = cur_ + consumed + 2 * sizeof(uint16); On 2013/04/23 14:06:41, szym wrote: > Please, leave the comment QTYPE + QCLASS explaining what sizeof(uint16) stands > for. Done. https://codereview.chromium.org/14049018/diff/59001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:136: if (static_cast<unsigned>(next - packet_) > length_) On 2013/04/23 14:06:41, szym wrote: > This comparison is unsafe, it should instead be: > > if (next > packet_ + length) Done.
lgtm https://chromiumcodereview.appspot.com/14049018/diff/64001/net/dns/dns_respon... File net/dns/dns_response.cc (right): https://chromiumcodereview.appspot.com/14049018/diff/64001/net/dns/dns_respon... net/dns/dns_response.cc:135: const char* next = cur_ + consumed + 2 * sizeof(uint16); // QTYPE + QCLASS nit: two spaces before //
https://codereview.chromium.org/14049018/diff/64001/net/dns/dns_response.cc File net/dns/dns_response.cc (right): https://codereview.chromium.org/14049018/diff/64001/net/dns/dns_response.cc#n... net/dns/dns_response.cc:135: const char* next = cur_ + consumed + 2 * sizeof(uint16); // QTYPE + QCLASS On 2013/04/23 19:36:41, szym wrote: > nit: two spaces before // Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/14049018/69001
Message was sent while issue was closed.
Change committed as 195937 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
