Fix several potential buffer over-read errors in JSONParser::ConsumeNumber.

Fix several potential buffer over-read errors in JSONParser::ConsumeNumber.

BUG=688086
TEST=base_unittests --gtest_filter=JSONParser* under MSan

Review-Url: https://codereview.chromium.org/2712013003
Cr-Commit-Position: refs/heads/master@{#453328}
Committed: https://chromium.googlesource.com/chromium/src/+/bef4f3ae9bdf490e9de52126f16d1e22ed2890e4

[Robert Sesek, 2017-02-24 18:50:12]

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

[commit-bot: I haz the power, 2017-02-24 18:50:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 19:10:24]

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

[commit-bot: I haz the power, 2017-02-24 19:10:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Robert Sesek, 2017-02-24 20:59:08]

rsesek@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-02-24 21:15:31]

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc#ne...
base/json/json_parser.cc:697: if (pos_ < end_pos_ && (*pos_ == 'e' || *pos_ ==
'E')) {
Should lines 697 to 713 be wrapped in a pos_ < end_pos_? Also, is there a
difference between this check and calling CanConsume(1)?

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser_unitt...
File base/json/json_parser_unittest.cc (right):

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser_unitt...
base/json/json_parser_unittest.cc:353: // clang-format off
Is this necessary =P

[Robert Sesek, 2017-02-24 21:25:33]

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc#ne...
base/json/json_parser.cc:697: if (pos_ < end_pos_ && (*pos_ == 'e' || *pos_ ==
'E')) {
On 2017/02/24 21:15:31, dcheng wrote:
> Should lines 697 to 713 be wrapped in a pos_ < end_pos_?

That's what this is... are you asking about line 707?

> Also, is there a
> difference between this check and calling CanConsume(1)?

For the checks on 683 and 697, yes because we don't want to potentially
dereference end_pos_. These checks are the equivalent of CanConsume(0).

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser_unitt...
File base/json/json_parser_unittest.cc (right):

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser_unitt...
base/json/json_parser_unittest.cc:353: // clang-format off
On 2017/02/24 21:15:31, dcheng wrote:
> Is this necessary =P

I mean, no, but without this it tries to put three test cases on a line. I don't
see how that's better. And clang-format being terrible with tables seems like a
KI.

If you don't like it I'll let clang-format make me sad :P.

[dcheng, 2017-02-24 21:55:45]

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc#ne...
base/json/json_parser.cc:697: if (pos_ < end_pos_ && (*pos_ == 'e' || *pos_ ==
'E')) {
On 2017/02/24 21:25:33, Robert Sesek wrote:
> On 2017/02/24 21:15:31, dcheng wrote:
> > Should lines 697 to 713 be wrapped in a pos_ < end_pos_?
> 
> That's what this is... are you asking about line 707?

I can't read, so ignore this.

> 
> > Also, is there a
> > difference between this check and calling CanConsume(1)?
> 
> For the checks on 683 and 697, yes because we don't want to potentially
> dereference end_pos_. These checks are the equivalent of CanConsume(0).

Does end_pos_ point *past* the end of the buffer? The comment seems to hint it
points to the last character, so I'm not sure.

Also, it seems like CanConsume(1) should be a synonym for pos_ < end_pos_?
CanConsume() uses <= instead of <.

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser_unitt...
File base/json/json_parser_unittest.cc (right):

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser_unitt...
base/json/json_parser_unittest.cc:353: // clang-format off
On 2017/02/24 21:25:33, Robert Sesek wrote:
> On 2017/02/24 21:15:31, dcheng wrote:
> > Is this necessary =P
> 
> I mean, no, but without this it tries to put three test cases on a line. I
don't
> see how that's better. And clang-format being terrible with tables seems like
a
> KI.
> 
> If you don't like it I'll let clang-format make me sad :P.

I see your point. While I think we should be generally minimizing use of this, I
can look the other way =P

[Robert Sesek, 2017-02-24 22:15:34]

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

[commit-bot: I haz the power, 2017-02-24 22:16:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Robert Sesek, 2017-02-24 22:16:22]

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/1/base/json/json_parser.cc#ne...
base/json/json_parser.cc:697: if (pos_ < end_pos_ && (*pos_ == 'e' || *pos_ ==
'E')) {
On 2017/02/24 21:55:45, dcheng wrote:
> Does end_pos_ point *past* the end of the buffer? The comment seems to hint it
> points to the last character, so I'm not sure.

end_pos_ is start_pos_+length, so it points at the nul terminator.

> Also, it seems like CanConsume(1) should be a synonym for pos_ < end_pos_?
> CanConsume() uses <= instead of <.

Yeah, done per IRC conversation.

[dcheng, 2017-02-24 22:38:17]

https://codereview.chromium.org/2712013003/diff/20001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/20001/base/json/json_parser.c...
base/json/json_parser.cc:701: if (!CanConsume(1)) {
Can this be handled by falling through to the ReadInt() below?

[Robert Sesek, 2017-02-24 23:01:29]

https://codereview.chromium.org/2712013003/diff/20001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/20001/base/json/json_parser.c...
base/json/json_parser.cc:701: if (!CanConsume(1)) {
On 2017/02/24 22:38:17, dcheng wrote:
> Can this be handled by falling through to the ReadInt() below?

Yup, done.

[Robert Sesek, 2017-02-24 23:01:44]

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

[commit-bot: I haz the power, 2017-02-24 23:02:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-02-24 23:05:33]

LGTM

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.c...
base/json/json_parser.cc:695: if (!CanConsume(1)) {
In theory, we don't need 696-697, and we can fold CanConsume(1) into 699.

I think it might be a bit more confusing to read that way though, so I'm OK with
this.

[Robert Sesek, 2017-02-24 23:07:21]

Thanks!

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.c...
base/json/json_parser.cc:695: if (!CanConsume(1)) {
On 2017/02/24 23:05:33, dcheng wrote:
> In theory, we don't need 696-697, and we can fold CanConsume(1) into 699.
>
> I think it might be a bit more confusing to read that way though, so I'm OK
with
> this.

Yeah, I agree that this is easier to read than folding onto 699.

[commit-bot: I haz the power, 2017-02-25 00:46:51]

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

[commit-bot: I haz the power, 2017-02-25 00:46:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdoerrie, 2017-02-27 10:17:03]

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

[jdoerrie, 2017-02-27 10:17:03]

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.c...
base/json/json_parser.cc:746: char first = *pos_;
You should probably add

if (!CanConsume(1))
    return false;

here, before you dereference pos_. With the current set-up test cases 2, 6 and 8
(i.e. "2.", "43e-", and "2e+") should still trigger a buffer over-read. It's odd
that msan doesn't catch this, but it seems like that the StringPiece constructor
writes the length of string right after the payload. Using gdb I get "2.\002"
for input "2." and "43e-\004" for input "43e-". Therefore the memory past the
end technically is initialized and msan does not complain, but I don't think we
should rely on this behavior.

[Robert Sesek, 2017-02-27 18:12:40]

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

[commit-bot: I haz the power, 2017-02-27 18:13:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 19:22:34]

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

[commit-bot: I haz the power, 2017-02-27 19:22:35]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-02-27 19:43:37]

dcheng: PTAL, fixed ReadInt as well

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2712013003/diff/40001/base/json/json_parser.c...
base/json/json_parser.cc:746: char first = *pos_;
On 2017/02/27 10:17:03, jdoerrie wrote:
> You should probably add
> 
> if (!CanConsume(1))
>     return false;
> 
> here, before you dereference pos_. With the current set-up test cases 2, 6 and
8
> (i.e. "2.", "43e-", and "2e+") should still trigger a buffer over-read. It's
odd
> that msan doesn't catch this, but it seems like that the StringPiece
constructor
> writes the length of string right after the payload. Using gdb I get "2.\002"
> for input "2." and "43e-\004" for input "43e-". Therefore the memory past the
> end technically is initialized and msan does not complain, but I don't think
we
> should rely on this behavior.

Yeah, I noticed that as well after I ran this through ASan (which caught it, but
MSan didn't. Can't explain that).

[dcheng, 2017-02-27 20:43:14]

lgtm

[Robert Sesek, 2017-02-27 20:43:51]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2017-02-27 20:44:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 20:52:04]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488228231577000,
"parent_rev": "edeee45ff630ec55e37944197900c3b5846631c7", "commit_rev":
"bef4f3ae9bdf490e9de52126f16d1e22ed2890e4"}

[commit-bot: I haz the power, 2017-02-27 20:52:48]

Description was changed from

==========
Fix several potential buffer over-read errors in JSONParser::ConsumeNumber.

BUG=688086
TEST=base_unittests --gtest_filter=JSONParser* under MSan
==========

to

==========
Fix several potential buffer over-read errors in JSONParser::ConsumeNumber.

BUG=688086
TEST=base_unittests --gtest_filter=JSONParser* under MSan

Review-Url: https://codereview.chromium.org/2712013003
Cr-Commit-Position: refs/heads/master@{#453328}
Committed:
https://chromium.googlesource.com/chromium/src/+/bef4f3ae9bdf490e9de52126f16d...
==========

[commit-bot: I haz the power, 2017-02-27 20:52:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/bef4f3ae9bdf490e9de52126f16d...