Add base/parse_number.h to generalize number parsing done in //net.

Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it permits strings with a leading plus sign, which is invalid in most places where //net parses numbers.

BUG=596523, 596538
Committed: https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99
Cr-Commit-Position: refs/heads/master@{#382936}

[eroman, 2016-03-23 19:15:00]

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

[eroman, 2016-03-23 19:15:01]

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h
File net/base/parse_number.h (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:63: NET_EXPORT bool ParseNonNegativeDecimalInt(const
base::StringPiece& input,
This may seem like overkill to have 1 function in 1 file....

But I expect I will need a couple lesser used overloads, for uint/int64/uint64
variations so this will grow by the time I am done solving the bug.

Also the documentation is pretty dense.

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:64: int* output) WARN_UNUSED_RESULT;
WARN_UNUSED_RESULT is also a modification over the //base API, since I have seen
some sketchy looking usages in //net where we don't check for success.

https://codereview.chromium.org/1829873002/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/sdch_manager.cc#ne...
net/base/sdch_manager.cc:381: base::StringToInt64(value, &seconds);
Here for instance the result is not being checked, unclear if this results in a
bug but certainly looks sketchy.

Will re-visit this when/if I add a variation for signed int64 (here it also
legitimately allows for negatives)

[mmenke, 2016-03-23 19:32:28]

LGTM!

https://codereview.chromium.org/1829873002/diff/1/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/host_port_pair.cc#...
net/base/host_port_pair.cc:8: #include
"base/strings/string_number_conversions.h"
Hrm...Guess we still need this for Uint to string.  There goes my idea of
banning this in net via DEPS.  Maybe a presubmit check, eventually?

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.cc
File net/base/parse_number.cc (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.cc#ne...
net/base/parse_number.cc:12: if (input.empty() || input[0] == '+' || input[0] ==
'-')
Can we just check if it's not between 0 and 9?  Same thing, given StringToInts
current implementation, and likely all future ones, but I'm paranoid, and just
checking those two characters doesn't save us anything.

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h
File net/base/parse_number.h (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:24: //    //base versions.
Mind putting these under the includes?  I know that I, at least, always miss
comments up here.  When scrolling in a file, I see the includes, and think I'm
at the top.

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:30: #include "base/strings/string_piece.h"
Forward declare this, and include it in the CC file?

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:63: NET_EXPORT bool ParseNonNegativeDecimalInt(const
base::StringPiece& input,
On 2016/03/23 19:15:01, eroman wrote:
> This may seem like overkill to have 1 function in 1 file....
> 
> But I expect I will need a couple lesser used overloads, for uint/int64/uint64
> variations so this will grow by the time I am done solving the bug.
> 
> Also the documentation is pretty dense.

I think this is reasonable.

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number_unitt...
File net/base/parse_number_unittest.cc (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number_unitt...
net/base/parse_number_unittest.cc:16: } tests[] = {
+const

tests -> kTests

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number_unitt...
net/base/parse_number_unittest.cc:17: {"0", 0}, {"00000", 0}, {"003", 3},
{"003", 3}, {"1234566", 1234566},
You have no test that we can handle digits in decreasing order!  </silliness>

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number_unitt...
net/base/parse_number_unittest.cc:36: "0xFF",
0x11?  Just checking hex value with non-hex chars.  Not that it's likely to
regress independently, but the cost of extra cases here is pretty low.

[eroman, 2016-03-23 19:39:41]

Description was changed from

==========
Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it
permits strings with a leading plus sign, which is invalid in most places where
//net parses numbers.

BUG=596523
==========

to

==========
Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it
permits strings with a leading plus sign, which is invalid in most places where
//net parses numbers.

BUG=596523, 596538
==========

[eroman, 2016-03-23 19:50:30]

https://codereview.chromium.org/1829873002/diff/1/net/base/host_port_pair.cc
File net/base/host_port_pair.cc (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/host_port_pair.cc#...
net/base/host_port_pair.cc:8: #include
"base/strings/string_number_conversions.h"
On 2016/03/23 19:32:28, mmenke wrote:
> Hrm...Guess we still need this for Uint to string.  There goes my idea of
> banning this in net via DEPS.  Maybe a presubmit check, eventually?

Yeah, we can tackle this eventually.

There are a couple base::StringToInt() which I may not switch over -- in
particular ones being used to parse command-line flags. In this case it doesn't
seem harmful to be more permissive. Although it is inconsistent if we recognize
ports with a leading + from command line flag but nowhere else....

Either way, will think about once the bulk is addressed.

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.cc
File net/base/parse_number.cc (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.cc#ne...
net/base/parse_number.cc:12: if (input.empty() || input[0] == '+' || input[0] ==
'-')
On 2016/03/23 19:32:28, mmenke wrote:
> Can we just check if it's not between 0 and 9?  Same thing, given StringToInts
> current implementation, and likely all future ones, but I'm paranoid, and just
> checking those two characters doesn't save us anything.

Done.

(I had same intuition, but somehow rejected it).

An even more paranoid approach is that taken by websockets:

 24 bool GetWindowBits(const std::string& value, int* window_bits) {
 25   return !value.empty() && value[0] != '0' &&
 26          value.find_first_not_of("0123456789") == std::string::npos &&
 27          base::StringToInt(value, window_bits);
 28 }

Which checks for invalid characters throughout.
I think sufficient tests with checking of first character is sufficient though.

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h
File net/base/parse_number.h (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:24: //    //base versions.
On 2016/03/23 19:32:28, mmenke wrote:
> Mind putting these under the includes?  I know that I, at least, always miss
> comments up here.  When scrolling in a file, I see the includes, and think I'm
> at the top.

Done, I agree with finding it there hard.

However technically I believe that is a violation of the google C++ guide:

https://google.github.io/styleguide/cppguide.html#File_Comments

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:30: #include "base/strings/string_piece.h"
On 2016/03/23 19:32:28, mmenke wrote:
> Forward declare this, and include it in the CC file?

StringPiece I believe can't (easily) be forward declared as it uses template
magic.
Do you have suggestions on how to do it?

The code I have come across gives up and just includes the header.

And some (inconsistent on exact pattern) code passes by value so necessarily
needs the definition.

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number_unitt...
File net/base/parse_number_unittest.cc (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number_unitt...
net/base/parse_number_unittest.cc:17: {"0", 0}, {"00000", 0}, {"003", 3},
{"003", 3}, {"1234566", 1234566},
On 2016/03/23 19:32:28, mmenke wrote:
> You have no test that we can handle digits in decreasing order!  </silliness>

Done

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number_unitt...
net/base/parse_number_unittest.cc:36: "0xFF",
On 2016/03/23 19:32:28, mmenke wrote:
> 0x11?  Just checking hex value with non-hex chars.  Not that it's likely to
> regress independently, but the cost of extra cases here is pretty low.

Good idea. Done.

[eroman, 2016-03-23 19:51:36]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-23 19:51:48]

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

[mmenke, 2016-03-23 20:12:07]

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h
File net/base/parse_number.h (right):

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:24: //    //base versions.
On 2016/03/23 19:50:30, eroman wrote:
> On 2016/03/23 19:32:28, mmenke wrote:
> > Mind putting these under the includes?  I know that I, at least, always miss
> > comments up here.  When scrolling in a file, I see the includes, and think
I'm
> > at the top.
> 
> Done, I agree with finding it there hard.
> 
> However technically I believe that is a violation of the google C++ guide:
> 
> https://google.github.io/styleguide/cppguide.html#File_Comments

I dunno...This doesn't describe the contents of the license boilerplate.  Surely
that's what "it" refers to "Start each file with license boilerplate, followed
by a description of its contents.", so I think we're good.  :)

https://codereview.chromium.org/1829873002/diff/1/net/base/parse_number.h#new...
net/base/parse_number.h:30: #include "base/strings/string_piece.h"
On 2016/03/23 19:50:30, eroman wrote:
> On 2016/03/23 19:32:28, mmenke wrote:
> > Forward declare this, and include it in the CC file?
> 
> StringPiece I believe can't (easily) be forward declared as it uses template
> magic.
> Do you have suggestions on how to do it?
> 
> The code I have come across gives up and just includes the header.
> 
> And some (inconsistent on exact pattern) code passes by value so necessarily
> needs the definition.

I'm afraid I don't.  StringPiece looks so nice and fluffy...I hadn't thought of
the template horror that lurks just beneath the surface.

[commit-bot: I haz the power, 2016-03-23 20:58:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-23 20:58:43]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-03-23 21:56:07]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-03-23 21:56:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1829873002/#ps40001
(title: "more tests")

[commit-bot: I haz the power, 2016-03-23 21:56:39]

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

[commit-bot: I haz the power, 2016-03-23 22:07:16]

Description was changed from

==========
Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it
permits strings with a leading plus sign, which is invalid in most places where
//net parses numbers.

BUG=596523, 596538
==========

to

==========
Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it
permits strings with a leading plus sign, which is invalid in most places where
//net parses numbers.

BUG=596523, 596538
==========

[commit-bot: I haz the power, 2016-03-23 22:07:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-23 22:08:18]

Description was changed from

==========
Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it
permits strings with a leading plus sign, which is invalid in most places where
//net parses numbers.

BUG=596523, 596538
==========

to

==========
Add base/parse_number.h to generalize number parsing done in //net.

In particular, the current use of base::StringToInt() is problematic as it
permits strings with a leading plus sign, which is invalid in most places where
//net parses numbers.

BUG=596523, 596538

Committed: https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99
Cr-Commit-Position: refs/heads/master@{#382936}
==========

[commit-bot: I haz the power, 2016-03-23 22:08:19]

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