Move some net::IPAddressNumber implementations to net::IPAddress.

Move some net::IPAddressNumber implementations to net::IPAddress.

  - IsIPAddressReserved -> IPAddress::IsReserved
  - IPNumberMatchesPrefix -> IPAddressMatchesPrefix

BUG=496258
Committed: https://crrev.com/eb485a96ca5bb8e557a2c5f1f65b3c68c0036942
Cr-Commit-Position: refs/heads/master@{#387033}

[martijnc, 2016-04-11 20:41:08]

Patchset #1 (id:1) has been deleted

[martijnc, 2016-04-11 21:33:24]

Patchset #1 (id:20001) has been deleted

[martijnc, 2016-04-11 21:53:16]

martijn@martijnc.be changed reviewers:
+ eroman@chromium.org

[martijnc, 2016-04-11 21:54:49]

Description was changed from

==========
Move some net::IPAddressNumber implementations to net::IPAddress.

BUG=496258
==========

to

==========
Move some net::IPAddressNumber implementations to net::IPAddress.

  - IsIPAddressReserved -> IPAddress::IsReserved
  - IPNumberMatchesPrefix -> IPAddressMatchesPrefix

BUG=496258
==========

[martijnc, 2016-04-11 22:02:09]

Can you take a look?

Thanks!

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

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address.cc#...
net/base/ip_address.cc:110: static const unsigned char kReservedIPv6[][17] = {
I copied this list but while trying to write tests for this method I wasn't
quite able to figure out how these ranges match those in [1]. Not sure what the
precise definition of 'reserved' is here.

I'm probably missing something but 0100::/8, 0200::/7, 0400::/6, 0800::/5 &
1000::/4 don't seem to match any of these ranges [2]?

[1] http://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml
[2]
https://docs.google.com/spreadsheets/d/1zRoJYtk3aM-QxRz-d_hYLICcXWSc3PKS9OWkA...

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address_uni...
File net/base/ip_address_unittest.cc (right):

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:80: TEST(IPAddressTest, IsReserved) {
Added these tests, the IPv6 addresses are from [1] which was (indirectly)
testing this code. But since the method is used in other places I though it
would be good to tests here as well.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/url_util_...

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:283: TEST(IPAddressTest, IPAddressMatchesPrefix)
{
Tests from net/base/ip_address_number_unittest.cc.

[eroman, 2016-04-12 05:30:56]

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

[eroman, 2016-04-12 05:30:56]

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

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address.cc#...
net/base/ip_address.cc:110: static const unsigned char kReservedIPv6[][17] = {
On 2016/04/11 22:02:08, martijnc wrote:
> I copied this list but while trying to write tests for this method I wasn't
> quite able to figure out how these ranges match those in [1]. Not sure what
the
> precise definition of 'reserved' is here.
> 
> I'm probably missing something but 0100::/8, 0200::/7, 0400::/6, 0800::/5 &
> 1000::/4 don't seem to match any of these ranges [2]?
> 
> [1]
http://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml
> [2]
>
https://docs.google.com/spreadsheets/d/1zRoJYtk3aM-QxRz-d_hYLICcXWSc3PKS9OWkA...

Thanks for raising this issue Martin!

Yeah, those ranges do seem to be missing reserved blocks unless I am reading
this wrong.

The scarcity of documentation in this function makes it hard to understand what
the author's intent was.

+rsleevi who may know more.

I expect we can simplify this by storing the pattern for non-reserved ranges
rather than the patterns for all the private address space. I think just these
two mappings:

  2000::/3 (unicast)
  ff00::/8 (multicast)

If it doesn't match one of those two patterns, then the function can return
true. This covers all the reserved ranges as well as Unique Local Unicast
(fc00::/7) and Link-Scoped Unicast (fe80::/10).

Alternately as a blacklist (using the current approach) if we add up all the
reserved ranges + unique local unicast + link-scoped unicast and simplify you
get:

4000::/2 (01*)                                                                 
8000::/2 (10*)                                                                 
0000::/3 (000*)                                                                
c000::/3 (110*)                                                                
e000::/4 (1110*)                                                               
f000::/5 (11110*)                                                              
f800::/6 (111110*)                                                             
fc00::/7 (1111110*)                                                            
fe00::/8 (11111110*)

[Ryan Sleevi, 2016-04-12 21:15:59]

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

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address.cc#...
net/base/ip_address.cc:110: static const unsigned char kReservedIPv6[][17] = {
On 2016/04/12 05:30:56, eroman wrote:
> Alternately as a blacklist (using the current approach) if we add up all the
> reserved ranges + unique local unicast + link-scoped unicast and simplify you
> get:
> 
> 4000::/2 (01*)                                                                

> 8000::/2 (10*)                                                                

> 0000::/3 (000*)                                                               

> c000::/3 (110*)                                                               

> e000::/4 (1110*)                                                              

> f000::/5 (11110*)                                                             

> f800::/6 (111110*)                                                            

> fc00::/7 (1111110*)                                                           

> fe00::/8 (11111110*)

When the review introducing this started, that's what we had - because the IPv6
ranges were represented as strings - see
https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc...

In Patchset 4, this got rewritten as
https://codereview.chromium.org/22538003/diff/21001/net/base/net_util.cc

Unfortunately, they were lost then.

So it was a bug that was overlooked during review.

[eroman, 2016-04-12 21:40:40]

Thanks Ryan for confirming and pointing to the intent!

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=602773 to cover
this.

Martin: I suggest keeping this CL as a strict refactor, and then doing a
follow-up that fixes the bug you found (if you are interested)

[martijnc, 2016-04-12 21:56:58]

On 2016/04/12 at 21:40:40, eroman wrote:
> Thanks Ryan for confirming and pointing to the intent!
> 
> Filed https://bugs.chromium.org/p/chromium/issues/detail?id=602773 to cover
this.
> 
> Martin: I suggest keeping this CL as a strict refactor, and then doing a
follow-up that fixes the bug you found (if you are interested)

Sounds good. I will follow-up with a separate CL to fix the ranges when this one
lands (feel free to assign that bug to me :)).

[eroman, 2016-04-13 01:36:21]

lgtm

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address_uni...
File net/base/ip_address_unittest.cc (right):

https://codereview.chromium.org/1860823005/diff/40001/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:80: TEST(IPAddressTest, IsReserved) {
On 2016/04/11 22:02:09, martijnc wrote:
> Added these tests, the IPv6 addresses are from [1] which was (indirectly)
> testing this code. But since the method is used in other places I though it
> would be good to tests here as well.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/url_util_...

Agreed, thanks!

I have some suggestions on the IPv6 data to test, but will save those comments
for the bugfix CL (basically we will want to test an address from each
allocations the IANA document lists).

[martijnc, 2016-04-13 16:29:36]

The CQ bit was checked by martijn@martijnc.be

[commit-bot: I haz the power, 2016-04-13 16:29:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860823005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860823005/40001

[commit-bot: I haz the power, 2016-04-13 18:25:17]

Committed patchset #1 (id:40001)

[commit-bot: I haz the power, 2016-04-13 18:26:18]

Description was changed from

==========
Move some net::IPAddressNumber implementations to net::IPAddress.

  - IsIPAddressReserved -> IPAddress::IsReserved
  - IPNumberMatchesPrefix -> IPAddressMatchesPrefix

BUG=496258
==========

to

==========
Move some net::IPAddressNumber implementations to net::IPAddress.

  - IsIPAddressReserved -> IPAddress::IsReserved
  - IPNumberMatchesPrefix -> IPAddressMatchesPrefix

BUG=496258

Committed: https://crrev.com/eb485a96ca5bb8e557a2c5f1f65b3c68c0036942
Cr-Commit-Position: refs/heads/master@{#387033}
==========

[commit-bot: I haz the power, 2016-04-13 18:26:19]

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