Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22538003:
Add IP address handling to net::IsHostnameNonUnique (Closed)
|
Created:
7 years, 4 months ago by felt Modified:
7 years, 4 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds a new method, net::IsIPAddressReserved, that returns true if an IP address
belongs to a reserved IANA range. This includes *all* reserved IANA ranges. Applies to both IPv4 and IPv6.
I used the following IANA links to decide whether a given range is reserved:
https://www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xml
https://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xml
BUG=225570, 119212, 116838
TEST=net_unittest
R=rsleevi@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219193
Patch Set 1 #Patch Set 2 : Switched from private to all reserved, separated ipv4 and ipv6 #Patch Set 3 : Adding logging statements to debug linux asan failure #
Total comments: 12
Patch Set 4 : Rewrote IsIPAddressReserved to avoid conversions #Patch Set 5 : Removed logging statement that was there for trybot testing #Patch Set 6 : Made arrays const #
Total comments: 7
Patch Set 7 : Rewritten with the prefix check as its own helper method #
Total comments: 17
Patch Set 8 : Take 3 #
Total comments: 15
Patch Set 9 : mods #Patch Set 10 : Changes to the comment #
Total comments: 6
Patch Set 11 : Added test ases #Patch Set 12 : Rebased #Patch Set 13 : Looking for broken android tests #Patch Set 14 : Disabled internal network check on Android #
Messages
Total messages: 33 (0 generated)
Hi Ryan, can you please review this? Note: 676768 (which you included in your test cases) seems to be a valid IP address. It also matches 0.0.0.0/8. Did you expect that? It is surprising to me and I'm uncertain whether it's a bug. Adrienne
https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1438: if (!ParseIPLiteralToNumber(hostname, &host_addr)) return false; nit: linebreak for the if. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1452: }; List the sources for these. eg: http://www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-... https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1457: "fec0::/10" List the sources for this data: http://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml Also, find your minimal ranges - eg: 0000::/8 contains ::1/128, so there's no need for the subset check. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1458: }; Rather than storing these as strings (and re-parsing on every single function call), we should be storing these in their parsed bitmask forms. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.h#newco... net/base/net_util.h:122: NET_EXPORT bool IsIPAddressReserved(const std::string& address, bool ipv4); std::string -> base::StringPiece (so it can easily be used with IPAddressNumber if needed)? https://codereview.chromium.org/22538003/diff/14001/net/base/net_util_unittes... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/22538003/diff/14001/net/base/net_util_unittes... net/base/net_util_unittest.cc:3526: //{ true, "676768" }, apparently this is valid & matches 0.0.0.0/8? This was just me banging on the keyboard.
Hi Ryan, I rewrote this to use char arrays with no conversions. It should be faster now. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1438: if (!ParseIPLiteralToNumber(hostname, &host_addr)) return false; On 2013/08/07 19:04:44, Ryan Sleevi wrote: > nit: linebreak for the if. Done. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1452: }; On 2013/08/07 19:04:44, Ryan Sleevi wrote: > List the sources for these. > > eg: > http://www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml > http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-... Done. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1457: "fec0::/10" On 2013/08/07 19:04:44, Ryan Sleevi wrote: > List the sources for this data: > > http://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml Done > Also, find your minimal ranges - eg: 0000::/8 contains ::1/128, so there's no > need for the subset check. Done. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.cc#newc... net/base/net_util.cc:1458: }; On 2013/08/07 19:04:44, Ryan Sleevi wrote: > Rather than storing these as strings (and re-parsing on every single function > call), we should be storing these in their parsed bitmask forms. Done. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/14001/net/base/net_util.h#newco... net/base/net_util.h:122: NET_EXPORT bool IsIPAddressReserved(const std::string& address, bool ipv4); On 2013/08/07 19:04:44, Ryan Sleevi wrote: > std::string -> base::StringPiece (so it can easily be used with IPAddressNumber > if needed)? I actually changed this to take an IPAddressNumber, since that seems the most consistent with the rest of net_util. https://codereview.chromium.org/22538003/diff/14001/net/base/net_util_unittes... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/22538003/diff/14001/net/base/net_util_unittes... net/base/net_util_unittest.cc:3526: //{ true, "676768" }, apparently this is valid & matches 0.0.0.0/8? On 2013/08/07 19:04:44, Ryan Sleevi wrote: > This was just me banging on the keyboard. ack
https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc#newc... net/base/net_util.cc:1410: if (!ParseIPLiteralToNumber(hostname, &host_addr)) CanonicalizeHost is going to put the canonical IP address into host_info.out_host , which seems to be easier to avoid canonicalization issues when passing back to ParseIPLiteral. Just FWIW. See https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon_host... https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc#newc... net/base/net_util.cc:1469: size_t arr_size = (ipv4) ? So, my main concern with the ParseCIDR duplication. It definitely seems like IPNumberMatchesPrefix and this should share the same code. You're already controlling to ensure that |host_addr| will always match v4 or v6. Minimally, it seems useful to move the match functionality into common code. This further eliminates the need for constant ipv4 / ipv6 switching. https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.h#newco... net/base/net_util.h:123: NET_EXPORT bool IsIPAddressReserved(const IPAddressNumber& address, bool ipv4); If using |IPAddressNumber|, then we just use kIPv4AddressSize / k IPv6AddressSize, and no need for the bool.
https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc#newc... net/base/net_util.cc:1469: size_t arr_size = (ipv4) ? On 2013/08/08 19:49:31, Ryan Sleevi wrote: > So, my main concern with the ParseCIDR duplication. > > It definitely seems like IPNumberMatchesPrefix and this should share the same > code. You're already controlling to ensure that |host_addr| will always match v4 > or v6. > > Minimally, it seems useful to move the match functionality into common code. > This further eliminates the need for constant ipv4 / ipv6 switching. I can do this, but I'll have to convert all of the const arrays into vectors, which is essentially what the ParseCIDR code does (so then we'd might as well go back to the older version).
On 2013/08/08 19:52:47, felt wrote: > https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc > File net/base/net_util.cc (right): > > https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc#newc... > net/base/net_util.cc:1469: size_t arr_size = (ipv4) ? > On 2013/08/08 19:49:31, Ryan Sleevi wrote: > > So, my main concern with the ParseCIDR duplication. > > > > It definitely seems like IPNumberMatchesPrefix and this should share the same > > code. You're already controlling to ensure that |host_addr| will always match > v4 > > or v6. > > > > Minimally, it seems useful to move the match functionality into common code. > > This further eliminates the need for constant ipv4 / ipv6 switching. > > I can do this, but I'll have to convert all of the const arrays into vectors, > which is essentially what the ParseCIDR code does (so then we'd might as well go > back to the older version). ParseCIDR does ParseIPLiteralToNumber to do string parsing, which requires canonicalization, which is messy. That's what I'm trying to avoid. However, you can easily extract this into a common template function that takes a char* and size_t for len, for which you provide &IPAddressNumber[0] / IPAddressNumber.size() or the array pointers and array[i][expected_ip_size + 1], which won't involve any conversion, and keep the masking common.
https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc#newc... net/base/net_util.cc:1410: if (!ParseIPLiteralToNumber(hostname, &host_addr)) On 2013/08/08 19:49:31, Ryan Sleevi wrote: > CanonicalizeHost is going to put the canonical IP address into > host_info.out_host , which seems to be easier to avoid canonicalization issues > when passing back to ParseIPLiteral. Just FWIW. > > See > https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon_host... Done. https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc#newc... net/base/net_util.cc:1469: size_t arr_size = (ipv4) ? On 2013/08/08 19:52:47, felt wrote: > On 2013/08/08 19:49:31, Ryan Sleevi wrote: > > So, my main concern with the ParseCIDR duplication. > > > > It definitely seems like IPNumberMatchesPrefix and this should share the same > > code. You're already controlling to ensure that |host_addr| will always match > v4 > > or v6. > > > > Minimally, it seems useful to move the match functionality into common code. > > This further eliminates the need for constant ipv4 / ipv6 switching. > > I can do this, but I'll have to convert all of the const arrays into vectors, > which is essentially what the ParseCIDR code does (so then we'd might as well go > back to the older version). Done. https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.h#newco... net/base/net_util.h:123: NET_EXPORT bool IsIPAddressReserved(const IPAddressNumber& address, bool ipv4); On 2013/08/08 19:49:31, Ryan Sleevi wrote: > If using |IPAddressNumber|, then we just use kIPv4AddressSize / k > IPv6AddressSize, and no need for the bool. Done.
On 2013/08/08 19:57:20, Ryan Sleevi wrote: > On 2013/08/08 19:52:47, felt wrote: > > https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc > > File net/base/net_util.cc (right): > > > > > https://codereview.chromium.org/22538003/diff/26001/net/base/net_util.cc#newc... > > net/base/net_util.cc:1469: size_t arr_size = (ipv4) ? > > On 2013/08/08 19:49:31, Ryan Sleevi wrote: > > > So, my main concern with the ParseCIDR duplication. > > > > > > It definitely seems like IPNumberMatchesPrefix and this should share the > same > > > code. You're already controlling to ensure that |host_addr| will always > match > > v4 > > > or v6. > > > > > > Minimally, it seems useful to move the match functionality into common code. > > > This further eliminates the need for constant ipv4 / ipv6 switching. > > > > I can do this, but I'll have to convert all of the const arrays into vectors, > > which is essentially what the ParseCIDR code does (so then we'd might as well > go > > back to the older version). > > ParseCIDR does ParseIPLiteralToNumber to do string parsing, which requires > canonicalization, which is messy. That's what I'm trying to avoid. > > However, you can easily extract this into a common template function that takes > a char* and size_t for len, for which you provide &IPAddressNumber[0] / > IPAddressNumber.size() or the array pointers and array[i][expected_ip_size + 1], > which won't involve any conversion, and keep the masking common. I use templating so rarely that I forget about it! This does look much nicer now.
Hi Ryan, do you think you could take a quick peek this afternoon and indicate if you think it's ready (or going to take more work)? If this version of the patch looks nice I'd love to send it to the CQ tonight so that it can go out with M30. If not, so it goes. Thanks!
Looking much better - but there's an opportunity for even more reduction (and eliminating the templates entirely). I had suggested the template if you wanted to have the loop w/in the template, but it's just as fine to have the loop outside - in which case, a simple function call will do. See below. Hopefully I haven't made any typos. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1440: template<typename Arr, typename Length> You can treat length as a size_t and let integer promotion promote it from an unsigned char to a size_t. Avoids the template specialization. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1442: Arr ip_prefix, This doesn't match your definition in the header (although you'll be removing that). However, you don't need to template on |Arr| here either, because it's always a "const unsigned char*" (IPAddressNumber == 'unsigned char') https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1472: const unsigned char reserved_ipv4[][5] = { nit: static const unsigned char kReservedIPv4 https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1478: const unsigned char reserved_ipv6[][17] = { nit: static const unsigned char kReservedIPv6 https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1505: } FWIW, you could rewrite this as size_t array_size = 0; const char* const* array = NULL; switch (host_addr.size()) { case kIPv4AddressSize: array_size = arraysize(kReservedIPv4); array = kReservedIPv4; break; case kIPv6AddressSize: array_size = arraysize(kReservedIPv6); array = kReservedIPv6; break; } if (!array) return false; for (size_t i = 0; i < array_size; ++i) { if (IPNumberPrefixCheck(host_addr, array[i], array[i][host_addr.size()])) return true; } return false; https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:2080: return IPNumberPrefixCheck(ip_number, ip_prefix, prefix_length_in_bits); Just pass &ip_prefix[0] here - char* goodness, without the templates. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.h#newco... net/base/net_util.h:491: template<typename Arr, typename Length> style nit: "template<" -> "template <" https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.h#newco... net/base/net_util.h:495: const Length prefix_length_in_bits); You can't (safely) forward declare the template like this. It's fine, however, because you don't need to expose this at all - it can be in the unnamed namespace in the implementation file.
https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1505: } On 2013/08/09 22:50:12, Ryan Sleevi wrote: > FWIW, you could rewrite this as > > size_t array_size = 0; > const char* const* array = NULL; > switch (host_addr.size()) { > case kIPv4AddressSize: > array_size = arraysize(kReservedIPv4); > array = kReservedIPv4; > break; > case kIPv6AddressSize: > array_size = arraysize(kReservedIPv6); > array = kReservedIPv6; > break; > } > if (!array) > return false; > for (size_t i = 0; i < array_size; ++i) { > if (IPNumberPrefixCheck(host_addr, array[i], array[i][host_addr.size()])) > return true; > } > return false; and I made a typo :) "const char* const* array" -> "const unsigned char* const* array"
Alas. However, I've had another go. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1440: template<typename Arr, typename Length> On 2013/08/09 22:50:12, Ryan Sleevi wrote: > You can treat length as a size_t and let integer promotion promote it from an > unsigned char to a size_t. Avoids the template specialization. Done. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1442: Arr ip_prefix, On 2013/08/09 22:50:12, Ryan Sleevi wrote: > This doesn't match your definition in the header (although you'll be removing > that). > > However, you don't need to template on |Arr| here either, because it's always a > "const unsigned char*" (IPAddressNumber == 'unsigned char') Done. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1472: const unsigned char reserved_ipv4[][5] = { On 2013/08/09 22:50:12, Ryan Sleevi wrote: > nit: static const unsigned char kReservedIPv4 Done. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1478: const unsigned char reserved_ipv6[][17] = { On 2013/08/09 22:50:12, Ryan Sleevi wrote: > nit: static const unsigned char kReservedIPv6 Done. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:1505: } On 2013/08/09 22:51:31, Ryan Sleevi wrote: > On 2013/08/09 22:50:12, Ryan Sleevi wrote: > > FWIW, you could rewrite this as > > > > size_t array_size = 0; > > const char* const* array = NULL; > > switch (host_addr.size()) { > > case kIPv4AddressSize: > > array_size = arraysize(kReservedIPv4); > > array = kReservedIPv4; > > break; > > case kIPv6AddressSize: > > array_size = arraysize(kReservedIPv6); > > array = kReservedIPv6; > > break; > > } > > if (!array) > > return false; > > for (size_t i = 0; i < array_size; ++i) { > > if (IPNumberPrefixCheck(host_addr, array[i], array[i][host_addr.size()])) > > return true; > > } > > return false; > > and I made a typo :) > > "const char* const* array" -> "const unsigned char* const* array" I didn't use this exactly, but something like it. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.cc#newc... net/base/net_util.cc:2080: return IPNumberPrefixCheck(ip_number, ip_prefix, prefix_length_in_bits); On 2013/08/09 22:50:12, Ryan Sleevi wrote: > Just pass &ip_prefix[0] here - char* goodness, without the templates. Ahhhh. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.h#newco... net/base/net_util.h:491: template<typename Arr, typename Length> On 2013/08/09 22:50:12, Ryan Sleevi wrote: > style nit: "template<" -> "template <" Done. https://codereview.chromium.org/22538003/diff/39001/net/base/net_util.h#newco... net/base/net_util.h:495: const Length prefix_length_in_bits); On 2013/08/09 22:50:12, Ryan Sleevi wrote: > You can't (safely) forward declare the template like this. > > It's fine, however, because you don't need to expose this at all - it can be in > the unnamed namespace in the implementation file. Done.
A few last nits, but I think we're in the home stretch. https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1456: if ((ip_number[i] & mask) != (ip_prefix[i] & mask)) style nit: indentation is off by one here https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1464: // consolidated when possible. Can you rewrite this comment without the "We". Enjoy the nice thread at https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... , but TL;DR rewrite would be: "Don't compare IPv4 and IPv6 against eachother, as they have different range reservations. Treat each IP type as a separate reservation set, with adjacent reserved ranges consolidated when possible." https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1475: { 203,0,113,0,24 }, { 224,0,0,0,3 } style nit: four spaces https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1488: { 0xfe,0xc0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,10 }, style nit: four spaces https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1504: int width = host_addr.size()+1; style nit: "host_addr.size()+1" -> "host_addr.size() + 1" style nit: size_t width (avoid the int coercion) https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1505: for (size_t i = 0; i < array_size*width; i = i + width) { style nit: "array_size*width" -> "array_size * width" "i = i + width" -> "i += width;" But you can rewrite this to avoid the indexing by: for (size_t i = 0; i < array_size; ++i, array += width) { if (IPNumberPrefixCheck(host_addr, array, array[width - 1]) return true; } https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.h#newco... net/base/net_util.h:490: NET_EXPORT_PRIVATE bool IPNumberPrefixCheck( I'm still not sure I've understood why you've added this as NET_EXPORT_PRIVATE - It's not used in any of the tests, nor is it needed outside of net_util.cc Just sticking it within the unnamed namespace should be sufficient, right? The comments describing ip_prefix and prefix_length_in_bits are out of date.
Thanks, updated https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1456: if ((ip_number[i] & mask) != (ip_prefix[i] & mask)) On 2013/08/12 21:11:51, Ryan Sleevi wrote: > style nit: indentation is off by one here Done. https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1464: // consolidated when possible. On 2013/08/12 21:11:51, Ryan Sleevi wrote: > Can you rewrite this comment without the "We". Enjoy the nice thread at > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... > , but TL;DR rewrite would be: > > "Don't compare IPv4 and IPv6 against eachother, as they have different range > reservations. Treat each IP type as a separate reservation set, with adjacent > reserved ranges consolidated when possible." Oh, huh, I hadn't heard that before. Done. https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1475: { 203,0,113,0,24 }, { 224,0,0,0,3 } On 2013/08/12 21:11:51, Ryan Sleevi wrote: > style nit: four spaces Done. https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1488: { 0xfe,0xc0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,10 }, On 2013/08/12 21:11:51, Ryan Sleevi wrote: > style nit: four spaces Done. https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1504: int width = host_addr.size()+1; On 2013/08/12 21:11:51, Ryan Sleevi wrote: > style nit: "host_addr.size()+1" -> "host_addr.size() + 1" > style nit: size_t width (avoid the int coercion) Done. https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1505: for (size_t i = 0; i < array_size*width; i = i + width) { On 2013/08/12 21:11:51, Ryan Sleevi wrote: > style nit: > "array_size*width" -> "array_size * width" > "i = i + width" -> "i += width;" > > But you can rewrite this to avoid the indexing by: > for (size_t i = 0; i < array_size; ++i, array += width) { > if (IPNumberPrefixCheck(host_addr, array, array[width - 1]) > return true; > } is it generally preferred to avoid indexing, or is it just preferred because it's a little shorter...? done https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.h File net/base/net_util.h (right): https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.h#newco... net/base/net_util.h:490: NET_EXPORT_PRIVATE bool IPNumberPrefixCheck( On 2013/08/12 21:11:51, Ryan Sleevi wrote: > I'm still not sure I've understood why you've added this as NET_EXPORT_PRIVATE - > It's not used in any of the tests, nor is it needed outside of net_util.cc > > Just sticking it within the unnamed namespace should be sufficient, right? > > The comments describing ip_prefix and prefix_length_in_bits are out of date. removed it
LGTM with nits. https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/50001/net/base/net_util.cc#newc... net/base/net_util.cc:1505: for (size_t i = 0; i < array_size*width; i = i + width) { On 2013/08/13 04:17:06, felt wrote: > On 2013/08/12 21:11:51, Ryan Sleevi wrote: > > style nit: > > "array_size*width" -> "array_size * width" > > "i = i + width" -> "i += width;" > > > > But you can rewrite this to avoid the indexing by: > > for (size_t i = 0; i < array_size; ++i, array += width) { > > if (IPNumberPrefixCheck(host_addr, array, array[width - 1]) > > return true; > > } > > is it generally preferred to avoid indexing, or is it just preferred because > it's a little shorter...? > > done Shorter and more readable. Forms like (&foo[bar])[baz] are usually a little less readable than the foo[bar + baz] equivalent (which at least in this case works) https://codereview.chromium.org/22538003/diff/63001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/63001/net/base/net_util.cc#newc... net/base/net_util.cc:1440: bool IPNumberPrefixCheck(const IPAddressNumber& ip_number, This function should be moved to ~line 910 (eg: in the unnamed namespace, not as a top-level net:: symbol) https://codereview.chromium.org/22538003/diff/63001/net/base/net_util_unittes... File net/base/net_util_unittest.cc (left): https://codereview.chromium.org/22538003/diff/63001/net/base/net_util_unittes... net/base/net_util_unittest.cc:3522: { true, "1.2.3" }, Note: This was testing the two-dot form, but the replacement is fully qualified. The comment on 3520 was trying to capture the "various forms" that an IP can be represented, and they should all be parsed correctly. If you want to test specifically for the reserved/non-reserved, feel free to add additioanl IPs. https://codereview.chromium.org/22538003/diff/63001/net/base/net_util_unittes... net/base/net_util_unittest.cc:3524: { true, "676768" }, Note: This was testing the dotless form (pure number) of an IPv4 address.
https://codereview.chromium.org/22538003/diff/63001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/22538003/diff/63001/net/base/net_util.cc#newc... net/base/net_util.cc:1440: bool IPNumberPrefixCheck(const IPAddressNumber& ip_number, On 2013/08/13 20:45:39, Ryan Sleevi wrote: > This function should be moved to ~line 910 (eg: in the unnamed namespace, not as > a top-level net:: symbol) Done. https://codereview.chromium.org/22538003/diff/63001/net/base/net_util_unittes... File net/base/net_util_unittest.cc (left): https://codereview.chromium.org/22538003/diff/63001/net/base/net_util_unittes... net/base/net_util_unittest.cc:3522: { true, "1.2.3" }, On 2013/08/13 20:45:39, Ryan Sleevi wrote: > Note: This was testing the two-dot form, but the replacement is fully qualified. > The comment on 3520 was trying to capture the "various forms" that an IP can be > represented, and they should all be parsed correctly. > > If you want to test specifically for the reserved/non-reserved, feel free to add > additioanl IPs. restructured this so that it has all the same test cases in terms of formatting but separated into reserved/unreserved https://codereview.chromium.org/22538003/diff/63001/net/base/net_util_unittes... net/base/net_util_unittest.cc:3524: { true, "676768" }, On 2013/08/13 20:45:39, Ryan Sleevi wrote: > Note: This was testing the dotless form (pure number) of an IPv4 address. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/22538003/69001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/22538003/69001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/22538003/134001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/22538003/134001
On 2013/08/21 21:10:33, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/felt%40chromium.org/22538003/134001 Ryan, did you send this to the CQ? If so, was it a bot flake that was causing the android issues? It looked to me like it might be a legit related failure.
On 2013/08/21 21:12:50, felt wrote: > On 2013/08/21 21:10:33, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/felt%2540chromium.org/22538003/134001 > > Ryan, did you send this to the CQ? Yup > If so, was it a bot flake that was causing > the android issues? It looked to me like it might be a legit related failure. Yes, it looks like bot flake. Either that, or the tests are fundamentally broken. But none of your change would or should be related, that's why I kicked it off again after going through the results.
On 2013/08/21 21:18:35, Ryan Sleevi wrote: > On 2013/08/21 21:12:50, felt wrote: > > On 2013/08/21 21:10:33, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-status.appspot.com/cq/felt%252540chromium.org/22538003/134001 > > > > Ryan, did you send this to the CQ? > > Yup > > > If so, was it a bot flake that was causing > > the android issues? It looked to me like it might be a legit related failure. > > Yes, it looks like bot flake. Either that, or the tests are fundamentally > broken. But none of your change would or should be related, that's why I kicked > it off again after going through the results. Cool, thanks for taking a look. I have been in 4 cities in the last 3 weeks and stuff started piling up...
Doh, I'm a fool. You're running into an issue because the cert is issued to 192.168.0.1, and even though it's a test root, it's being treated as a well-known root. See https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri... To "fix" this, we need to minimally consider whether or not the verified chain used the test root certs. This involves changing https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s... to make that information available (marshalling potentially more than an int across the Java boundary). The information about whether or not we're using a testing cert (which we treat as not a well known CA) is available at https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s... if sDefaultTrustManager is used, well-known CA if sTestTrustManager is used, non well-known CA [There is more we could be doing here, but that's the way it is today] It only triggers on the *real* Android bots, and not the Android Linux testers, because the Android-Linux testers use OpenSSL exclusively. Fun! Incidentally, this means we've also been warning for all user-added CAs in Android about 'dangerous names'. Short-term solution: For OS_ANDROID, fall back to the legacy behaviour and disable the tests. Long-term solution: Let's fix Android. If you're not up for it, palmer@ or I will need to work with ppi@
On 2013/08/22 02:19:52, Ryan Sleevi wrote: > Doh, I'm a fool. > > You're running into an issue because the cert is issued to 192.168.0.1, and even > though it's a test root, it's being treated as a well-known root. > > See > https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri... > > To "fix" this, we need to minimally consider whether or not the verified chain > used the test root certs. > > This involves changing > https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s... > to make that information available (marshalling potentially more than an int > across the Java boundary). > > The information about whether or not we're using a testing cert (which we treat > as not a well known CA) is available at > https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s... > > if sDefaultTrustManager is used, well-known CA > if sTestTrustManager is used, non well-known CA > > [There is more we could be doing here, but that's the way it is today] > > It only triggers on the *real* Android bots, and not the Android Linux testers, > because the Android-Linux testers use OpenSSL exclusively. Fun! > > Incidentally, this means we've also been warning for all user-added CAs in > Android about 'dangerous names'. > > Short-term solution: For OS_ANDROID, fall back to the legacy behaviour and > disable the tests. I'm not sure which option is worse: * Removing the broken lock icon from bunk certs issued by well known CAs * Accidentally showing a broken lock icon from user-added CAs Which population size do you think is bigger? The latter? > Long-term solution: Let's fix Android. If you're not up for it, palmer@ or I > will need to work with ppi@ How long term? Depending on the number of people who add CAs, this seems like it might be urgent enough that someone should fix it right now and merge to 30. I am already overextended for the next month and a half (I'm theoretically splitting my time 50/50 between two teams but it's more like 65/65).
On 2013/08/22 04:00:32, felt wrote: > On 2013/08/22 02:19:52, Ryan Sleevi wrote: > > Doh, I'm a fool. > > > > You're running into an issue because the cert is issued to 192.168.0.1, and > even > > though it's a test root, it's being treated as a well-known root. > > > > See > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri... > > > > To "fix" this, we need to minimally consider whether or not the verified chain > > used the test root certs. > > > > This involves changing > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s... > > to make that information available (marshalling potentially more than an int > > across the Java boundary). > > > > The information about whether or not we're using a testing cert (which we > treat > > as not a well known CA) is available at > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/android/java/s... > > > > if sDefaultTrustManager is used, well-known CA > > if sTestTrustManager is used, non well-known CA > > > > [There is more we could be doing here, but that's the way it is today] > > > > It only triggers on the *real* Android bots, and not the Android Linux > testers, > > because the Android-Linux testers use OpenSSL exclusively. Fun! > > > > Incidentally, this means we've also been warning for all user-added CAs in > > Android about 'dangerous names'. > > > > Short-term solution: For OS_ANDROID, fall back to the legacy behaviour and > > disable the tests. > > I'm not sure which option is worse: > > * Removing the broken lock icon from bunk certs issued by well known CAs > * Accidentally showing a broken lock icon from user-added CAs > > Which population size do you think is bigger? The latter? I should truly, truly hope it's the latter. > > > Long-term solution: Let's fix Android. If you're not up for it, palmer@ or I > > will need to work with ppi@ > > How long term? Depending on the number of people who add CAs, this seems like it > might be urgent enough that someone should fix it right now and merge to 30. I > am already overextended for the next month and a half (I'm theoretically > splitting my time 50/50 between two teams but it's more like 65/65). I don't see us being able to merge it to M30. If anything, I see us disabling the existing name check for well-known CAs on Android and merging that in. That's why I suggested the short-term of #ifdef for OS_ANDROID to keep the legacy behaviour. FWIW, the bug is https://code.google.com/p/chromium/issues/detail?id=116838
Disabled intranet check + test for Android. Running tests now to double check that everything should run green.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/22538003/61003
Message was sent while issue was closed.
Change committed as 219193 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
