url_canon: New CanonicalizeHostVerbose() function.

url_canon: New CanonicalizeHostVerbose() function.

This function allows the caller to distinguish between 4 cases:
- A valid, canonical IPv4 address.
- A valid, canonical IPv6 address.
- A "broken" address.  This is something which kinda almost looks like an IP
  address, but has been deemed to be invalid.  For IPv4, this indicates a
  numerical component which is longer than 16 digits, or one which can't be
  used without truncation/overflow.  For IPv6, this is any non-parseable
  address containing a colon or square brackets.
- "neutral".  This indicates that the output doesn't really look like an
  IP address at all, and should probably be treated as a hostname.

The CanonHostInfo struct is meant to be extensible.  In addition to the 4 cases
above, it also returns the number of components used to make an IPv4 address,
which will be useful to Chromium, and will make it possible stop exposing
FindIPv4Components.

This is the first step toward making Chrome behave better for these cases:
- http://[www.google.com]/
- http://192.168.0.0000000000000000000000000001/
- http://192.168.0.99999/

Edit:
- Changed the signature of CanonicalizeIPAddress(), instead of creating
  a new "Verbose" version.
- For completeness, now includes the CanonicalizeHost() changes,
  with an extended-output CanonicalizeHostVerbose(), because some callers
  only care whether the canonicalization was a success or not.

[eroman, 2009-05-26 17:59:05]

I generally like the CanonicalizeIPAddressVerbose() function and how it
distinguishes between {IPV4, IPV6, UNKNOWN, BROKEN}. But must defer this design
to Brett.

Some high-level comments:

- I don't like how there are both CanonicalizeIPAddressVerbose and
CanonicalizeIPAddress. Why not just have one function for doing the IP
canonicalization (i.e. non-backwards compatible API change)?

- AFAIK the truncation behavior with respect to IPv4 components was intentional.
What is the motivation behind changing the behavior?

[pmarks, 2009-05-26 18:12:08]

On 2009/05/26 17:59:05, eroman wrote:
> I generally like the CanonicalizeIPAddressVerbose() function and how it
> distinguishes between {IPV4, IPV6, UNKNOWN, BROKEN}. But must defer this
design
> to Brett.
> 
> Some high-level comments:
> 
> - I don't like how there are both CanonicalizeIPAddressVerbose and
> CanonicalizeIPAddress. Why not just have one function for doing the IP
> canonicalization (i.e. non-backwards compatible API change)?

Oh, I had assumed backwards compatibility was necessary.  I would also prefer to
just replace the existing function, if that's okay.

> - AFAIK the truncation behavior with respect to IPv4 components was
intentional.
> What is the motivation behind changing the behavior?

I figured the truncation was just an artifact of not wanting to mark
almost-IP-addresses as hostnames.  Now that an IP can be marked as broken,
there's no point in munging questionable input.  Or can you think of a case
where truncation is still useful?

[pmarks, 2009-05-27 21:39:02]

Brett, are you around?  Would you support making this a non-compatible API
change?

We're also going to need to extend CanonicalizeHost() to export the CanonIPInfo
struct, so we face the same issue there: change the API, or add a "verbose"
CanonicalizeHost?

[brettw, 2009-05-27 22:01:25]

Can you give some context for this change? Where will it be called? If this is
from the main URL canonicalization function, I'd like to get that in at the same
time (url_canon_stdurl.cc).

For your examples, what are you expecting the browser to do differently for
things like 192.168.0.0000000001?

[brettw, 2009-05-27 22:01:54]

I just noticed the other CL, I'll look at it and get back to you.

[pmarks, 2009-05-27 22:12:30]

On 2009/05/27 22:01:54, brettw wrote:
> I just noticed the other CL, I'll look at it and get back to you.

http://codereview.chromium.org/113696/diff/20/1019 is just a quick preview of
how this change can be used.

I also plan to make CanonicalizeHost export the CanonIPInfo struct, so that (for
example) Chrome will be able to identify whether the host was an IP address, and
whether an IPv4 address contained all 4 components.

If you want to do everything at once, I can flesh out my CanonicalizeHost
changes and add them to this review.

[pmarks, 2009-05-27 22:18:20]

On 2009/05/27 22:01:25, brettw wrote:
> For your examples, what are you expecting the browser to do differently for
> things like 192.168.0.0000000001?

Prior to my change, 192.168.000<lots>1 gets tagged as "not an IP address"
because the last component is too long.  In Chrome, you can still use that as an
IP, but it bypasses canonicalization.

After my change(s), addresses with too-long components will get marked as BROKEN
instead.

A subtlety to note is, 192.168.0.00000000000000001 should be marked as BROKEN,
but 192.168.0.00000000000000001foo should be treated as a hostname because it's
non-numeric.

[pmarks, 2009-05-27 22:30:32]

On Wed, May 27, 2009 at 3:15 PM, Brett Wilson <brettw@chromium.org> wrote:
> On Wed, May 27, 2009 at 3:12 PM,  <pmarks@google.com> wrote:
>> On 2009/05/27 22:01:54, brettw wrote:
>>>
>>> I just noticed the other CL, I'll look at it and get back to you.
>>
>> http://codereview.chromium.org/113696/diff/20/1019 is just a quick
>> preview of how this change can be used.
>>
>> I also plan to make CanonicalizeHost export the CanonIPInfo struct, so
>> that (for example) Chrome will be able to identify whether the host was
>> an IP address, and whether an IPv4 address contained all 4 components.
>
> We already have this with GURL.HostIsIPAddress, and the
> canonicalization will normally make all 4 components. What is the
> use-case for the extra function?

It's a matter of efficiency.  See these two snippets from Chromium:

http://www.google.com/codesearch/p?hl=en#h0RrPvyPu-c/chrome/browser/autocompl...
file:autocomplete.cc&l=183

http://www.google.com/codesearch/p?hl=en#YwaaF8DorRY/src/net/base/net_util.cc...

To summarize, Chrome's autocomplete does this:
1. Canonicalize the hostname (this also canonicalizes IPs)
2. Canonicalize the IP again, to find out if it was really an IP.
3. Test for a square bracket, to infer whether the IP was an IPv6 address.
4. Call FindIPv4Components(), to find out if the IPv4 address actually contained
4 components.

Basically, the IP address is getting parsed 3+ times.  By exporting the
information that callers actually need to know, we can get rid of these
repetitive calls.

[brettw, 2009-05-27 22:39:33]

http://codereview.chromium.org/114050/diff/1015/2013
File src/url_canon.h (right):

http://codereview.chromium.org/114050/diff/1015/2013#newcode401
Line 401: void CanonicalizeIPAddressVerbose(const char16* spec,
I'm starting to understand this better.

In what cases will we need all the output from CanonIPInfo? The canonicalizer
will make all IPv4 components for you, so it seems like the result will always
be 4, and you don't use this stuff in the patch.

Can it just return the enum?

[pmarks, 2009-05-27 23:05:05]

http://codereview.chromium.org/114050/diff/1015/2013
File src/url_canon.h (right):

http://codereview.chromium.org/114050/diff/1015/2013#newcode401
Line 401: void CanonicalizeIPAddressVerbose(const char16* spec,
On 2009/05/27 22:39:33, brettw wrote:
> I'm starting to understand this better.
> 
> In what cases will we need all the output from CanonIPInfo?

num_ipv4_components tells you how many components were in the input text. 
Chrome's autocomplete won't treat a string of numbers as an IPv4 addresss,
unless it contains all four components.  Exporting this value means Chrome won't
have to call the (formerly private) FindIPv4Components() function.

I moved out_host from a parameter into the struct, because it's usually
redundant, and this way it's easier for callers to ignore.

> The canonicalizer
> will make all IPv4 components for you, so it seems like the result will always
> be 4, and you don't use this stuff in the patch.

The output will always have 4 components, but the input can vary from 1 to 4.

> Can it just return the enum?

That *could* work, but the struct also gives us some future extensibility
without breaking the API.  For example, if we ever thought of a case where the
caller wants to know if a hostname contained escaped characters, or if it's an
IDN, we could just add it to the struct.

Actually, I'm considering changing CanonIPInfo -> CanonHostInfo, and UNKNOWN ->
HOSTNAME, because CanonicalizeHost() will need to export the struct also, and it
may be useful to make modifications on the way out.

For example, if you want to ban the formation of IP addresses from escape
sequences (like %3192.168.0.1), then we can just add a blurb to the end of
CanonicalizeHost() which says "if CanonicalizeIP reports IPV4 or IPV6, but the
original input contained escape characters, then set the family to BROKEN".

[pmarks, 2009-05-28 09:11:51]

Patch set 6 now contains the full collection of changes on the googleurl side.

You can see here how it fits in on the Chromium side:
http://codereview.chromium.org/113944

I've run into a couple issues, though:

1. AutocompleteInput::Parse, when encountering http://[google.com]/, returns
'URL' before testing whether the hostname is valid.  Somehow Chrome still treats
it as a search query instead of a URL, but I'm not sure how at the moment.

http://www.google.com/codesearch/p?hl=en#h0RrPvyPu-c/chrome/browser/autocompl...

2. chrome/browser/extensions/extensions_service_unittest.cc is now failing,
because it uses a URL with a long sequence of 0's in it:

http://www.google.com/codesearch/p?hl=en#h0RrPvyPu-c/chrome/browser/extension...

I'm a bit concerned about this.  It may indicate that the broken IP detection is
applying to a wider scope than it should be.

[pmarks, 2009-05-28 21:07:36]

Brett, are you familiar with what's going on in extensions_service_unittest, or
should I ask about this on a mailing list instead?

[pmarks, 2009-05-28 22:48:27]

That 16-char IPv4 component limit seemed a bit artificial, so I made it
unnecessary in patch set 7-8.

extensions_service_unittest still fails, but now the reason for failure is more
explicit:

/home/paul/src/chromium/chromium/src/chrome/browser/extensions/extensions_service_unittest.cc:454:
Failure
Value of: loaded_[0]->url().spec()
  Actual: "chrome-extension://0.0.0.0/"
Expected: "chrome-extension://0000000000000000000000000000000000000000/"

Does someone else want to fix that part of the code?  I really just dropped in
to add IPv6 literals support, not to rewrite the whole browser...

[pmarks, 2009-06-14 07:29:05]

Aaron Boodman has fixed the numeric chrome-extension problem:
http://codereview.chromium.org/126074

We can continue with this review now.

[pmarks, 2009-06-17 19:49:32]

On 2009/06/14 07:29:05, pmarks wrote:
> Aaron Boodman has fixed the numeric chrome-extension problem:
> http://codereview.chromium.org/126074
> 
> We can continue with this review now.

ping?

[brettw, 2009-06-18 03:04:20]

http://codereview.chromium.org/114050/diff/1043/2032
File src/url_canon_ip.cc (right):

http://codereview.chromium.org/114050/diff/1043/2032#newcode143
Line 143: // Put the component, minus any base prefix, into a NUL-terminated
buffer so
We usually say NULL instead of NUL.

http://codereview.chromium.org/114050/diff/1043/2032#newcode153
Line 153: const char input = static_cast<char>(spec[i]);
I wouldn't use "const" here.

http://codereview.chromium.org/114050/diff/1043/2032#newcode159
Line 159: // Fill the buffer, if there's space remaining.
Can you comment that we do this so we can validate all the characters even if it
doesn't fit in the buffer?

http://codereview.chromium.org/114050/diff/1043/2032#newcode165
Line 165: buf[dest_i] = '\0';
I don't see the point of changing 0 to '\0'. I prefer 0, and '\0' is just a
number anyway. Also, the comment seems unnecessary to me.

http://codereview.chromium.org/114050/diff/1043/2032#newcode169
Line 169: const uint64_t num = _strtoui64(buf, NULL, BaseForType(base));
...or here.

http://codereview.chromium.org/114050/diff/1043/2032#newcode209
Line 209: // If |num_ipv4_components| is non-NULL, it will be populated on
success.
I kind of hate optional output arguments. Since this is only used once, it would
probably be more efficient and less lines of code to just require it and have a
var at the callsite that you pass in the address of.

http://codereview.chromium.org/114050/diff/1043/2032#newcode214
Line 214: int *num_ipv4_components) {
The * should go on the type to match the rest of the code.

http://codereview.chromium.org/114050/diff/1043/2032#newcode227
Line 227: const CanonHostInfo::Family family = IPv4ComponentToNumber(
No const.

http://codereview.chromium.org/114050/diff/1043/2032#newcode249
Line 249: for (int i = 3; i >= existing_components - 1; i--) {
Your algorithm of not truncating nor allowing the remaining IPV4 violates the
RFC that I can't find anymore.

http://codereview.chromium.org/114050/diff/1043/2032#newcode693
Line 693: spec, host, output, host_info) ||
I find this notation of using the || here very difficult to notice. Can you use
a normal if statement?

http://codereview.chromium.org/114050/diff/1043/2032#newcode702
Line 702: DoCanonicalizeIPv4Address<char16, char16>(spec, host, output,
host_info) ||
Same here

[pmarks, 2009-06-18 20:17:19]

See Patch Set 9.

http://codereview.chromium.org/114050/diff/1043/2032
File src/url_canon_ip.cc (right):

http://codereview.chromium.org/114050/diff/1043/2032#newcode143
Line 143: // Put the component, minus any base prefix, into a NUL-terminated
buffer so
On 2009/06/18 03:04:20, brettw wrote:
> We usually say NULL instead of NUL.

NUL is the typical notation for ASCII 0, but I won't bother arguing.  Fixed.

http://codereview.chromium.org/114050/diff/1043/2032#newcode153
Line 153: const char input = static_cast<char>(spec[i]);
On 2009/06/18 03:04:20, brettw wrote:
> I wouldn't use "const" here.

Done.

http://codereview.chromium.org/114050/diff/1043/2032#newcode159
Line 159: // Fill the buffer, if there's space remaining.
On 2009/06/18 03:04:20, brettw wrote:
> Can you comment that we do this so we can validate all the characters even if
it
> doesn't fit in the buffer?

Done.

http://codereview.chromium.org/114050/diff/1043/2032#newcode165
Line 165: buf[dest_i] = '\0';
On 2009/06/18 03:04:20, brettw wrote:
> I don't see the point of changing 0 to '\0'. I prefer 0, and '\0' is just a
> number anyway. Also, the comment seems unnecessary to me.

Removed the superfluous comment.  I'm leaving '\0' alone, because the Google C++
Style Guide says "Use 0 for integers, 0.0 for reals, NULL for pointers, and '\0'
for chars."

http://codereview.chromium.org/114050/diff/1043/2032#newcode169
Line 169: const uint64_t num = _strtoui64(buf, NULL, BaseForType(base));
On 2009/06/18 03:04:20, brettw wrote:
> ...or here.

Done.

http://codereview.chromium.org/114050/diff/1043/2032#newcode209
Line 209: // If |num_ipv4_components| is non-NULL, it will be populated on
success.
On 2009/06/18 03:04:20, brettw wrote:
> I kind of hate optional output arguments. Since this is only used once, it
would
> probably be more efficient and less lines of code to just require it and have
a
> var at the callsite that you pass in the address of.

Done.

http://codereview.chromium.org/114050/diff/1043/2032#newcode214
Line 214: int *num_ipv4_components) {
On 2009/06/18 03:04:20, brettw wrote:
> The * should go on the type to match the rest of the code.

Done.

http://codereview.chromium.org/114050/diff/1043/2032#newcode227
Line 227: const CanonHostInfo::Family family = IPv4ComponentToNumber(
On 2009/06/18 03:04:20, brettw wrote:
> No const.

Done.

http://codereview.chromium.org/114050/diff/1043/2032#newcode249
Line 249: for (int i = 3; i >= existing_components - 1; i--) {
On 2009/06/18 03:04:20, brettw wrote:
> Your algorithm of not truncating nor allowing the remaining IPV4 violates the
> RFC that I can't find anymore.

Can you be more specific about what you don't like?  This algorithm allows
"4.0xffffff", but disallows "4.0x1000000".  I think it makes perfect sense, and
I'd need a stronger argument to change it.  Fwiw, "ping" command on my Linux box
behaves the same way.

http://codereview.chromium.org/114050/diff/1043/2032#newcode693
Line 693: spec, host, output, host_info) ||
On 2009/06/18 03:04:20, brettw wrote:
> I find this notation of using the || here very difficult to notice. Can you
use
> a normal if statement?

Changed.  This new way is slightly more verbose than it needs to be, but it
keeps things looking symmetric.

http://codereview.chromium.org/114050/diff/1043/2032#newcode702
Line 702: DoCanonicalizeIPv4Address<char16, char16>(spec, host, output,
host_info) ||
On 2009/06/18 03:04:20, brettw wrote:
> Same here

Done.

[brettw, 2009-06-18 21:24:01]

I'll get to the rest soon.

http://codereview.chromium.org/114050/diff/1043/2032
File src/url_canon_ip.cc (right):

http://codereview.chromium.org/114050/diff/1043/2032#newcode249
Line 249: for (int i = 3; i >= existing_components - 1; i--) {
Sorry, I meant to remove this comment. I wrote it, did some research, and
decided your behavior was OK.

[brettw, 2009-06-22 19:21:53]

LGTM

[pmarks, 2009-06-22 20:30:48]

Committed to googleurl:
http://code.google.com/p/google-url/source/detail?r=107

Next, I'll work on the CL to merge this into Chromium.