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

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

It helps to simplify the code in three places that were doing the
ugly reinterpret_cast.

BUG=None
TEST=net_unittests --gtest_filter=DnsQuery*
R=pauljensen@chromium.org

Committed: https://crrev.com/0e534a8037d2816c4bc9349ee8a7c8403c66a88d
Cr-Commit-Position: refs/heads/master@{#367151}

[tfarina, 2015-12-22 14:24:44]

PTAL!

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

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
net/dns/dns_query.cc:26: memset(header(), 0, sizeof(dns_protocol::Header));
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.

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode32
net/dns/dns_query.cc:32: base::BigEndianWriter
writer(reinterpret_cast<char*>(header() + 1),
I'd much rather all this was rewritten in a more type-safe manner, say perhaps
using a "flexible array member".

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.h
File net/dns/dns_query.h (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.h#newcode55
net/dns/dns_query.h:55: dns_protocol::Header* header();
I'm not really a fan of making something hacky (the reinterpret_cast) easier to
use.

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

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
net/dns/dns_query.cc:26: memset(header(), 0, sizeof(dns_protocol::Header));
On 2015/12/29 12:38:02, pauljensen wrote:
> 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.
I don't see how this matters much. I don't think changing to sizeof(*var)
belongs to this change.

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode32
net/dns/dns_query.cc:32: base::BigEndianWriter
writer(reinterpret_cast<char*>(header() + 1),
On 2015/12/29 12:38:02, pauljensen wrote:
> I'd much rather all this was rewritten in a more type-safe manner, say perhaps
> using a "flexible array member".

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

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.h
File net/dns/dns_query.h (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.h#newcode55
net/dns/dns_query.h:55: dns_protocol::Header* header();
On 2015/12/29 12:38:02, pauljensen wrote:
> I'm not really a fan of making something hacky (the reinterpret_cast) easier
to
> use.

dns_response.h already has an accessor like this. I don't consider something
from the C++ cast-system like reinterpret_cast a hack.

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

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
net/dns/dns_query.cc:26: memset(header(), 0, sizeof(dns_protocol::Header));
On 2015/12/29 13:57:43, tfarina wrote:
> On 2015/12/29 12:38:02, pauljensen wrote:
> > 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.
> I don't see how this matters much. I don't think changing to sizeof(*var)
> belongs to this change.

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

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode32
net/dns/dns_query.cc:32: base::BigEndianWriter
writer(reinterpret_cast<char*>(header() + 1),
On 2015/12/29 13:57:43, tfarina wrote:
> On 2015/12/29 12:38:02, pauljensen wrote:
> > I'd much rather all this was rewritten in a more type-safe manner, say
perhaps
> > using a "flexible array member".
> 
> Maybe? I don't think this belongs to this change.

Acknowledged.

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.h
File net/dns/dns_query.h (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.h#newcode55
net/dns/dns_query.h:55: dns_protocol::Header* header();
On 2015/12/29 13:57:43, tfarina wrote:
> On 2015/12/29 12:38:02, pauljensen wrote:
> > I'm not really a fan of making something hacky (the reinterpret_cast) easier
> to
> > use.
> 
> dns_response.h already has an accessor like this. I don't consider something
> from the C++ cast-system like reinterpret_cast a hack.

Casting something to a different type and dereferencing is a hack in my book. 
It violates C aliasing rules and optimizing compiler are apt to reinterpret the
code in unforseen ways.

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

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
net/dns/dns_query.cc:26: memset(header(), 0, sizeof(dns_protocol::Header));
On 2015/12/29 15:06:22, pauljensen wrote:
> On 2015/12/29 13:57:43, tfarina wrote:
> > On 2015/12/29 12:38:02, pauljensen wrote:
> > > 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.
> > I don't see how this matters much. I don't think changing to sizeof(*var)
> > belongs to this change.
> 
> You're making this code more fragile and susceptible to introduction of
> inadvertent memory trashing.  Please change to this type-safe code:
>   *header() = {};
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:38]

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
net/dns/dns_query.cc:26: memset(header(), 0, sizeof(dns_protocol::Header));
On 2015/12/29 18:48:01, tfarina wrote:
> On 2015/12/29 15:06:22, pauljensen wrote:
> > On 2015/12/29 13:57:43, tfarina wrote:
> > > On 2015/12/29 12:38:02, pauljensen wrote:
> > > > 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.
> > > I don't see how this matters much. I don't think changing to sizeof(*var)
> > > belongs to this change.
> > 
> > You're making this code more fragile and susceptible to introduction of
> > inadvertent memory trashing.  Please change to this type-safe code:
> >   *header() = {};
> 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.

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]

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
net/dns/dns_query.cc:26: memset(header(), 0, sizeof(dns_protocol::Header));
On 2015/12/29 19:42:37, pauljensen wrote:
> On 2015/12/29 18:48:01, tfarina wrote:
> > On 2015/12/29 15:06:22, pauljensen wrote:
> > > On 2015/12/29 13:57:43, tfarina wrote:
> > > > On 2015/12/29 12:38:02, pauljensen wrote:
> > > > > 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.
> > > > I don't see how this matters much. I don't think changing to
sizeof(*var)
> > > > belongs to this change.
> > > 
> > > You're making this code more fragile and susceptible to introduction of
> > > inadvertent memory trashing.  Please change to this type-safe code:
> > >   *header() = {};
> > 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.
> 
> 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.
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.

[mmenke, 2015-12-29 22:17:25]

On 2015/12/29 19:57:07, tfarina wrote:
> https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
> File net/dns/dns_query.cc (right):
> 
>
https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
> net/dns/dns_query.cc:26: memset(header(), 0, sizeof(dns_protocol::Header));
> On 2015/12/29 19:42:37, pauljensen wrote:
> > On 2015/12/29 18:48:01, tfarina wrote:
> > > On 2015/12/29 15:06:22, pauljensen wrote:
> > > > On 2015/12/29 13:57:43, tfarina wrote:
> > > > > On 2015/12/29 12:38:02, pauljensen wrote:
> > > > > > 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.
> > > > > I don't see how this matters much. I don't think changing to
> sizeof(*var)
> > > > > belongs to this change.
> > > > 
> > > > You're making this code more fragile and susceptible to introduction of
> > > > inadvertent memory trashing.  Please change to this type-safe code:
> > > >   *header() = {};
> > > 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.
> > 
> > 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.
> Sorry, someone changing one but not the other? How could someone make such
> change and get unnoticed in the review process?

Erm...  Plenty of similarly obvious bugs make it through the codereview process.
 I've seen plenty of cases where the wrong naming style is used for variables on
the stack, for instance, which is about as obvious a problem as you can get.

> 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.

[szym, 2015-12-29 22:34:14]

Agreed, and it's possible a mistake in the memset would pass all tests.

On Tue, Dec 29, 2015 at 2:17 PM, <mmenke@chromium.org> wrote:

> On 2015/12/29 19:57:07, tfarina wrote:
>
>> https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc
>> File net/dns/dns_query.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1546623002/diff/1/net/dns/dns_query.cc#newcode26
>
>> net/dns/dns_query.cc:26: memset(header(), 0,
>> sizeof(dns_protocol::Header));
>> On 2015/12/29 19:42:37, pauljensen wrote:
>> > On 2015/12/29 18:48:01, tfarina wrote:
>> > > On 2015/12/29 15:06:22, pauljensen wrote:
>> > > > On 2015/12/29 13:57:43, tfarina wrote:
>> > > > > On 2015/12/29 12:38:02, pauljensen wrote:
>> > > > > > 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.
>> > > > > I don't see how this matters much. I don't think changing to
>> sizeof(*var)
>> > > > > belongs to this change.
>> > > >
>> > > > You're making this code more fragile and susceptible to
>> introduction of
>> > > > inadvertent memory trashing.  Please change to this type-safe code:
>> > > >   *header() = {};
>> > > 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.
>> >
>> > 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.
>> Sorry, someone changing one but not the other? How could someone make such
>> change and get unnoticed in the review process?
>>
>
> Erm...  Plenty of similarly obvious bugs make it through the codereview
> process.
>  I've seen plenty of cases where the wrong naming style is used for
> variables on
> the stack, for instance, which is about as obvious a problem as you can
> get.
>
>
> 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.
>>
>
>
>
> https://codereview.chromium.org/1546623002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2015-12-29 22:41:53]

PTAL!

[pauljensen, 2015-12-30 03:35:21]

On 2015/12/29 22:34:14, szym wrote:
> Agreed, and it's possible a mistake in the memset would pass all tests.

SZYMON LIVES!

[pauljensen, 2015-12-30 03:41:08]

On 2015/12/29 22:41:53, tfarina wrote:
> PTAL!

lgtm.  I'm glad we got rid of one memset() and it's nice that all the
reinterpret_casts are combined into one.  Down the road it'd still be nice to
rewrite into a form with more type safety, like just one pointer with type like:
struct {
  dns_protocol::Header header;
  char question[];
}

[tfarina, 2015-12-30 13:29:30]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2015-12-30 13:29:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1546623002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1546623002/20001

[commit-bot: I haz the power, 2015-12-30 13:34:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-30 13:35:21]

Description was changed from

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

It helps to simplify the code in three places that were doing the
ugly reinterpret_cast.

BUG=None
TEST=net_unittests --gtest_filter=DnsQuery*
R=pauljensen@chromium.org
==========

to

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

It helps to simplify the code in three places that were doing the
ugly reinterpret_cast.

BUG=None
TEST=net_unittests --gtest_filter=DnsQuery*
R=pauljensen@chromium.org

Committed: https://crrev.com/0e534a8037d2816c4bc9349ee8a7c8403c66a88d
Cr-Commit-Position: refs/heads/master@{#367151}
==========

[commit-bot: I haz the power, 2015-12-30 13:35:22]

Patchset 2 (id:??) landed as
https://crrev.com/0e534a8037d2816c4bc9349ee8a7c8403c66a88d
Cr-Commit-Position: refs/heads/master@{#367151}