memio_SetPeerName implicitly assumes that struct sockaddr is the same as...

memio_SetPeerName implicitly assumes that struct sockaddr is the same as PRNetAddr, but this isn't true on *BSD
patch from sprewell@jaggeri.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43179

[Evan Martin, 2010-03-30 13:49:00]

+the other nss man

LGTM

[agl, 2010-03-30 14:04:13]

LGTM

http://codereview.chromium.org/1589001/diff/6001/7001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/1589001/diff/6001/7001#newcode327
net/socket/ssl_client_socket_nss.cc:327: sockaddr_in* sockaccess = (struct
sockaddr_in *) ai->ai_addr;
reinterpret_cast<> rather than a C style cast here.
(Which won't let you lose the constness, so const at the start of the line too.)

[Peter Valchev, 2010-03-30 21:21:26]

Thanks for the quick review guys!

http://codereview.chromium.org/1589001/diff/6001/7001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/1589001/diff/6001/7001#newcode327
net/socket/ssl_client_socket_nss.cc:327: sockaddr_in* sockaccess = (struct
sockaddr_in *) ai->ai_addr;
On 2010/03/30 14:04:13, agl wrote:
> reinterpret_cast<> rather than a C style cast here.
> (Which won't let you lose the constness, so const at the start of the line
too.)

Done.

[wtc, 2010-03-30 22:31:16]

Pawel worked on this issue before.  Please do not check this
in until I've taken a closer look.

[wtc, 2010-03-30 23:04:40]

I like the approach of the contributed patch.  The problem is
that the code that copies a struct sockaddr to a PRNetAddr
does not handle struct sockaddr_in6 (IPv6).

I suggested some changes below that combine Pawel's fix and
the contributed patch.  The key to handling *BSD is to deal
with the one-byte length and one-byte address family field
at the beginning of their struct sockaddr.

http://codereview.chromium.org/1589001/diff/13001/11003
File net/base/nss_memio.c (right):

http://codereview.chromium.org/1589001/diff/13001/11003#newcode369
net/base/nss_memio.c:369: memset(&secret->peername, 0,
sizeof(secret->peername));
Lines 367-372 are Pawel's fix for this issue.

Please replace these lines by a simple structure
assignment:
  secret->peername = *peername;

and remove the "size_t peername_len" input argument.

http://codereview.chromium.org/1589001/diff/13001/11004
File net/base/nss_memio.h (right):

http://codereview.chromium.org/1589001/diff/13001/11004#newcode17
net/base/nss_memio.h:17: struct sockaddr;
Remove this forward declaration of struct sockaddr.

http://codereview.chromium.org/1589001/diff/13001/11002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/1589001/diff/13001/11002#newcode322
net/socket/ssl_client_socket_nss.cc:322: const struct addrinfo* ai =
peer_address.head();
Let's move Pawel's code here and use it on all platforms:

  PRNetAddr peername;
  memset(&peername, 0, sizeof(peername));
  DCHECK_LE(ai->ai_addrlen, sizeof(peername));
  size_t len = std::min(ai->ai_addrlen, sizeof(peername));
  memcpy(&peername, ai->ai_addr, len);

  // Adjust the address family field for BSD, whose sockaddr
  // structure has a one-byte length and one-byte address family
  // field at the beginning.  PRNetAddr has a two-byte address
  // family field at the beginning.
  peername.raw.family = ai->ai_addr->sa_family;  // Use ai->ai_family if this
doesn't work.

  memio_SetPeerName(nss_fd_, &peername);

[Peter Valchev, 2010-03-30 23:48:03]

Great, this is much cleaner. I made the changes & uploaded them (had to add a
cast for ai_addrlen as std::min does not like socklen_t), please take a look.

http://codereview.chromium.org/1589001/diff/13001/11003
File net/base/nss_memio.c (right):

http://codereview.chromium.org/1589001/diff/13001/11003#newcode369
net/base/nss_memio.c:369: memset(&secret->peername, 0,
sizeof(secret->peername));
On 2010/03/30 23:04:40, wtc wrote:
> Lines 367-372 are Pawel's fix for this issue.
> 
> Please replace these lines by a simple structure
> assignment:
>   secret->peername = *peername;
> 
> and remove the "size_t peername_len" input argument.

Done.

http://codereview.chromium.org/1589001/diff/13001/11004
File net/base/nss_memio.h (right):

http://codereview.chromium.org/1589001/diff/13001/11004#newcode17
net/base/nss_memio.h:17: struct sockaddr;
On 2010/03/30 23:04:40, wtc wrote:
> Remove this forward declaration of struct sockaddr.

Done.

http://codereview.chromium.org/1589001/diff/13001/11002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/1589001/diff/13001/11002#newcode322
net/socket/ssl_client_socket_nss.cc:322: const struct addrinfo* ai =
peer_address.head();
On 2010/03/30 23:04:40, wtc wrote:
> Let's move Pawel's code here and use it on all platforms:
> 
>   PRNetAddr peername;
>   memset(&peername, 0, sizeof(peername));
>   DCHECK_LE(ai->ai_addrlen, sizeof(peername));
>   size_t len = std::min(ai->ai_addrlen, sizeof(peername));
>   memcpy(&peername, ai->ai_addr, len);
> 
>   // Adjust the address family field for BSD, whose sockaddr
>   // structure has a one-byte length and one-byte address family
>   // field at the beginning.  PRNetAddr has a two-byte address
>   // family field at the beginning.
>   peername.raw.family = ai->ai_addr->sa_family;  // Use ai->ai_family if this
> doesn't work.
> 
>   memio_SetPeerName(nss_fd_, &peername);

Done.

[wtc, 2010-03-30 23:54:17]

LGTM.  Thanks!