IPEndPoint::ToString should not crash when the IP address is empty.

Do not call IPEndpoint::ToString for empty IP address.
Also add more logging to SocketHostTcp for easier debugging.

BUG=463680
Committed: https://crrev.com/a572ab9bc342e4d1eac0c1dab3badd82aa5df1fb
Cr-Commit-Position: refs/heads/master@{#320136}

[jiayl, 2015-03-04 00:05:10]

jiayl@chromium.org changed reviewers:
+ agl@chromium.org

[jiayl, 2015-03-04 00:05:10]

PTAL

[jiayl, 2015-03-05 17:09:41]

jiayl@chromium.org changed reviewers:
+ rsleevi@chromium.org

[jiayl, 2015-03-05 17:09:42]

PTAL

[Ryan Sleevi, 2015-03-05 17:14:52]

rsleevi@chromium.org changed reviewers:
- agl@chromium.org

[Ryan Sleevi, 2015-03-05 17:14:52]

-agl

I'm inclined to say not LGTM

The caller is violating the implicit API contract of failing to sanitize /
validate the IP address. That is, I strongly feel sanitization belongs closest
to the thing taking user input, and we shouldn't be creating IPEndPoints that
aren't valid.

On a more pedantic side, //net doesn't use {} for single-line ifs, and you
should always prefer std::string() over "" for representing empty-strings.

[jiayl, 2015-03-05 17:19:19]

On 2015/03/05 17:14:52, Ryan Sleevi wrote:
> -agl
> 
> I'm inclined to say not LGTM
> 
> The caller is violating the implicit API contract of failing to sanitize /
> validate the IP address. That is, I strongly feel sanitization belongs closest
> to the thing taking user input, and we shouldn't be creating IPEndPoints that
> aren't valid.
> 
> On a more pedantic side, //net doesn't use {} for single-line ifs, and you
> should always prefer std::string() over "" for representing empty-strings.

The API is documented as
// Returns value as a string (e.g. "127.0.0.1:80"). Returns empty
// string if the address is invalid, and cannot not be converted to a
// string.
Also IPEndPoint should probably hide the public ctor if it does not allow to be
created
from invalid address.

[Ryan Sleevi, 2015-03-05 17:26:35]

On 2015/03/05 17:19:19, jiayl wrote:
> The API is documented as
> // Returns value as a string (e.g. "127.0.0.1:80"). Returns empty
> // string if the address is invalid, and cannot not be converted to a
> // string.

Hrm. It's never ever behaved as documented. Fun.

> Also IPEndPoint should probably hide the public ctor if it does not allow to
be
> created
> from invalid address.

If //net had a better public/private API separation, yes.

In general, //net assumes two key things:
- Single thread (the 'network task runner')
- Sanitized inputs (generally handled by //content)

That said, better to make assumptions explicit. For the record, I think we
should fix the header (which was never accurate and thus not depended upon
behaviour until now) and consider moving the sanitizing up to the input place,
before you string it through the API.

[jiayl, 2015-03-05 17:50:33]

On 2015/03/05 17:26:35, Ryan Sleevi wrote:
> On 2015/03/05 17:19:19, jiayl wrote:
> > The API is documented as
> > // Returns value as a string (e.g. "127.0.0.1:80"). Returns empty
> > // string if the address is invalid, and cannot not be converted to a
> > // string.
> 
> Hrm. It's never ever behaved as documented. Fun.
> 
> > Also IPEndPoint should probably hide the public ctor if it does not allow to
> be
> > created
> > from invalid address.
> 
> If //net had a better public/private API separation, yes.
> 
> In general, //net assumes two key things:
> - Single thread (the 'network task runner')
> - Sanitized inputs (generally handled by //content)
> 
> That said, better to make assumptions explicit. For the record, I think we
> should fix the header (which was never accurate and thus not depended upon
> behaviour until now) and consider moving the sanitizing up to the input place,
> before you string it through the API.

OK, fixed the comment and the caller. PTAL

[jiayl, 2015-03-05 17:51:19]

jiayl@chromium.org changed reviewers:
+ juberti@chromium.org

[jiayl, 2015-03-05 17:51:20]

PTAL

[Ryan Sleevi, 2015-03-05 18:07:34]

https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host_tcp.cc:265: result =
socket_->GetPeerAddress(&remote_address);
How is this possible? I assume you can reproduce the issue in some way?

GetPeerAddress() should never be returning an invalid address for a connected
socket. The HttpProxyClientSocket puts the transport socket (of the proxy) into
the remote_address

This sounds like it might be an API violation in //net that we should fix,
because you shouldn't have to worry about //net giving you junk if you're doing
"the right thing"

[jiayl, 2015-03-05 18:48:34]

On 2015/03/05 18:07:34, Ryan Sleevi wrote:
>
https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):
> 
>
https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
> content/browser/renderer_host/p2p/socket_host_tcp.cc:265: result =
> socket_->GetPeerAddress(&remote_address);
> How is this possible? I assume you can reproduce the issue in some way?
> 
> GetPeerAddress() should never be returning an invalid address for a connected
> socket. The HttpProxyClientSocket puts the transport socket (of the proxy)
into
> the remote_address
> 
> This sounds like it might be an API violation in //net that we should fix,
> because you shouldn't have to worry about //net giving you junk if you're
doing
> "the right thing"

[jiayl, 2015-03-05 18:49:18]

On 2015/03/05 18:48:34, jiayl wrote:
> On 2015/03/05 18:07:34, Ryan Sleevi wrote:
> >
>
https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
> > File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):
> > 
> >
>
https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
> > content/browser/renderer_host/p2p/socket_host_tcp.cc:265: result =
> > socket_->GetPeerAddress(&remote_address);
> > How is this possible? I assume you can reproduce the issue in some way?
> > 
> > GetPeerAddress() should never be returning an invalid address for a
connected
> > socket. The HttpProxyClientSocket puts the transport socket (of the proxy)
> into
> > the remote_address
> > 
> > This sounds like it might be an API violation in //net that we should fix,
> > because you shouldn't have to worry about //net giving you junk if you're
> doing
> > "the right thing"

It's from ProxyResolvingClientSocket::GetPeerAddress, which returns empty ip
address intentionally if it's behind a proxy, 
since we don't want to expose the proxy address to the lower layer.

[juberti2, 2015-03-05 19:11:32]

On 2015/03/05 18:49:18, jiayl wrote:
> On 2015/03/05 18:48:34, jiayl wrote:
> > On 2015/03/05 18:07:34, Ryan Sleevi wrote:
> > >
> >
>
https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
> > > File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/976903002/diff/20001/content/browser/renderer...
> > > content/browser/renderer_host/p2p/socket_host_tcp.cc:265: result =
> > > socket_->GetPeerAddress(&remote_address);
> > > How is this possible? I assume you can reproduce the issue in some way?
> > > 
> > > GetPeerAddress() should never be returning an invalid address for a
> connected
> > > socket. The HttpProxyClientSocket puts the transport socket (of the proxy)
> > into
> > > the remote_address
> > > 
> > > This sounds like it might be an API violation in //net that we should fix,
> > > because you shouldn't have to worry about //net giving you junk if you're
> > doing
> > > "the right thing"
> 
> It's from ProxyResolvingClientSocket::GetPeerAddress, which returns empty ip
> address intentionally if it's behind a proxy, 
> since we don't want to expose the proxy address to the lower layer.

and, because the socket will not have the exact peer address, as the proxy
doesn't provide that information.

[Ryan Sleevi, 2015-03-05 19:26:50]

On 2015/03/05 19:11:32, juberti2 wrote:
> > It's from ProxyResolvingClientSocket::GetPeerAddress, which returns empty ip
> > address intentionally if it's behind a proxy, 
> > since we don't want to expose the proxy address to the lower layer.
> 
> and, because the socket will not have the exact peer address, as the proxy
> doesn't provide that information.

Sure, but even the HttpProxyClientSocket in Chrome implements this method, and
surfaces the *proxy*'s information.

[jiayl, 2015-03-06 01:02:00]

On 2015/03/05 19:26:50, Ryan Sleevi wrote:
> On 2015/03/05 19:11:32, juberti2 wrote:
> > > It's from ProxyResolvingClientSocket::GetPeerAddress, which returns empty
ip
> > > address intentionally if it's behind a proxy, 
> > > since we don't want to expose the proxy address to the lower layer.
> > 
> > and, because the socket will not have the exact peer address, as the proxy
> > doesn't provide that information.
> 
> Sure, but even the HttpProxyClientSocket in Chrome implements this method, and
> surfaces the *proxy*'s information.

What about return ERR_NAME_NOT_RESOLVED from
ProxyResolvingClientSocket::GetPeerAddress
instead of returning empty IP address?

[Ryan Sleevi, 2015-03-06 19:37:57]

On 2015/03/06 01:02:00, jiayl wrote:
> What about return ERR_NAME_NOT_RESOLVED from
> ProxyResolvingClientSocket::GetPeerAddress
> instead of returning empty IP address?

Indicating a //net failure seems fine, although it does seem better to follow
//net and give the proxy's address (when using a proxy). I suppose this is
mostly a WebRTC thing, but yes, an error over an empty address is desirable
(since we already surface errors if not connected, usually ERR_ADDRESS_INVALID
or ERR_SOCKET_NOT_CONNECTED)

[jiayl, 2015-03-06 20:36:09]

PTAL

[Ryan Sleevi, 2015-03-06 21:23:41]

//net LGTM % nits
//jingle has a bug(ish)

https://codereview.chromium.org/976903002/diff/40001/jingle/glue/proxy_resolv...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/976903002/diff/40001/jingle/glue/proxy_resolv...
jingle/glue/proxy_resolving_client_socket.cc:361:
net::IPEndPoint(net::IPAddressNumber(), dest_host_port_pair_.port());
BUG: Don't assign to *address

Just return an error (similar to line 351)

https://codereview.chromium.org/976903002/diff/40001/net/base/ip_endpoint.h
File net/base/ip_endpoint.h (right):

https://codereview.chromium.org/976903002/diff/40001/net/base/ip_endpoint.h#n...
net/base/ip_endpoint.h:57: // valid, or it will crash.
remove "or it will crash."

[jiayl, 2015-03-09 23:01:56]

Justin, 

PTAL.

[juberti2, 2015-03-11 00:59:23]

lgtm % nits

https://codereview.chromium.org/976903002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/976903002/diff/60001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host_tcp.cc:220: LOG(WARNING) << "Error
from  connecting TLS socket, status=" << status;
extra space after from

https://codereview.chromium.org/976903002/diff/60001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host_tcp.cc:263: // |remote_address|
could be empty if it is connected through a proxy.
does this comment need an update to discuss NAME_NOT_RESOLVED?

https://codereview.chromium.org/976903002/diff/60001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host_tcp.cc:272: if
(!remote_address.address().empty()) {
Should we VLOG to indicate that we can't get a remote address in the 'else'
case?

[jiayl, 2015-03-11 17:34:48]

jiayl@chromium.org changed reviewers:
+ sergeyu@chromium.org

[jiayl, 2015-03-11 17:34:49]

Adding Sergey for jingle/glue

[Sergey Ulanov, 2015-03-11 18:35:23]

lgtm

https://codereview.chromium.org/976903002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

https://codereview.chromium.org/976903002/diff/80001/content/browser/renderer...
content/browser/renderer_host/p2p/socket_host_tcp.cc:280: VLOG(1) << "Cannot get
remote address due to proxy.";
Suggest rewording: "Remote address is unknown, connection is proxied."

[jiayl, 2015-03-11 18:36:59]

The CQ bit was checked by jiayl@chromium.org

[jiayl, 2015-03-11 18:37:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, juberti@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/976903002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-03-11 18:37:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976903002/100001

[commit-bot: I haz the power, 2015-03-11 20:27:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-11 20:29:09]

Patchset 6 (id:??) landed as
https://crrev.com/a572ab9bc342e4d1eac0c1dab3badd82aa5df1fb
Cr-Commit-Position: refs/heads/master@{#320136}