net: add a convenience getter accessor for dns Header to DnsQuery

net/dns/dns_query.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/dns/dns_query.h"
 
-#include <limits>
-
 #include "base/big_endian.h"
 #include "base/sys_byteorder.h"
 #include "net/base/io_buffer.h"
 #include "net/dns/dns_protocol.h"
 #include "net/dns/dns_util.h"
 
 namespace net {
 
 // DNS query consists of a 12-byte header followed by a question section.
 // For details, see RFC 1035 section 4.1.1.  This header template sets RD
 // bit, which directs the name server to pursue query recursively, and sets
 // the QDCOUNT to 1, meaning the question section has a single entry.
 DnsQuery::DnsQuery(uint16_t id, const base::StringPiece& qname, uint16_t qtype)
     : qname_size_(qname.size()) {
   DCHECK(!DNSDomainToString(qname).empty());
   // QNAME + QTYPE + QCLASS
   size_t question_size = qname_size_ + sizeof(uint16_t) + sizeof(uint16_t);
   io_buffer_ = new IOBufferWithSize(sizeof(dns_protocol::Header) +
                                     question_size);
-  dns_protocol::Header* header =
-      reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
-  memset(header, 0, sizeof(dns_protocol::Header));
-  header->id = base::HostToNet16(id);
-  header->flags = base::HostToNet16(dns_protocol::kFlagRD);
-  header->qdcount = base::HostToNet16(1);
+  memset(header(), 0, sizeof(dns_protocol::Header));

[pauljensen, 2015/12/29 12:38:02]

I don't like how the sizeof() uses a length different from the first argument
(arg1).  sizeof(*arg1) is generally much safer.  With your change the type of
arg1 is further away (line 81) from the sizeof() (line 26) than it was
previously.

[tfarina, 2015/12/29 13:57:43]

I don't see how this matters much. I don't think changing to sizeof(*var)
belongs to this change.

[pauljensen, 2015/12/29 15:06:22]

You're making this code more fragile and susceptible to introduction of
inadvertent memory trashing.  Please change to this type-safe code:
  *header() = {};

[tfarina, 2015/12/29 18:48:01]

Sorry, I'm not getting your point Paul. The code has been like this since 2011.
I'm not changing it. I'm just calling the header() function to get the pointer
to Header. Which should be the same as before, no? I might be missing something
from your type-safety comments in the existing code. If you want to make this
code more safe then it should be done separate from the changes I'm making here
IMO. I don't like mixing two kinds of change in patch.

[pauljensen, 2015/12/29 19:42:37]

I dislike memset() because it's very type-unsafe.  A type indicates how some
data can be accessed including how big it is.  With memset() you pass a pointer
and a size separately.  If either doesn't match the other you get nasty memory
corruption or uninitialized problems.
Prior to your change the code was essentially:
  T* x = blah;
  memset(x, 0, sizeof(T))
So x's declared type was on the line right before memset(), where we use
sizeof(T).
With your change the declared type of header() is many lines away from the
sizeof(T) in the call to memset(), thereby increasing the chance of someone
changing one but not the other, and running into nasty problems.
I'd like to rectify your CL so it's not exacerbating this problem.

[tfarina, 2015/12/29 19:57:07]

Sorry, someone changing one but not the other? How could someone make such
change and get unnoticed in the review process?

OK, I will shut up here, and making the change to the "C++ safe way" of doing
things. Which looks like uses the C++11 initializer syntax.

+  header()->id = base::HostToNet16(id);
+  header()->flags = base::HostToNet16(dns_protocol::kFlagRD);
+  header()->qdcount = base::HostToNet16(1);
 
   // Write question section after the header.
-  base::BigEndianWriter writer(reinterpret_cast<char*>(header + 1),
+  base::BigEndianWriter writer(reinterpret_cast<char*>(header() + 1),

[pauljensen, 2015/12/29 12:38:02]

I'd much rather all this was rewritten in a more type-safe manner, say perhaps
using a "flexible array member".

[tfarina, 2015/12/29 13:57:43]

Maybe? I don't think this belongs to this change.

[pauljensen, 2015/12/29 15:06:22]

Acknowledged.

                                question_size);
   writer.WriteBytes(qname.data(), qname.size());
   writer.WriteU16(qtype);
   writer.WriteU16(dns_protocol::kClassIN);
 }
 
 DnsQuery::~DnsQuery() {
 }
 
 scoped_ptr<DnsQuery> DnsQuery::CloneWithNewId(uint16_t id) const {
@@ skipping 16 matching lines @@
   base::ReadBigEndian<uint16_t>(
       io_buffer_->data() + sizeof(dns_protocol::Header) + qname_size_, &type);
   return type;
 }
 
 base::StringPiece DnsQuery::question() const {
   return base::StringPiece(io_buffer_->data() + sizeof(dns_protocol::Header),
                            qname_size_ + sizeof(uint16_t) + sizeof(uint16_t));
 }
 
+void DnsQuery::set_flags(uint16_t flags) {
+  header()->flags = flags;
+}
+
 DnsQuery::DnsQuery(const DnsQuery& orig, uint16_t id) {
   qname_size_ = orig.qname_size_;
   io_buffer_ = new IOBufferWithSize(orig.io_buffer()->size());
   memcpy(io_buffer_.get()->data(), orig.io_buffer()->data(),
          io_buffer_.get()->size());
-  dns_protocol::Header* header =
-      reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
-  header->id = base::HostToNet16(id);
+  header()->id = base::HostToNet16(id);
 }
 
-void DnsQuery::set_flags(uint16_t flags) {
-  dns_protocol::Header* header =
-      reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
-  header->flags = flags;
+dns_protocol::Header* DnsQuery::header() {
+  return reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
 }
 
 }  // namespace net