ensure that UnixDomainSocket::RecvMsgWithFlags doesn't memcpy from 0

ensure that UnixDomainSocket::RecvMsgWithFlags doesn't memcpy from 0

If wire_fds is never updated after being initialized to 0, then we should not memcpy from it.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257504

[Mostyn Bramley-Moore, 2014-03-11 23:46:54]

@agl: I spotted this while testing static analyzer utils.  I guess we could set
errno to EPROTO in this case?  Or use a DCHECK instead?

[agl, 2014-03-12 15:01:55]

This is a false positive and it would be best if the static analyzer could be
fixed. If that's too hard, then I guess I don't mind too much working around it.

[Mostyn Bramley-Moore, 2014-03-12 15:38:53]

It looks to me as though a malformed msghdr could hit this, but perhaps the
source of this struct doesn't allow that to be the case?  If we had a
DCHECK(wire_fds) here it might be a little more understandable if such a problem
occurs.

This warning came from clang-analyzer, which suffers from lots of false
positives, including a use-after-DCHECK unfortunately.

[agl, 2014-03-12 15:57:53]

On 2014/03/12 15:38:53, Mostyn Bramley-Moore wrote:
> It looks to me as though a malformed msghdr could hit this, but perhaps the
> source of this struct doesn't allow that to be the case?

If wire_fds is NULL then wire_fds_len is zero. The structure could contain an
invalid length, but it comes from the kernel and the kernel doesn't need to do
that sort of thing if it wants to mess with us.

> This warning came from clang-analyzer, which suffers from lots of false
> positives, including a use-after-DCHECK unfortunately.

Does that mean that it's too hard to fix and we would prefer to workaround?

[Mostyn Bramley-Moore, 2014-03-12 16:11:02]

On 2014/03/12 15:57:53, agl wrote:
> On 2014/03/12 15:38:53, Mostyn Bramley-Moore wrote:
> > It looks to me as though a malformed msghdr could hit this, but perhaps the
> > source of this struct doesn't allow that to be the case?
> 
> If wire_fds is NULL then wire_fds_len is zero. The structure could contain an
> invalid length, but it comes from the kernel and the kernel doesn't need to do
> that sort of thing if it wants to mess with us.

Oh I see.  I guess the kernel is trustworthy.  

> > This warning came from clang-analyzer, which suffers from lots of false
> > positives, including a use-after-DCHECK unfortunately.
> 
> Does that mean that it's too hard to fix and we would prefer to workaround?

I'm not sure how difficult it would be to fix clang-analyzer, I'm not familiar
its internals.  I suspect the use-after-dcheck warnings will probably be fixed
at some point in the future, but that's just a guess.  I'm happy to drop this CL
if you want, or leave a DCHECK that might avoid the warning in a future version
of clang-analyzer.

[agl, 2014-03-12 17:05:38]

lgtm

Eh, whatever. It's not a big deal and it's nice to be static-analysis clean.

[Mostyn Bramley-Moore, 2014-03-12 17:37:59]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-03-12 17:41:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/196353002/1

[commit-bot: I haz the power, 2014-03-12 18:17:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 18:17:51]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Mostyn Bramley-Moore, 2014-03-12 22:06:52]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-03-12 22:08:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/196353002/1

[commit-bot: I haz the power, 2014-03-12 22:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 22:26:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Mostyn Bramley-Moore, 2014-03-12 22:30:13]

@ajwong: can you OK this?

[Mostyn Bramley-Moore, 2014-03-17 09:59:41]

*ping*

@willchan: perhaps you can give this a thumbs-up instead?

[awong, 2014-03-17 18:21:55]

LGTM

sorry for slow response...was at offsite most of last week.

[Mostyn Bramley-Moore, 2014-03-17 19:50:10]

> sorry for slow response...was at offsite most of last week.

No problem- thanks for the review.

[Mostyn Bramley-Moore, 2014-03-17 19:50:13]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-03-17 19:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/196353002/1

[commit-bot: I haz the power, 2014-03-17 22:00:42]

Change committed as 257504