Fix one uninitialized and one incorrectly initialized arguments in sendmsg sy...

Fix one uninitialized and one incorrectly initialized arguments in sendmsg syscall.
Found with Valgrind.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=775

Committed: http://code.google.com/p/nativeclient/source/detail?r=2916

[Mark Seaborn, 2010-08-03 10:22:14]

http://codereview.chromium.org/3026044/diff/1/2
File src/trusted/desc/posix/nacl_desc_conn_cap.c (right):

http://codereview.chromium.org/3026044/diff/1/2#newcode103
src/trusted/desc/posix/nacl_desc_conn_cap.c:103: connect_msg.msg_flags = 0;
Good.  Thanks for catching that.

http://codereview.chromium.org/3026044/diff/1/2#newcode112
src/trusted/desc/posix/nacl_desc_conn_cap.c:112: connect_msg.msg_controllen =
cmsg->cmsg_len;

You're now assigning to connect_msg.msg_controllen twice.

I don't think this is right.  The cmsg man page says:

"Finally, the msg_controllen field of the msghdr should be set to the
sum of the CMSG_SPACE() of the length of all control messages in the
buffer."

You're now using CMSG_LEN() instead.

In practice, on Linux CMSG_SPACE and CMSG_LEN seem to give the same
result, so I don't see why this would make a difference.

[Evgeniy Stepanov, 2010-08-03 10:42:40]

http://codereview.chromium.org/3026044/diff/1/2
File src/trusted/desc/posix/nacl_desc_conn_cap.c (right):

http://codereview.chromium.org/3026044/diff/1/2#newcode112
src/trusted/desc/posix/nacl_desc_conn_cap.c:112: connect_msg.msg_controllen =
cmsg->cmsg_len;
On 2010/08/03 10:22:15, Mark Seaborn wrote:
> 
> You're now assigning to connect_msg.msg_controllen twice.
> 
> I don't think this is right.  The cmsg man page says:
> 
> "Finally, the msg_controllen field of the msghdr should be set to the
> sum of the CMSG_SPACE() of the length of all control messages in the
> buffer."
> 
> You're now using CMSG_LEN() instead.
> 
> In practice, on Linux CMSG_SPACE and CMSG_LEN seem to give the same
> result, so I don't see why this would make a difference.

In this case, they are different - one is 24 and another is 20 bytes. The man
page seems to contradict itself, first saying that msg_controllen should be the
sum of CMSG_SPACE() and then, in the code snippet, assigning cmsg->cmsg_len to
it.

RFC 2292 has a longer code snippet with 2 ancillary data objects, where they set
msg_controllen to the sum of CMSG_SPACE of the first object and CMSG_LEN of the
second one. This seems correct - there is no need to send the padding of the
last object over network.

[Mark Seaborn, 2010-08-04 13:46:24]

On 2010/08/03 10:42:40, Evgeniy Stepanov wrote:
> On 2010/08/03 10:22:15, Mark Seaborn wrote:
> > In practice, on Linux CMSG_SPACE and CMSG_LEN seem to give the same
> > result, so I don't see why this would make a difference.

> In this case, they are different - one is 24 and another is 20
> bytes. The man page seems to contradict itself, first saying that
> msg_controllen should be the sum of CMSG_SPACE() and then, in the
> code snippet, assigning cmsg->cmsg_len to it.

OK.  The difference on x86-64 is just padding then...

> RFC 2292 has a longer code snippet with 2 ancillary data objects,
> where they set msg_controllen to the sum of CMSG_SPACE of the first
> object and CMSG_LEN of the second one. This seems correct - there is
> no need to send the padding of the last object over network.

In this case the data is not sent over the network.  The FD number
used by the kernel to look up the FD in the FD table.

If you do have 2 ancillary data objects and use CMSG_SPACE(first) +
CMSG_LEN(second), valgrind is presumably going to complain about the
padding bytes in the middle being uninitialised.  So this is kind of a
bug in valgrind -- it doesn't know enough about the buffer format.
However, working around the valgrind limitation is fine with me.

Just remove the first msg_controllen assignment and this will LGTM.

Cheers,
Mark

http://codereview.chromium.org/3026044/diff/1/2
File src/trusted/desc/posix/nacl_desc_conn_cap.c (right):

http://codereview.chromium.org/3026044/diff/1/2#newcode102
src/trusted/desc/posix/nacl_desc_conn_cap.c:102: connect_msg.msg_controllen =
sizeof(control_buf);
Please remove this assignment to msg_controllen since it's redundant now.

[Mark Seaborn, 2010-08-04 13:47:21]

In description:
> BUG=775

BTW, please use the full URL otherwise codereview.chromium.org links
to the wrong bug (a Chromium bug).

[Evgeniy Stepanov, 2010-08-04 14:10:40]

On 2010/08/04 13:46:24, Mark Seaborn wrote:
> On 2010/08/03 10:42:40, Evgeniy Stepanov wrote:
> > On 2010/08/03 10:22:15, Mark Seaborn wrote:
> > > In practice, on Linux CMSG_SPACE and CMSG_LEN seem to give the same
> > > result, so I don't see why this would make a difference.
> 
> > In this case, they are different - one is 24 and another is 20
> > bytes. The man page seems to contradict itself, first saying that
> > msg_controllen should be the sum of CMSG_SPACE() and then, in the
> > code snippet, assigning cmsg->cmsg_len to it.
> 
> OK.  The difference on x86-64 is just padding then...
> 
> > RFC 2292 has a longer code snippet with 2 ancillary data objects,
> > where they set msg_controllen to the sum of CMSG_SPACE of the first
> > object and CMSG_LEN of the second one. This seems correct - there is
> > no need to send the padding of the last object over network.
> 
> In this case the data is not sent over the network.  The FD number
> used by the kernel to look up the FD in the FD table.
> 
> If you do have 2 ancillary data objects and use CMSG_SPACE(first) +
> CMSG_LEN(second), valgrind is presumably going to complain about the
> padding bytes in the middle being uninitialised.  So this is kind of a
> bug in valgrind -- it doesn't know enough about the buffer format.
> However, working around the valgrind limitation is fine with me.

At least on Linux you can not initialize a multiple object buffer without
zeroing it first. The procedure recommended in the man page (CMSG_FIRSTHDR, then
CMSG_NXTHDR) will fail because CMSG_NXTHDR reads whatever is in the place of the
second object and checks that it's length is fully within the buffer.

So, yes, it would be right for Valgrind to ignore uninitialized padding bytes,
but really it is just a case of inadequate documentation :(

> Just remove the first msg_controllen assignment and this will LGTM.

No, this is exactly what man page suggests - first set it to the available
buffer space, then change it to the actual used bytes. Not sure about
CMSG_FIRSTHDR, but CMSG_NXTHDR definitely reads the current buffer length and
interprets it this way.

> Cheers,
> Mark
> 
> http://codereview.chromium.org/3026044/diff/1/2
> File src/trusted/desc/posix/nacl_desc_conn_cap.c (right):
> 
> http://codereview.chromium.org/3026044/diff/1/2#newcode102
> src/trusted/desc/posix/nacl_desc_conn_cap.c:102: connect_msg.msg_controllen =
> sizeof(control_buf);
> Please remove this assignment to msg_controllen since it's redundant now.

[Mark Seaborn, 2010-08-05 10:09:08]

On 2010/08/04 14:10:40, Evgeniy Stepanov wrote:
> > Just remove the first msg_controllen assignment and this will LGTM.
> 
> No, this is exactly what man page suggests - first set it to the
> available buffer space, then change it to the actual used bytes. Not
> sure about CMSG_FIRSTHDR, but CMSG_NXTHDR definitely reads the
> current buffer length and interprets it this way.

I think CMSG_FIRSTHDR doesn't read msg_controllen, but OK.

LGTM and sorry if I was overly picky!

Can you add a comment for why msg_controllen is assigned twice, and a
comment to say that we use CMSG_LEN to avoid a valgrind warning?

Thanks for investigating - I have also been seeing this warning when
running valgrind on fake_browser_test.

Mark

[Evgeniy Stepanov, 2010-08-05 11:05:10]

On 2010/08/05 10:09:08, Mark Seaborn wrote:
> On 2010/08/04 14:10:40, Evgeniy Stepanov wrote:
> > > Just remove the first msg_controllen assignment and this will LGTM.
> > 
> > No, this is exactly what man page suggests - first set it to the
> > available buffer space, then change it to the actual used bytes. Not
> > sure about CMSG_FIRSTHDR, but CMSG_NXTHDR definitely reads the
> > current buffer length and interprets it this way.
> 
> I think CMSG_FIRSTHDR doesn't read msg_controllen, but OK.
> 
> LGTM and sorry if I was overly picky!

Not at all. Thanks for reviewing this!

> Can you add a comment for why msg_controllen is assigned twice, and a
> comment to say that we use CMSG_LEN to avoid a valgrind warning?

Done.

> Thanks for investigating - I have also been seeing this warning when
> running valgrind on fake_browser_test.
> 
> Mark