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

net/dns/dns_query.cc

Index: net/dns/dns_query.cc
diff --git a/net/dns/dns_query.cc b/net/dns/dns_query.cc
index 0966def976d8b0c3189fc116561f1df034e871b7..c47aeafb8bc9c1fe8aba8f1ddc5a26b5b49cce45 100644
--- a/net/dns/dns_query.cc
+++ b/net/dns/dns_query.cc
@@ -4,8 +4,6 @@
 
 #include "net/dns/dns_query.h"
 
-#include <limits>
-
 #include "base/big_endian.h"
 #include "base/sys_byteorder.h"
 #include "net/base/io_buffer.h"
@@ -25,15 +23,13 @@ DnsQuery::DnsQuery(uint16_t id, const base::StringPiece& qname, uint16_t qtype)
   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);
@@ -70,20 +66,20 @@ base::StringPiece DnsQuery::question() const {
                            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