Use uint16 for port numbers, net/ edition

Use uint16 for port numbers, net/ edition

This is a piece of a larger CL, split up to avoid overwhelming reviewers. The
entire CL can be viewed at https://codereview.chromium.org/655063002/ .
The intent is to get each piece reviewed separately, then finally land the large
CL after all parts have been approved.

BUG=81439
TEST=none

[Peter Kasting, 2014-11-12 23:50:50]

pkasting@chromium.org changed reviewers:
+ mmenke@chromium.org

[Peter Kasting, 2014-11-12 23:54:31]

This is not an independent CL; please see the description for a link to the full
context including brief rationale.

To minimize the chance of problems, after getting green try runs on the large
change, I manually reviewed each smaller CL and added comments where helpful.

https://codereview.chromium.org/718273002/diff/20001/net/base/host_mapping_ru...
File net/base/host_mapping_rules.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/base/host_mapping_ru...
net/base/host_mapping_rules.cc:61:
host_port->set_port(static_cast<uint16>(rule.replacement_port));
ParseHostAndPort() guarantees that this number will either be -1 or [0..65535],
so this cast is safe.

https://codereview.chromium.org/718273002/diff/20001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/base/net_util.cc#new...
net/base/net_util.cc:304: return port >= 0 && port <= 65535;
This ensures it's safe to cast the port number to uint16 in places like
ftp_network_transaction.cc.

https://codereview.chromium.org/718273002/diff/20001/net/dns/mock_host_resolv...
File net/dns/mock_host_resolver.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/dns/mock_host_resolv...
net/dns/mock_host_resolver.cc:51: addrlist->push_back(IPEndPoint(ip_number, 0));
The port number here isn't actually used -- MockHostResolverBase::ResolveProc()
calls AddressList::CopyWithPort().

https://codereview.chromium.org/718273002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_manager.cc:422: (port >
std::numeric_limits<uint16>::max())) {
The old code would have erroneously allowed 65536.

https://codereview.chromium.org/718273002/diff/20001/net/http/http_stream_fac...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:63: port <= 0 || port >
std::numeric_limits<uint16>::max()) {
In this case the old code was safe, but it's so close to the broken code in
http_server_properties_manager.cc that I changed to this less-likely-to-screw-up
form (also for consistency).

https://codereview.chromium.org/718273002/diff/20001/net/proxy/proxy_server.cc
File net/proxy/proxy_server.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/proxy/proxy_server.c...
net/proxy/proxy_server.cc:241: host_port_pair = HostPortPair(host,
static_cast<uint16>(port));
ParseHostAndPort() guarantees that this number will either be -1 or [0..65535],
so this cast is safe.

https://codereview.chromium.org/718273002/diff/20001/net/socket/tcp_listen_so...
File net/socket/tcp_listen_socket.h (left):

https://codereview.chromium.org/718273002/diff/20001/net/socket/tcp_listen_so...
net/socket/tcp_listen_socket.h:44: class NET_EXPORT TCPListenSocketFactory :
public StreamListenSocketFactory {
This factory was never used, so I deleted it while I was here.

[mmenke, 2014-11-13 16:00:43]

I need to really carefully go over this, and make sure we aren't breaking
anything.  GURL's special values for ports, in particular, make this whole thing
very messy.

https://codereview.chromium.org/718273002/diff/20001/net/base/ip_endpoint_uni...
File net/base/ip_endpoint_unittest.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/base/ip_endpoint_uni...
net/base/ip_endpoint_unittest.cc:32: uint16 test_count =
static_cast<uint16>(arraysize(tests));
This seems a little weird - think it makes more sense just casting in
IPEndPointTest, maybe with a DCHECK that index is in the right range.

https://codereview.chromium.org/718273002/diff/20001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/base/net_util.cc#new...
net/base/net_util.cc:304: return port >= 0 && port <= 65535;
On 2014/11/12 23:54:31, Peter Kasting wrote:
> This ensures it's safe to cast the port number to uint16 in places like
> ftp_network_transaction.cc.

Not a big fan of depending on this for correctness, but since it doesn't look
like we had any better protection for this case before, that's beyond the scope
of this CL.

https://codereview.chromium.org/718273002/diff/20001/net/test/embedded_test_s...
File net/test/embedded_test_server/embedded_test_server.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/test/embedded_test_s...
net/test/embedded_test_server/embedded_test_server.cc:118: : port_(65535),
0 is probably a better initial value - 65535 is a valid port, 0 isn't.

[Peter Kasting, 2014-11-14 20:42:53]

On 2014/11/13 16:00:43, mmenke wrote:
> I need to really carefully go over this, and make sure we aren't breaking
> anything.  GURL's special values for ports, in particular, make this whole
thing
> very messy.

Please take as much time as you need.

https://codereview.chromium.org/718273002/diff/20001/net/base/ip_endpoint_uni...
File net/base/ip_endpoint_unittest.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/base/ip_endpoint_uni...
net/base/ip_endpoint_unittest.cc:32: uint16 test_count =
static_cast<uint16>(arraysize(tests));
On 2014/11/13 16:00:42, mmenke wrote:
> This seems a little weird - think it makes more sense just casting in
> IPEndPointTest, maybe with a DCHECK that index is in the right range.

You mean, add casts to every TEST_F below where they construct an IPEndPoint? 
It's hard for me to be enthusiastic about replacing one cast with seven,
especially when the cast here is obviously safe by inspection, while casts below
would force the caller to check elsewhere in the file if they really want to
know the cast is safe.

https://codereview.chromium.org/718273002/diff/20001/net/test/embedded_test_s...
File net/test/embedded_test_server/embedded_test_server.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/test/embedded_test_s...
net/test/embedded_test_server/embedded_test_server.cc:118: : port_(65535),
On 2014/11/13 16:00:42, mmenke wrote:
> 0 is probably a better initial value - 65535 is a valid port, 0 isn't.

Done.

[mmenke, 2014-11-14 21:05:53]

https://codereview.chromium.org/718273002/diff/20001/net/base/ip_endpoint_uni...
File net/base/ip_endpoint_unittest.cc (right):

https://codereview.chromium.org/718273002/diff/20001/net/base/ip_endpoint_uni...
net/base/ip_endpoint_unittest.cc:32: uint16 test_count =
static_cast<uint16>(arraysize(tests));
On 2014/11/14 20:42:53, Peter Kasting wrote:
> On 2014/11/13 16:00:42, mmenke wrote:
> > This seems a little weird - think it makes more sense just casting in
> > IPEndPointTest, maybe with a DCHECK that index is in the right range.
> 
> You mean, add casts to every TEST_F below where they construct an IPEndPoint? 
> It's hard for me to be enthusiastic about replacing one cast with seven,
> especially when the cast here is obviously safe by inspection, while casts
below
> would force the caller to check elsewhere in the file if they really want to
> know the cast is safe.

Sorry - I misread the code.  Had been thinking it was only the last test, but
it's a lot more than that, so this makes sense.  You're right.

[mmenke, 2014-11-17 20:10:01]

On 2014/11/14 20:42:53, Peter Kasting wrote:
> On 2014/11/13 16:00:43, mmenke wrote:
> > I need to really carefully go over this, and make sure we aren't breaking
> > anything.  GURL's special values for ports, in particular, make this whole
> thing
> > very messy.
> 
> Please take as much time as you need.

Just an update:  A couple of these changes concern me a bit, so I'm going to go
through and land a couple small preliminary CLs to make port checking more
robust in a couple places, and run them by the appropriate owners.  Then we can
land this.  These are all small CLs, so expect to be able to sign off on this
later this week.

[Peter Kasting, 2014-11-17 22:14:42]

On 2014/11/17 20:10:01, mmenke wrote:
> On 2014/11/14 20:42:53, Peter Kasting wrote:
> > On 2014/11/13 16:00:43, mmenke wrote:
> > > I need to really carefully go over this, and make sure we aren't breaking
> > > anything.  GURL's special values for ports, in particular, make this whole
> > thing
> > > very messy.
> > 
> > Please take as much time as you need.
> 
> Just an update:  A couple of these changes concern me a bit, so I'm going to
go
> through and land a couple small preliminary CLs to make port checking more
> robust in a couple places, and run them by the appropriate owners.  Then we
can
> land this.  These are all small CLs, so expect to be able to sign off on this
> later this week.

Cool, thanks for taking the time to help out!

[mmenke, 2014-11-18 21:06:40]

Ok, this looks good.  Could you check that files you've added uint16 to include
basictypes.h?  Also, I've pointed out a bunch of places where the code is
equivalent to HostPortPair::FromURL - code should have used it before, and with
the casting, anyways, and nice to have the casting in fewer places.

https://codereview.chromium.org/718273002/diff/40001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:304: return port >= 0 && port <= 65535;
return IsPortValid(port);

(New function, inspire by this CL)

https://codereview.chromium.org/718273002/diff/40001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/718273002/diff/40001/net/dns/host_resolver.h#...
net/dns/host_resolver.h:70: uint16 port() const { return host_port_pair_.port();
}
should include basictypes.h

https://codereview.chromium.org/718273002/diff/40001/net/ftp/ftp_network_tran...
File net/ftp/ftp_network_transaction.h (right):

https://codereview.chromium.org/718273002/diff/40001/net/ftp/ftp_network_tran...
net/ftp/ftp_network_transaction.h:248: uint16 data_connection_port_;
include basictypes.h

https://codereview.chromium.org/718273002/diff/40001/net/http/http_network_tr...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_network_tr...
net/http/http_network_transaction.cc:1032:
static_cast<uint16>(request_->url.EffectiveIntPort()));
HostPortPair::FromURL(request.url)

https://codereview.chromium.org/718273002/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_manager.cc:452: static_cast<uint16>(port),
protocol, probability);
Should include basictypes.h

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:63: port <= 0 || port >
std::numeric_limits<uint16>::max()) {
nit:  Replace these two lines with "port == 0 || !IsPortValid(port)"

and include net/net_util.h.

IsPortValid allows ports of 0, since it's more consistent with previous
behavior.

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:98: host_port, static_cast<uint16>(port),
protocol, probability);
include basictypes.h

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl.cc:189:
static_cast<uint16>(original_url.EffectiveIntPort()));
HostPortPair::FromURL(original_url)

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:137:
static_cast<uint16>(request_info_.url.EffectiveIntPort()));
HostPortPair::FromURL

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:620: uint16 port =
static_cast<uint16>(request_info_.url.EffectiveIntPort());
HostPortPair::FromURL

https://codereview.chromium.org/718273002/diff/40001/net/socket/client_socket...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:96:
static_cast<uint16>(request_url.EffectiveIntPort()));
HostPortPair::FromURL

https://codereview.chromium.org/718273002/diff/40001/net/socket/server_socket.h
File net/socket/server_socket.h (right):

https://codereview.chromium.org/718273002/diff/40001/net/socket/server_socket...
net/socket/server_socket.h:33: uint16 port,
include basictypes.h

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
File net/spdy/hpack_huffman_aggregator.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
net/spdy/hpack_huffman_aggregator.cc:54:
static_cast<uint16>(request.url.EffectiveIntPort()));
While you're here, this is just HostPortPair::FromURL(request.url)

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
net/spdy/hpack_huffman_aggregator.cc:114:
static_cast<uint16>(request.url.EffectiveIntPort()));
HostPortPair::FromURL(request.url)

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
net/spdy/hpack_huffman_aggregator.cc:125:
static_cast<uint16>(referer.EffectiveIntPort()));
HostPortPair::FromURL(request.url)

[mmenke, 2014-11-18 21:24:50]

Oh, also need to merge - I modified one of the lines you touched (Line 422 in
https://codereview.chromium.org/718273002/diff/40001/net/http/http_server_pro...).
 Think that's the only conflict.

[Peter Kasting, 2014-11-18 23:38:43]

New snap up.

I verified that in all the files where I had referenced uint16, either that file
or its immediate header file now #include "base/basictypes.h".

https://codereview.chromium.org/718273002/diff/40001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:304: return port >= 0 && port <= 65535;
On 2014/11/18 21:06:39, mmenke wrote:
> return IsPortValid(port);
> 
> (New function, inspire by this CL)

Done.

https://codereview.chromium.org/718273002/diff/40001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/718273002/diff/40001/net/dns/host_resolver.h#...
net/dns/host_resolver.h:70: uint16 port() const { return host_port_pair_.port();
}
On 2014/11/18 21:06:39, mmenke wrote:
> should include basictypes.h

Done.

https://codereview.chromium.org/718273002/diff/40001/net/ftp/ftp_network_tran...
File net/ftp/ftp_network_transaction.h (right):

https://codereview.chromium.org/718273002/diff/40001/net/ftp/ftp_network_tran...
net/ftp/ftp_network_transaction.h:248: uint16 data_connection_port_;
On 2014/11/18 21:06:39, mmenke wrote:
> include basictypes.h

Done.

https://codereview.chromium.org/718273002/diff/40001/net/http/http_network_tr...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_network_tr...
net/http/http_network_transaction.cc:1032:
static_cast<uint16>(request_->url.EffectiveIntPort()));
On 2014/11/18 21:06:39, mmenke wrote:
> HostPortPair::FromURL(request.url)

Done.

https://codereview.chromium.org/718273002/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_manager.cc:452: static_cast<uint16>(port),
protocol, probability);
On 2014/11/18 21:06:39, mmenke wrote:
> Should include basictypes.h

In this file's header file.

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:98: host_port, static_cast<uint16>(port),
protocol, probability);
On 2014/11/18 21:06:39, mmenke wrote:
> include basictypes.h

In this file's header file.

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl.cc:189:
static_cast<uint16>(original_url.EffectiveIntPort()));
On 2014/11/18 21:06:39, mmenke wrote:
> HostPortPair::FromURL(original_url)

Done.

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:137:
static_cast<uint16>(request_info_.url.EffectiveIntPort()));
On 2014/11/18 21:06:39, mmenke wrote:
> HostPortPair::FromURL

Done.

https://codereview.chromium.org/718273002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:620: uint16 port =
static_cast<uint16>(request_info_.url.EffectiveIntPort());
On 2014/11/18 21:06:39, mmenke wrote:
> HostPortPair::FromURL

Done.

https://codereview.chromium.org/718273002/diff/40001/net/socket/client_socket...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/socket/client_socket...
net/socket/client_socket_pool_manager.cc:96:
static_cast<uint16>(request_url.EffectiveIntPort()));
On 2014/11/18 21:06:39, mmenke wrote:
> HostPortPair::FromURL

Done.

https://codereview.chromium.org/718273002/diff/40001/net/socket/server_socket.h
File net/socket/server_socket.h (right):

https://codereview.chromium.org/718273002/diff/40001/net/socket/server_socket...
net/socket/server_socket.h:33: uint16 port,
On 2014/11/18 21:06:39, mmenke wrote:
> include basictypes.h

Done.

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
File net/spdy/hpack_huffman_aggregator.cc (right):

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
net/spdy/hpack_huffman_aggregator.cc:54:
static_cast<uint16>(request.url.EffectiveIntPort()));
On 2014/11/18 21:06:39, mmenke wrote:
> While you're here, this is just HostPortPair::FromURL(request.url)

Done.

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
net/spdy/hpack_huffman_aggregator.cc:114:
static_cast<uint16>(request.url.EffectiveIntPort()));
On 2014/11/18 21:06:39, mmenke wrote:
> HostPortPair::FromURL(request.url)

Done.

https://codereview.chromium.org/718273002/diff/40001/net/spdy/hpack_huffman_a...
net/spdy/hpack_huffman_aggregator.cc:125:
static_cast<uint16>(referer.EffectiveIntPort()));
On 2014/11/18 21:06:39, mmenke wrote:
> HostPortPair::FromURL(request.url)

Done.

[mmenke, 2014-11-19 15:29:22]

LGTM, thanks for doing this!

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

https://codereview.chromium.org/718273002/diff/60001/net/base/ip_endpoint.h#n...
net/base/ip_endpoint.h:28: IPEndPoint(const IPEndPoint& endpoint);
While you're here, mind adding explicit to the last constructor?

[Peter Kasting, 2014-11-19 22:41:28]

On 2014/11/19 15:29:22, mmenke wrote:
> LGTM, thanks for doing this!
> 
> https://codereview.chromium.org/718273002/diff/60001/net/base/ip_endpoint.h
> File net/base/ip_endpoint.h (right):
> 
>
https://codereview.chromium.org/718273002/diff/60001/net/base/ip_endpoint.h#n...
> net/base/ip_endpoint.h:28: IPEndPoint(const IPEndPoint& endpoint);
> While you're here, mind adding explicit to the last constructor?

Sure.  Will do it on the main CL.

[Peter Kasting, 2014-11-20 00:39:56]

On 2014/11/19 22:41:28, Peter Kasting wrote:
> On 2014/11/19 15:29:22, mmenke wrote:
> > LGTM, thanks for doing this!
> > 
> > https://codereview.chromium.org/718273002/diff/60001/net/base/ip_endpoint.h
> > File net/base/ip_endpoint.h (right):
> > 
> >
>
https://codereview.chromium.org/718273002/diff/60001/net/base/ip_endpoint.h#n...
> > net/base/ip_endpoint.h:28: IPEndPoint(const IPEndPoint& endpoint);
> > While you're here, mind adding explicit to the last constructor?
> 
> Sure.  Will do it on the main CL.

Update -- I should have realized this isn't right, it's the copy constructor, so
making it explicit breaks e.g. returning IPEndPoint by value from functions. 
Reverting the change (again, on the main CL).

[mmenke, 2014-11-20 06:07:06]

On 2014/11/20 00:39:56, Peter Kasting wrote:
> On 2014/11/19 22:41:28, Peter Kasting wrote:
> > On 2014/11/19 15:29:22, mmenke wrote:
> > > LGTM, thanks for doing this!
> > > 
> > >
https://codereview.chromium.org/718273002/diff/60001/net/base/ip_endpoint.h
> > > File net/base/ip_endpoint.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/718273002/diff/60001/net/base/ip_endpoint.h#n...
> > > net/base/ip_endpoint.h:28: IPEndPoint(const IPEndPoint& endpoint);
> > > While you're here, mind adding explicit to the last constructor?
> > 
> > Sure.  Will do it on the main CL.
> 
> Update -- I should have realized this isn't right, it's the copy constructor,
so
> making it explicit breaks e.g. returning IPEndPoint by value from functions. 
> Reverting the change (again, on the main CL).

Oops, sorry about that.