Really connect to the same server in FTP network transaction.

Really connect to the same server in FTP network transaction.

Also create necessary infrastructure to know the address
a client socket is connected to.

TEST=Covered by net_unittests.
BUG=35670
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39559

[eroman, 2010-02-11 21:11:58]

It seems redundant to have both:
  ClientSocket::GetPeerName()    [which gets a sockaddr]
  ClientSocket::GetPeerAddress()  [which gets an addrinfo]

I think it would be sufficient to replace GetPeerName() by GetPeerAddress(), and
now we can express GetPeerName() in terms of GetPeerAddress():

pseudo-code:

AddressList address_list;
GetPeerAddress(&address_list);
struct addrinfo* ai = address_list.head();

// Extract sockaddr
*sock_addr = ai->ai_addr;
*sock_addrlen = ai->ai_addr_len;


Otherwise, LGTM

http://codereview.chromium.org/598071/diff/1/4
File net/base/address_list.cc (right):

http://codereview.chromium.org/598071/diff/1/4#newcode16
net/base/address_list.cc:16: // Make a deep copy of |info|. This copy should be
deleted using
nit: This comment needs to be updated (only a "deep" copy when recursive=true
now).

http://codereview.chromium.org/598071/diff/1/6
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/598071/diff/1/6#newcode53
net/base/address_list_unittest.cc:53: TEST(AddressListTest, Copy) {
Can you add a test case that uses non-recursive copy?

Although it is simple, it is good to exercise the code from the unittests, since
that way valgrind's memcheck can make sure memory isn't being leaked.

http://codereview.chromium.org/598071/diff/1/9
File net/ftp/ftp_network_transaction_unittest.cc (right):

http://codereview.chromium.org/598071/diff/1/9#newcode817
net/ftp/ftp_network_transaction_unittest.cc:817: while (addrinfo) {
why is this a loop... can you unwind it so the test only accepts 1 result?

[eroman, 2010-02-11 21:20:29]

(Er, I should have written that pseudo-code differently. Once AddressList goes
out of scope, the sockaddr will be deleted. But the general idea is the same).

[Paweł Hajdan Jr., 2010-02-12 17:09:21]

PTAL, thanks for the pseudocode. I'm somewhat uncomfortable with these
reinterpret_casts. Please review especially carefully.

[eroman, 2010-02-12 21:49:33]

Thanks for making this change!

LGTM after you see my comment in
<http://codereview.chromium.org/598071/diff/5001/6001#newcode258>.

Also, can you file a separate bug for this?

It is in fact a separate issue from 32945, and I was mistaken earlier in the
review when I lumped them together.

(32945 is about supporting EPSV, whereas this one is about connecting to the
same server in data connection).

I appreciate you making the change, and I think it is great to address both of
these!

(For 32945 I am cool with us committing wtc's earlier patch to disable IPv6 as a
workaround).

http://codereview.chromium.org/598071/diff/5001/6001
File chrome/browser/sync/notifier/communicator/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/598071/diff/5001/6001#newcode258
chrome/browser/sync/notifier/communicator/ssl_socket_adapter.cc:258:
NOTIMPLEMENTED();
I wander if this code is needed to avoid regression?

Hmm interesting, the old code doesn't look quite right (it is not setting
|namelen| for example, and libjingle is only supporting IPv4 addresses).

I recommend implementing this. We can do so by artificially augmenting the
sockaddr_in (IPv4 address) to an addrinfo for TCP socket. Here is how to do
that:

talk_base::SocketAddress address = socket_->GetRemoteAddress();

sockaddr_in ipv4addr;
address.ToSockAddr(&ipv4addr);

struct addrinfo ai;
memset(&ai, sizeof(ai), 0);
ai->ai_family = ipv4addr->sin_family;
ai->ai_socktype = SOCK_STREAM;
ai->ai_protocol = IPPROTO_TCP;
ai->ai_addr = reinterpret_cast<struct sockaddr*>(&ipv4addr);
ai->ai_addrlen = sizeof(ipv4addr);

// Copy.
address->Copy(ai, false);

http://codereview.chromium.org/598071/diff/5001/6004
File net/base/address_list.h (right):

http://codereview.chromium.org/598071/diff/5001/6004#newcode30
net/base/address_list.h:30: // Appends a copy of |head| and all its linked
addrinfos to rhe stored
typo: "rhe"

http://codereview.chromium.org/598071/diff/5001/6005
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/598071/diff/5001/6005#newcode84
net/base/address_list_unittest.cc:84: TEST(AddressListTest, CopyNonRecursive) {
Excellent, thanks for the new tests!

http://codereview.chromium.org/598071/diff/5001/6010
File net/socket/client_socket.h (left):

http://codereview.chromium.org/598071/diff/5001/6010#oldcode10
net/socket/client_socket.h:10: // For struct sockaddr and socklen_t.
Nice remove.

http://codereview.chromium.org/598071/diff/5001/6010
File net/socket/client_socket.h (right):

http://codereview.chromium.org/598071/diff/5001/6010#newcode51
net/socket/client_socket.h:51: // Puts the peer address in |address| and returns
a network error code.
nit on the wording: I suggest "Copies the peer address to |address| ..."

http://codereview.chromium.org/598071/diff/5001/6018
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/598071/diff/5001/6018#newcode735
net/socket/ssl_client_socket_mac.cc:735: return rv;
Is this behavior different?

It looks like before it would fail silently, whereas now we fail explicitly.

I think the current behavior is best, but I don't know this code well enough to
be sure.

http://codereview.chromium.org/598071/diff/5001/6020
File net/socket/ssl_client_socket_nss.cc (left):

http://codereview.chromium.org/598071/diff/5001/6020#oldcode322
net/socket/ssl_client_socket_nss.cc:322: return ERR_UNEXPECTED;
wohoo, another ERR_UNEXPECTED is removed!

http://codereview.chromium.org/598071/diff/5001/6028
File net/socket/tcp_client_socket_win.h (right):

http://codereview.chromium.org/598071/diff/5001/6028#newcode8
net/socket/tcp_client_socket_win.h:8: #include <ws2tcpip.h>
Is this header necessary here?

[Paweł Hajdan Jr., 2010-02-13 18:26:38]

Patch updated.

wtc/willchan: please take a look

[Paweł Hajdan Jr., 2010-02-16 20:35:19]

Wan-Teh, could you advice about changes in nss_socket_nss.cc? Currently it
causes an invalid read, see
http://build.chromium.org/buildbot/try-server/builders/linux_valgrind/builds/...

The general issue is that I don't know how to properly convert AddressList to
PRNetAddr. PRNetAddr seems to be very similar to sockaddr, but it seems to be a
bit different.

[wtc, 2010-02-17 02:00:13]

Pawel,

Please fix the following code:

void memio_SetPeerName(PRFileDesc *fd, const PRNetAddr *peername)
{
    PRFileDesc *memiofd = PR_GetIdentitiesLayer(fd, memio_identity);
    struct PRFilePrivate *secret =  memiofd->secret;
    secret->peername = *peername;
}

The third line in the function:
    secret->peername = *peername;
which does a structure copy, is the problem.

PRNetAddr is like struct sockaddr_storage.  It is larger than
struct sockaddr_in and struct sockaddr_in6, the actual sockaddr.

So we can't do a structure copy of PRNetAddr.  Instead, we
need to copy the actual sockaddr (which can be struct sockaddr_in
or struct sockaddr_in6).  I suggest that you change the function
prototype like this:

void memio_SetPeerName(PRFileDesc *fd,
                       const struct sockaddr *peername,
                       socklen_t peerlen)
{
    PRFileDesc *memiofd = PR_GetIdentitiesLayer(fd, memio_identity);
    struct PRFilePrivate *secret =  memiofd->secret;
    memset(&secret->peername, 0, sizeof(secret->peername));
    memcpy(&secret->peername, peername, peerlen);
}

[wtc, 2010-02-17 02:03:11]

Pawel: you can also use size_t instead of socklen_t as the type
of the peerlen argument.  The peerlen argument probably should be
renamed peername_len.

[Paweł Hajdan Jr., 2010-02-17 08:25:42]

Thanks for very good description how to fix it. Valgrind shows it's ok now.
Please take a look.

[wtc, 2010-02-17 20:18:17]

eroman: are you going to review this CL?

Pawel: I only reviewed a few files in this CL.  If eroman or
willchan is not going to review this CL, I'll review the CL
in its entirety.

http://codereview.chromium.org/598071/diff/3007/8004
File net/base/address_list.cc (right):

http://codereview.chromium.org/598071/diff/3007/8004#newcode16
net/base/address_list.cc:16: // Make a copy of |info| (the dynamically-allocated
parts are copied as well).
Nit: you can still call this a "deep" copy because the
dynamically allocated parts are copied as well.  But I guess
"deep" could be confused with "recursive"...

http://codereview.chromium.org/598071/diff/3007/8004#newcode109
net/base/address_list.cc:109: // system-created data (because we don't know
whether |head|
Really?  We can't get the is_system_created flag from data_?

http://codereview.chromium.org/598071/diff/3007/8004#newcode127
net/base/address_list.cc:127: uint16* port_field = GetPortField(data_->head);
GetPortField may return a NULL pointer.  We should check
that and return either 0 or -1 if port_field is NULL.

All callers of GetPortField should be checked.

http://codereview.chromium.org/598071/diff/3007/8005
File net/base/address_list.h (right):

http://codereview.chromium.org/598071/diff/3007/8005#newcode27
net/base/address_list.h:27: // all linked struct addrinfos will be copied as
well.
It's more important to document what happens when |recursive|
is false.

An alternative design is to add a CopyOne method that does
a nonrecursive copy.  This way we don't need to remember
what 'true' or 'false' means to Copy.

http://codereview.chromium.org/598071/diff/3007/8005#newcode32
net/base/address_list.h:32: void AppendCopy(const struct addrinfo* head);
Nit: This method can be named just "Append".  The 'const' on
the input argument implies that it will be copied rather than
adopted.

http://codereview.chromium.org/598071/diff/3007/8008
File net/base/nss_memio.c (right):

http://codereview.chromium.org/598071/diff/3007/8008#newcode368
net/base/nss_memio.c:368: memcpy(&secret->peername, peername, peername_len);
To be safe, let's add a check that peername_len is <= sizeof(PRNetAddr).

  DCHECK_LE(peername_len, sizeof(secret->peername));
  size_t len = std::min(peername_len, sizeof(secret->peername));
  memcpy(&secret->peername, peername, len);

http://codereview.chromium.org/598071/diff/3007/8009
File net/base/nss_memio.h (right):

http://codereview.chromium.org/598071/diff/3007/8009#newcode41
net/base/nss_memio.h:41: void memio_SetPeerName(PRFileDesc *fd, const PRNetAddr
*peername,
peername should be declared as
  const struct sockaddr *peername,

unless it would make the code more complicated.  You should
be able to use a forward declaration for struct sockaddr:

  struct sockaddr;

http://codereview.chromium.org/598071/diff/3007/8023
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/598071/diff/3007/8023#newcode318
net/socket/ssl_client_socket_nss.cc:318: AddressList address;
Nit: address => peer_address

http://codereview.chromium.org/598071/diff/3007/8027
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/598071/diff/3007/8027#newcode474
net/socket/tcp_client_socket_libevent.cc:474: return ERR_FAILED;
Return ERR_UNEXPECTED or ERR_INVALID_ARGUMENT.  Ideally,
we should have an error code that means the function is
called when the object is in the wrong state.  Do we have
a ERR_NOT_CONNECTED error code?

The default error code ERR_FAILED should be avoided whenever
possible.

http://codereview.chromium.org/598071/diff/3007/8031
File net/socket/tcp_client_socket_win.h (right):

http://codereview.chromium.org/598071/diff/3007/8031#newcode11
net/socket/tcp_client_socket_win.h:11: #include "net/base/sys_addrinfo.h"
We shouldn't need to include this header.

[Paweł Hajdan Jr., 2010-02-18 11:55:28]

Thanks for the review, patch updated.

eroman has already reviewed this issue.

http://codereview.chromium.org/598071/diff/3007/8004
File net/base/address_list.cc (right):

http://codereview.chromium.org/598071/diff/3007/8004#newcode109
net/base/address_list.cc:109: // system-created data (because we don't know
whether |head|
On 2010/02/17 20:18:18, wtc wrote:
> Really?  We can't get the is_system_created flag from data_?

I changed this code to be smarter about that.

http://codereview.chromium.org/598071/diff/3007/8004#newcode127
net/base/address_list.cc:127: uint16* port_field = GetPortField(data_->head);
On 2010/02/17 20:18:18, wtc wrote:
> GetPortField may return a NULL pointer.  We should check
> that and return either 0 or -1 if port_field is NULL.
> 
> All callers of GetPortField should be checked.

Done.

http://codereview.chromium.org/598071/diff/3007/8005
File net/base/address_list.h (right):

http://codereview.chromium.org/598071/diff/3007/8005#newcode27
net/base/address_list.h:27: // all linked struct addrinfos will be copied as
well.
On 2010/02/17 20:18:18, wtc wrote:
> It's more important to document what happens when |recursive|
> is false.

Done.

> An alternative design is to add a CopyOne method that does
> a nonrecursive copy.  This way we don't need to remember
> what 'true' or 'false' means to Copy.

I considered that too, but CopyOne still seems somewhat ambiguous to me. Or
CopyHead etc. A lone boolean parameter is ambiguous too, but file_util::Delete
etc. use a similar solution for recursive, so at least it's consistent.

What do you think? I can implement some alternative solution if you'd like it
more.

http://codereview.chromium.org/598071/diff/3007/8005#newcode32
net/base/address_list.h:32: void AppendCopy(const struct addrinfo* head);
On 2010/02/17 20:18:18, wtc wrote:
> Nit: This method can be named just "Append".  The 'const' on
> the input argument implies that it will be copied rather than
> adopted.

Done.

http://codereview.chromium.org/598071/diff/3007/8008
File net/base/nss_memio.c (right):

http://codereview.chromium.org/598071/diff/3007/8008#newcode368
net/base/nss_memio.c:368: memcpy(&secret->peername, peername, peername_len);
On 2010/02/17 20:18:18, wtc wrote:
> To be safe, let's add a check that peername_len is <= sizeof(PRNetAddr).
> 
>   DCHECK_LE(peername_len, sizeof(secret->peername));
>   size_t len = std::min(peername_len, sizeof(secret->peername));
>   memcpy(&secret->peername, peername, len);

Done in an NSPR style. Some comment in these files indicate that they may get
included by upstream. Also, other places in the file use NSPR style.

http://codereview.chromium.org/598071/diff/3007/8009
File net/base/nss_memio.h (right):

http://codereview.chromium.org/598071/diff/3007/8009#newcode41
net/base/nss_memio.h:41: void memio_SetPeerName(PRFileDesc *fd, const PRNetAddr
*peername,
On 2010/02/17 20:18:18, wtc wrote:
> peername should be declared as
>   const struct sockaddr *peername,
> 
> unless it would make the code more complicated.  You should
> be able to use a forward declaration for struct sockaddr:
> 
>   struct sockaddr;

Done.

http://codereview.chromium.org/598071/diff/3007/8023
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/598071/diff/3007/8023#newcode318
net/socket/ssl_client_socket_nss.cc:318: AddressList address;
On 2010/02/17 20:18:18, wtc wrote:
> Nit: address => peer_address

Done.

http://codereview.chromium.org/598071/diff/3007/8027
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/598071/diff/3007/8027#newcode474
net/socket/tcp_client_socket_libevent.cc:474: return ERR_FAILED;
On 2010/02/17 20:18:18, wtc wrote:
> Return ERR_UNEXPECTED or ERR_INVALID_ARGUMENT.  Ideally,
> we should have an error code that means the function is
> called when the object is in the wrong state.  Do we have
> a ERR_NOT_CONNECTED error code?
> 
> The default error code ERR_FAILED should be avoided whenever
> possible.

Done.

http://codereview.chromium.org/598071/diff/3007/8031
File net/socket/tcp_client_socket_win.h (right):

http://codereview.chromium.org/598071/diff/3007/8031#newcode11
net/socket/tcp_client_socket_win.h:11: #include "net/base/sys_addrinfo.h"
On 2010/02/17 20:18:18, wtc wrote:
> We shouldn't need to include this header.

Replaced with a system header, which *is* needed so that we have definition of
SOCKET (used below in the header file). The trybot agrees.

[eroman, 2010-02-18 23:50:49]

> eroman: are you going to review this CL?

I signed off on this earlier, barring some comments.
The latest patchset still LGTM.

http://codereview.chromium.org/598071/diff/14001/14004
File net/base/address_list.cc (right):

http://codereview.chromium.org/598071/diff/14001/14004#newcode91
net/base/address_list.cc:91: return;
What was this needed for something? Are we hitting it?

http://codereview.chromium.org/598071/diff/14001/14004#newcode125
net/base/address_list.cc:125: data_ = new Data(new_head, false
/*is_system_created*/);
nit: The location of this is confusing to me.

I think it would be more readable to have this earlier (i.e in the first if
block when we CreateCopyOfAddrinfo() for new_head).

[Paweł Hajdan Jr., 2010-02-19 16:46:53]

http://codereview.chromium.org/598071/diff/14001/14004
File net/base/address_list.cc (right):

http://codereview.chromium.org/598071/diff/14001/14004#newcode91
net/base/address_list.cc:91: return;
On 2010/02/18 23:50:50, eroman wrote:
> What was this needed for something? Are we hitting it?

wtc asked for this change. It's not directly related. In theory this should be
NOTREACHED().

http://codereview.chromium.org/598071/diff/14001/14004#newcode125
net/base/address_list.cc:125: data_ = new Data(new_head, false
/*is_system_created*/);
On 2010/02/18 23:50:50, eroman wrote:
> nit: The location of this is confusing to me.
> 
> I think it would be more readable to have this earlier (i.e in the first if
> block when we CreateCopyOfAddrinfo() for new_head).

Done.

[wtc, 2010-02-19 21:58:51]

http://codereview.chromium.org/598071/diff/14001/14004
File net/base/address_list.cc (right):

http://codereview.chromium.org/598071/diff/14001/14004#newcode90
net/base/address_list.cc:90: NOTREACHED();
This NOTREACHED() and the NOTREACHED() at line 135 is not
necessary because GetPortField already calls NOTREACHED()
before returning NULL (at line 81).

[eroman, 2010-02-19 22:04:18]

(Not to extend this discussion, since I am fine with either outcome... but my
preference regarding these short circuits is not to handle it gracefully. I
think the earlier behavior of crashing with a NULL deref is actually preferable
to handling it as a no-op. If for some reason we do hit this, the silent failure
will only result in a latent bug somewhere else, like connecting to the wrong
server or crashing anway. Just my two cents).

[wtc, 2010-02-19 23:12:50]

Pawel, since eroman already reviewed this, you can check
this in.

I only checked the three places you responded to.  Thank
you for using PR_ASSERT and PR_MIN in nss_memio.c.  I
forgot that file should depend on only NSPR.

http://codereview.chromium.org/598071/diff/3007/8005
File net/base/address_list.h (right):

http://codereview.chromium.org/598071/diff/3007/8005#newcode27
net/base/address_list.h:27: // all linked struct addrinfos will be copied as
well.
Using a 'bool recursive' parameter for Copy() is fine by me.

http://codereview.chromium.org/598071/diff/3007/8031
File net/socket/tcp_client_socket_win.h (right):

http://codereview.chromium.org/598071/diff/3007/8031#newcode11
net/socket/tcp_client_socket_win.h:11: #include "net/base/sys_addrinfo.h"
On 2010/02/18 11:55:29, Paweł Hajdan Jr. wrote:
> 
> Replaced with a system header, which *is* needed so that we have definition of
> SOCKET (used below in the header file). The trybot agrees.

I see.  I didn't notice the 'SOCKET'.  In that case,
<winsock2.h> is a better system header than <ws2tcpip.h>.

http://codereview.chromium.org/598071/diff/16001/16004
File net/base/address_list.cc (right):

http://codereview.chromium.org/598071/diff/16001/16004#newcode89
net/base/address_list.cc:89: if (!port_field) {
The proper error handling is to skip 'info' and move on to
info->ai_next.  I.e.,

  if (port_field)
    *port_field = htons(port);

because info->ai_next may be of an address family that we
support.

http://codereview.chromium.org/598071/diff/16001/16004#newcode133
net/base/address_list.cc:133: if (!port_field) {
Returning port -1 on an unsupported address family is
fine.

[Paweł Hajdan Jr., 2010-02-20 18:51:33]

Committed after addressing wtc's latest comments.