nacl_io: only copy up to the size of the sockaddr_in*, not the given len.

nacl_io: only copy up to the size of the sockaddr_in*, not the given len.

One of the tests (SocketTestTCP, Listen) submits a len
"addrlen = sizeof(addr) + 10", so that causes the memcpy
to overwrite part of the caller's stack (because
the outparam points at the stack).

Related to:
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3867
(last time the compile change caused the test to fail)

NOTRY=true
(only an SDK change)

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

[jvoung (off chromium), 2014-06-03 19:24:23]

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc (right):

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc:160: len =
sizeof(addr4);
Hmm, not sure of the point of the len argument then?

[Sam Clegg, 2014-06-03 19:32:43]

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc (right):

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc:160: len =
sizeof(addr4);
On 2014/06/03 19:24:23, jvoung wrote:
> Hmm, not sure of the point of the len argument then?

I think we should mimik the behavior of getsockname here.

" The returned address is truncated if the buffer provided  is  too  small;  in 
this  case, addrlen will return a value greater than was supplied to the call."

So it sounds like we should always return sizeof(addr6), memcpy
MIN(sizeof(addr6), len) bytes.  We should have tests for this too,
although its not really your responsibility to add them in this
CL I guess.

[binji, 2014-06-03 20:10:05]

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc (right):

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc:160: len =
sizeof(addr4);
On 2014/06/03 19:32:43, Sam Clegg wrote:
> On 2014/06/03 19:24:23, jvoung wrote:
> > Hmm, not sure of the point of the len argument then?
> 
> I think we should mimik the behavior of getsockname here.
> 
> " The returned address is truncated if the buffer provided  is  too  small; 
in 
> this  case, addrlen will return a value greater than was supplied to the
call."
> 
> So it sounds like we should always return sizeof(addr6), memcpy
> MIN(sizeof(addr6), len) bytes.  We should have tests for this too,
> although its not really your responsibility to add them in this
> CL I guess.

+1, the opengroup spec says the same thing.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/getsockname.html

[Sam Clegg, 2014-06-03 20:11:21]

On 2014/06/03 19:32:43, Sam Clegg wrote:
>
https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
> File native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc (right):
> 
>
https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
> native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc:160: len =
> sizeof(addr4);
> On 2014/06/03 19:24:23, jvoung wrote:
> > Hmm, not sure of the point of the len argument then?
> 
> I think we should mimik the behavior of getsockname here.
> 
> " The returned address is truncated if the buffer provided  is  too  small; 
in 
> this  case, addrlen will return a value greater than was supplied to the
call."
> 
> So it sounds like we should always return sizeof(addr6), memcpy
> MIN(sizeof(addr6), len) bytes.  We should have tests for this too,
> although its not really your responsibility to add them in this
> CL I guess.

I'd be ok with a TODO(sbc) to get some tests written if you don't feel like
writing them.

[jvoung (off chromium), 2014-06-03 21:59:32]

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
File native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc (right):

https://codereview.chromium.org/310903002/diff/1/native_client_sdk/src/librar...
native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc:160: len =
sizeof(addr4);
On 2014/06/03 20:10:05, binji wrote:
> On 2014/06/03 19:32:43, Sam Clegg wrote:
> > On 2014/06/03 19:24:23, jvoung wrote:
> > > Hmm, not sure of the point of the len argument then?
> > 
> > I think we should mimik the behavior of getsockname here.
> > 
> > " The returned address is truncated if the buffer provided  is  too  small; 
> in 
> > this  case, addrlen will return a value greater than was supplied to the
> call."
> > 
> > So it sounds like we should always return sizeof(addr6), memcpy
> > MIN(sizeof(addr6), len) bytes.  We should have tests for this too,
> > although its not really your responsibility to add them in this
> > CL I guess.
> 
> +1, the opengroup spec says the same thing.
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getsockname.html

Done.

[binji, 2014-06-04 00:23:34]

lgtm

[jvoung (off chromium), 2014-06-04 00:42:25]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2014-06-04 00:42:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jvoung@chromium.org/310903002/60001

[commit-bot: I haz the power, 2014-06-04 00:44:50]

Change committed as 274689

[Sam Clegg, 2014-06-04 00:45:28]

https://codereview.chromium.org/310903002/diff/60001/native_client_sdk/src/te...
File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right):

https://codereview.chromium.org/310903002/diff/60001/native_client_sdk/src/te...
native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:655:
ASSERT_EQ(client_addr2.sin_addr.s_addr, 0);
This seems like it should be its own test really...  I can move it out in
another CL if the CQ is gonna land this version.