Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1886043002:
Update IPAddress::IsReserved() ranges. (Closed)
|
Created:
4 years, 8 months ago by martijnc Modified:
4 years, 8 months ago Reviewers:
eroman CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, grt+watch_chromium.org, jam, Ryan Sleevi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate IPAddress::IsReserved() ranges.
BUG=602773
Committed: https://crrev.com/821620aa901ab5fe90e89734fc1837c401ea8a37
Cr-Commit-Position: refs/heads/master@{#389114}
Patch Set 1 : #
Total comments: 19
Patch Set 2 : comments eroman #Patch Set 3 : fix windows? #
Total comments: 8
Patch Set 4 : comments eroman #Messages
Total messages: 27 (14 generated)
Description was changed from ========== Update IPAddress::IsReserved() ranges and rename to IsGloballyUnique(). BUG=602773 ========== to ========== Update IPAddress::IsReserved() ranges. BUG=602773 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
martijn@martijnc.be changed reviewers: + eroman@chromium.org
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
This Cl moves the IPAddress::IsReserved() logic for IPv6 to a whitelist of (unreserved) ranges instead of the blacklist. This should fix the issue where some reserved ranges were missing. The IPv4 logic still uses the blacklist of reserved ranges. Switching to a whitelist won't do much as the IPv4 address space is much more fragmented. Since the IPv4 and IPv6 logic is now different, I also split up the logic and moved it to two helper functions. I also validated the list of reserved IPv4 ranges in the code against [1] and [2]. Taking the ranges marked RESERVED in [1] and all ranges in [2]; the following ranges are currently not included in the blacklist: 192.0.0.0/24 (including 192.0.0.0/29, 192.0.0.8/32, 192.0.0.9/32, 192.0.0.170/32, 192.0.0.171/32) 192.31.196.0/24 192.52.193.0/24 192.175.48.0/24 255.255.255.255/32 These were not included in the initial patchset [3] of the CL that added these so I assume they are left out intentionally. The method name is unchanged (the bug suggests a rename), not sure what a better name would be (maybe IsGloballyUnique(), if we update all callers to invert the result?). This CL also updates the tests; it tests all reserved ranges by checking the first and last address of each range, assuming they will always be affected when (prefix) bits start falling over. The unreserved blocks in between reserved ranges are tested similarly so the tests should cover the entire IPv4 and IPv6 address ranges. What do you think? [1] http://www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml [2] http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-... [3] https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc
https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:20: int num_entire_bytes_in_prefix = prefix_length_in_bits / 8; side-comment: This existing function is a bit of a mess when it comes to "int" vs "size_t", which could result in truncation/overflow. In practice it is only safe to call this after doing size checks anyway so probably not a reachable problem. Might be nice to clean it up in the future though. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:47: DCHECK(ip_address.size() == net::IPAddress::kIPv4AddressSize); DCHECK_EQ https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:51: } static const kReservedIPv4Ranges[] = { For the IPv4 ranges here, I confirmed that these correspond with the previously recognized ones. As far as whether we are missing some for ranges IPv4, I haven't fully analyzed that data yet. Feel free to proceed with this CL as-is. I will post my analysis of the IPv4 ranges when I get the chance, and if we are missing stuff can add it in a follow-up. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:72: DCHECK(ip_address.size() == net::IPAddress::kIPv6AddressSize); DCHECK_EQ. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:74: const uint8_t address[16]; The largest prefix length we need for the ranges below is just 1 byte. But there is a bit of extra clarity for using 2 bytes. Can you change this to just the prefix portion: const uint8_t address_prefix[2]; And also reword "prefix_length" to "prefix_length_in_bits" https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:77: {{0x20, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 3}, Please add comments to these that identify what they are: // 2000::/3 -- Global Unicast ... // ff00::/8 -- Multicast ... https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:84: matched = true; Can you early return here instead? return false; (No need to check the remaining ranges when one is matched). https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address_uni... File net/base/ip_address_unittest.cc (right): https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address_uni... net/base/ip_address_unittest.cc:87: bool is_reserved; Consider making this an enum, for better readability of the test data. Could then write things like: {"10.255.255.255", RESERVED}, https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address_uni... net/base/ip_address_unittest.cc:186: {"255.255.255.255", true}}; Thanks for adding all these tests!
Patchset #2 (id:100001) has been deleted
https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:20: int num_entire_bytes_in_prefix = prefix_length_in_bits / 8; On 2016/04/18 at 18:13:15, eroman wrote: > side-comment: This existing function is a bit of a mess when it comes to "int" vs "size_t", which could result in truncation/overflow. In practice it is only safe to call this after doing size checks anyway so probably not a reachable problem. Might be nice to clean it up in the future though. Changed to size_t for consistency (think that's the best option here). This still truncates when |prefix_length_in_bits| isn't divisible by 8 but that seems expected. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:47: DCHECK(ip_address.size() == net::IPAddress::kIPv4AddressSize); On 2016/04/18 at 18:13:15, eroman wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:72: DCHECK(ip_address.size() == net::IPAddress::kIPv6AddressSize); On 2016/04/18 at 18:13:15, eroman wrote: > DCHECK_EQ. Done. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:74: const uint8_t address[16]; On 2016/04/18 at 18:13:15, eroman wrote: > The largest prefix length we need for the ranges below is just 1 byte. But there is a bit of extra clarity for using 2 bytes. > > Can you change this to just the prefix portion: > > const uint8_t address_prefix[2]; > > And also reword "prefix_length" to "prefix_length_in_bits" Done & done. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:77: {{0x20, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 3}, On 2016/04/18 at 18:13:15, eroman wrote: > Please add comments to these that identify what they are: > > // 2000::/3 -- Global Unicast > ... > > // ff00::/8 -- Multicast > ... Done. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:84: matched = true; On 2016/04/18 at 18:13:15, eroman wrote: > Can you early return here instead? > return false; > > (No need to check the remaining ranges when one is matched). Done. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address_uni... File net/base/ip_address_unittest.cc (right): https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address_uni... net/base/ip_address_unittest.cc:87: bool is_reserved; On 2016/04/18 at 18:13:15, eroman wrote: > Consider making this an enum, for better readability of the test data. Could then write things like: > > > {"10.255.255.255", RESERVED}, Done.
lgtm https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:16: bool IPAddressPrefixCheck(const std::vector<uint8_t>& ip_address, While you are here, do you mind documenting the implicit assumptions of this function? * ip_address is at least prefix_length_in_bits (bits) long * ip_prefix is at least prefix_length_in_bits (bits) long https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:20: int num_entire_bytes_in_prefix = prefix_length_in_bits / 8; On 2016/04/18 21:39:54, martijnc wrote: > On 2016/04/18 at 18:13:15, eroman wrote: > > side-comment: This existing function is a bit of a mess when it comes to "int" > vs "size_t", which could result in truncation/overflow. In practice it is only > safe to call this after doing size checks anyway so probably not a reachable > problem. Might be nice to clean it up in the future though. > > Changed to size_t for consistency (think that's the best option here). > This still truncates when |prefix_length_in_bits| isn't divisible by 8 but that > seems expected. Correct -- truncation in that case is working as intended. https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address.cc... net/base/ip_address.cc:58: for (const auto& range : kReservedIPv4Ranges) nit: put curly braces around the "for" and "if". This is somewhat open to interpretation in the style guide, but consistent with more //net code. https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address.cc... net/base/ip_address.cc:84: for (const auto& range : kPublicIPv6Ranges) Same here with curly braces {} https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address_un... File net/base/ip_address_unittest.cc (right): https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address_un... net/base/ip_address_unittest.cc:80: enum IPAddressReservedResult : bool { UNRESERVED = false, RESERVED = true }; nit: How about NOT_RESERVED rather than "UNRESERVED" ? https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address_un... net/base/ip_address_unittest.cc:86: TEST(IPAddressTest, IsReserved_IPv4) { nit: Remove the underscore (_) from the name. *Technically* GTEST names should not have underscores [1] We get it wrong in other code, and honestly underscores are hard to cause problems, but may as well just remove it :) [1] https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#si...
Patchset #4 (id:150001) has been deleted
Thank you. https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/1886043002/diff/80001/net/base/ip_address.cc#... net/base/ip_address.cc:16: bool IPAddressPrefixCheck(const std::vector<uint8_t>& ip_address, On 2016/04/18 at 22:52:39, eroman wrote: > While you are here, do you mind documenting the implicit assumptions of this function? > > * ip_address is at least prefix_length_in_bits (bits) long > * ip_prefix is at least prefix_length_in_bits (bits) long Done. https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address.cc File net/base/ip_address.cc (right): https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address.cc... net/base/ip_address.cc:58: for (const auto& range : kReservedIPv4Ranges) On 2016/04/18 at 22:52:39, eroman wrote: > nit: put curly braces around the "for" and "if". > > This is somewhat open to interpretation in the style guide, but consistent with more //net code. Done. https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address.cc... net/base/ip_address.cc:84: for (const auto& range : kPublicIPv6Ranges) On 2016/04/18 at 22:52:39, eroman wrote: > Same here with curly braces {} Done. https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address_un... File net/base/ip_address_unittest.cc (right): https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address_un... net/base/ip_address_unittest.cc:80: enum IPAddressReservedResult : bool { UNRESERVED = false, RESERVED = true }; On 2016/04/18 at 22:52:39, eroman wrote: > nit: How about NOT_RESERVED rather than "UNRESERVED" ? Done. https://codereview.chromium.org/1886043002/diff/120002/net/base/ip_address_un... net/base/ip_address_unittest.cc:86: TEST(IPAddressTest, IsReserved_IPv4) { On 2016/04/18 at 22:52:39, eroman wrote: > nit: Remove the underscore (_) from the name. > > *Technically* GTEST names should not have underscores [1] > > We get it wrong in other code, and honestly underscores are hard to cause problems, but may as well just remove it :) > > [1] https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#si... Oh, didn't know that, thanks. Removed.
lgtm
The CQ bit was checked by martijn@martijnc.be
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886043002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886043002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by martijn@martijnc.be
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886043002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886043002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by martijn@martijnc.be
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886043002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886043002/170001
Message was sent while issue was closed.
Committed patchset #4 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Update IPAddress::IsReserved() ranges. BUG=602773 ========== to ========== Update IPAddress::IsReserved() ranges. BUG=602773 Committed: https://crrev.com/821620aa901ab5fe90e89734fc1837c401ea8a37 Cr-Commit-Position: refs/heads/master@{#389114} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/821620aa901ab5fe90e89734fc1837c401ea8a37 Cr-Commit-Position: refs/heads/master@{#389114} |
