Make IP Address related functions return the empty string when used on an invalid address.

Make IP Address related functions return the empty string when used on an invalid address.

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

Committed: https://crrev.com/7246f783ae6b768423ba6caeff7229f3775fd789
Cr-Commit-Position: refs/heads/master@{#376124}

[martijnc, 2016-02-12 21:41:39]

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

[martijnc, 2016-02-14 23:00:50]

Patchset #1 (id:1) has been deleted

[martijnc, 2016-02-15 21:17:16]

Patchset #1 (id:20001) has been deleted

[martijnc, 2016-02-15 21:30:15]

I also updated IPAddressToStringWithPort() and IPEndPoint::ToString() to return
the empty string when called on an invalid address (and ignore the port) to
avoid it returning something like ":1" which might be confusing?

We also could return "invalid" or something instead of the empty string since
there is currently no code that relies on this.

What do you think?

[eroman, 2016-02-16 19:48:52]

lgtm

https://codereview.chromium.org/1692353002/diff/40001/net/base/ip_address_num...
File net/base/ip_address_number.cc (right):

https://codereview.chromium.org/1692353002/diff/40001/net/base/ip_address_num...
net/base/ip_address_number.cc:116: if (address_len != kIPv4AddressSize &&
address_len != kIPv6AddressSize)
How about instead, put the following line after IPAddressToString() invocation
below:

if (address_str.empty())
  return address_str;

This way it is tied more directly to the success/failure of IPAddressToString().
This is useful in case IPAddressToString were extended to support other sizes.

[martijnc, 2016-02-16 22:01:59]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2016-02-16 22:02:00]

Thank you.

https://codereview.chromium.org/1692353002/diff/40001/net/base/ip_address_num...
File net/base/ip_address_number.cc (right):

https://codereview.chromium.org/1692353002/diff/40001/net/base/ip_address_num...
net/base/ip_address_number.cc:116: if (address_len != kIPv4AddressSize &&
address_len != kIPv6AddressSize)
On 2016/02/16 at 19:48:52, eroman wrote:
> How about instead, put the following line after IPAddressToString() invocation
below:
> 
> if (address_str.empty())
>   return address_str;
> 
> This way it is tied more directly to the success/failure of
IPAddressToString(). This is useful in case IPAddressToString were extended to
support other sizes.

Done.

[martijnc, 2016-02-16 22:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1692353002/#ps60001
(title: "comments eroman")

[commit-bot: I haz the power, 2016-02-16 22:04:26]

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

[eroman, 2016-02-16 22:07:04]

lgtm

[commit-bot: I haz the power, 2016-02-16 23:12:28]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-02-16 23:13:39]

Description was changed from

==========
Make IP Address related functions return the empty string when used on an
invalid address.

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

to

==========
Make IP Address related functions return the empty string when used on an
invalid address.

BUG=496258

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

[commit-bot: I haz the power, 2016-02-16 23:13:40]

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

[Nico, 2016-02-17 02:27:43]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-02-17 02:27:43]

Some of the tests fail here:
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28dbg%29%20test...

Is it possible the test is incorrect? See here:

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
File net/base/ip_address_number_unittest.cc (right):

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number_unittest.cc:39: uint8_t addr4[2];
Doesn't this have to be 4 bytes? Also, should it be initialized to something?
Else this has undefined behavior

[martijnc, 2016-02-17 08:29:53]

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
File net/base/ip_address_number_unittest.cc (right):

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number_unittest.cc:39: uint8_t addr4[2];
On 2016/02/17 at 02:27:43, Nico wrote:
> Doesn't this have to be 4 bytes? Also, should it be initialized to something?
Else this has undefined behavior

The length of 2 bytes is correct. This tests if IPAddressToString() returns an
empty string when called with an invalid address. The contents isn't used when
the IP has invalid length.

I'll take a look why these are failing in debug builds when I get home. Should
this be reverted in the meantime?

[henrika (OOO until Aug 14), 2016-02-17 08:58:18]

A revert of this CL (patchset #2 id:60001) has been created in
https://codereview.chromium.org/1708483002/ by henrika@chromium.org.

The reason for reverting is: Suspect that this patch breaks some net_unittests.

See e.g
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....

[eroman, 2016-02-17 16:11:36]

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
File net/base/ip_address_number.cc (right):

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number.cc:128: return IPAddressToString(&addr.front(),
addr.size());
Change &addr.front() to addr.data()

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number.cc:133: return
IPAddressToStringWithPort(&addr.front(), addr.size(), port);
Same thing here: Change &addr.front() to addr.data()

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number.cc:137: return std::string(reinterpret_cast<const
char *>(&addr.front()),
May as well make the change here too, although this won't be directly related to
current change.

[martijnc, 2016-02-17 21:46:55]

Sorry about this.

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
File net/base/ip_address_number.cc (right):

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number.cc:128: return IPAddressToString(&addr.front(),
addr.size());
On 2016/02/17 at 16:11:36, eroman wrote:
> Change &addr.front() to addr.data()

Done.

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number.cc:133: return
IPAddressToStringWithPort(&addr.front(), addr.size(), port);
On 2016/02/17 at 16:11:36, eroman wrote:
> Same thing here: Change &addr.front() to addr.data()

Done.

https://codereview.chromium.org/1692353002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number.cc:137: return std::string(reinterpret_cast<const
char *>(&addr.front()),
On 2016/02/17 at 16:11:36, eroman wrote:
> May as well make the change here too, although this won't be directly related
to current change.

Done.

https://codereview.chromium.org/1692353002/diff/80001/net/base/ip_address_num...
File net/base/ip_address_number.cc (right):

https://codereview.chromium.org/1692353002/diff/80001/net/base/ip_address_num...
net/base/ip_address_number.cc:105: }
Also simplified this. The else clause wasn't necessary.

When the address is invalid, the contents of str will be returned (and will be
the empty string).

[eroman, 2016-02-17 22:20:54]

lgtm

[martijnc, 2016-02-18 08:01:35]

The CQ bit was checked by martijn@martijnc.be

[commit-bot: I haz the power, 2016-02-18 08:02:57]

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

[commit-bot: I haz the power, 2016-02-18 09:00:58]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-02-18 09:02:20]

Description was changed from

==========
Make IP Address related functions return the empty string when used on an
invalid address.

BUG=496258

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

to

==========
Make IP Address related functions return the empty string when used on an
invalid address.

BUG=496258

Committed: https://crrev.com/c925513a3212a55d9c56bec2eea2058c47a76612
Cr-Commit-Position: refs/heads/master@{#375701}

Committed: https://crrev.com/7246f783ae6b768423ba6caeff7229f3775fd789
Cr-Commit-Position: refs/heads/master@{#376124}
==========

[commit-bot: I haz the power, 2016-02-18 09:02:21]

Patchset 3 (id:??) landed as
https://crrev.com/7246f783ae6b768423ba6caeff7229f3775fd789
Cr-Commit-Position: refs/heads/master@{#376124}