Fix potential buffer over-read errors for un-terminated JSON strings and comments.

Fix potential buffer over-read errors for un-terminated JSON strings and comments.

BUG=698693
TEST=base_unittests --gtest_filter=JSON* under MSan

Review-Url: https://codereview.chromium.org/2859513002
Cr-Commit-Position: refs/heads/master@{#470555}
Committed: https://chromium.googlesource.com/chromium/src/+/20abc56a850ed875b6be6d5ed2f8eeafeca5371a

[Robert Sesek, 2017-05-02 16:27:38]

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

[commit-bot: I haz the power, 2017-05-02 16:27:57]

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-05-02 17:03:32]

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

[commit-bot: I haz the power, 2017-05-02 17:03:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Robert Sesek, 2017-05-02 17:34:46]

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

[commit-bot: I haz the power, 2017-05-02 17:35:00]

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-05-02 19:08:31]

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

[commit-bot: I haz the power, 2017-05-02 19:08:32]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-05-02 19:37:58]

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

[Robert Sesek, 2017-05-02 19:49:30]

Description was changed from

==========
Fix potential buffer over-read errors for un-terminated JSON strings and
comments.

BUG=698693
TEST=base_unittests --gtest_filter=JSON* under MSan
==========

to

==========
Fix potential buffer over-read errors for un-terminated JSON strings and
comments.

BUG=698693
TEST=base_unittests --gtest_filter=JSON* under MSan
==========

[Robert Sesek, 2017-05-02 19:49:30]

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

[Robert Sesek, 2017-05-04 17:21:45]

ping

[jdoerrie, 2017-05-05 09:34:11]

Thanks for the fix! The test cases mentioned in the bug don't cause the
uninitialized reads any more.

Using the patched fuzzer from http://crrev.com/2731943002 I found some other bad
inputs, though.

These include inputs triggering |ConsumeLiteral| such as "tru", "fals" and
"nul", as well as starts of escape sequences such as "\"\\x0".

Furthermore, there seems to be an issue with CBU_NEXT, where index_ and pos_ get
out of sync.

An example input here is "\"\xC3\x83" (not that these are the literal 0xC3 and
0x83 characters, not an escaped version").

Putting a DCHECK_EQ(pos_, start_pos_ + index_) it CanConsume fires, which is
another cause of this issue.

The first mentioned cases are relatively easy to fix by updating the
corresponding CanConsumes, however the out of sync issue might be a bit harder
to solve.

Do you want to address these issues in this CL as well or should I file another
bug?

https://codereview.chromium.org/2859513002/diff/20001/base/json/json_parser_u...
File base/json/json_parser_unittest.cc (right):

https://codereview.chromium.org/2859513002/diff/20001/base/json/json_parser_u...
base/json/json_parser_unittest.cc:34: StringPiece MakeNullTerminatedInput(const
char* input,
nit: MakeNonNullTerminatedInput

[Robert Sesek, 2017-05-05 19:08:05]

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

[commit-bot: I haz the power, 2017-05-05 19:09:01]

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-05-05 21:20:50]

On 2017/05/05 09:34:11, jdoerrie wrote:
> Thanks for the fix! The test cases mentioned in the bug don't cause the
> uninitialized reads any more.
> 
> Using the patched fuzzer from http://crrev.com/2731943002 I found some other
bad
> inputs, though.
> 
> These include inputs triggering |ConsumeLiteral| such as "tru", "fals" and
> "nul", as well as starts of escape sequences such as "\"\\x0".

Fixed these.

> Furthermore, there seems to be an issue with CBU_NEXT, where index_ and pos_
get
> out of sync.
> 
> An example input here is "\"\xC3\x83" (not that these are the literal 0xC3 and
> 0x83 characters, not an escaped version").
> 
> Putting a DCHECK_EQ(pos_, start_pos_ + index_) it CanConsume fires, which is
> another cause of this issue.
> 
> The first mentioned cases are relatively easy to fix by updating the
> corresponding CanConsumes, however the out of sync issue might be a bit harder
> to solve.
> 
> Do you want to address these issues in this CL as well or should I file
another
> bug?

Yes I noticed the issue with CBU8_NEXT when working on this fix. It's to do with
the fact that it's pre-increment. Feel free to file a bug.

https://codereview.chromium.org/2859513002/diff/20001/base/json/json_parser_u...
File base/json/json_parser_unittest.cc (right):

https://codereview.chromium.org/2859513002/diff/20001/base/json/json_parser_u...
base/json/json_parser_unittest.cc:34: StringPiece MakeNullTerminatedInput(const
char* input,
On 2017/05/05 09:34:11, jdoerrie wrote:
> nit: MakeNonNullTerminatedInput

Done.

[commit-bot: I haz the power, 2017-05-05 22:20:12]

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

[commit-bot: I haz the power, 2017-05-05 22:20:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[brettw, 2017-05-05 23:47:11]

On 2017/05/05 21:20:50, Robert Sesek wrote:
> > Do you want to address these issues in this CL as well or should I file
> another
> > bug?
> 
> Yes I noticed the issue with CBU8_NEXT when working on this fix. It's to do
with
> the fact that it's pre-increment. Feel free to file a bug.

This is used by all of our UTF conversion routines, and is copied from ICU which
is used everywhere, so if there is a bug here we should treat it as high
priority. Or is this a bug in the way we call it?

[jdoerrie, 2017-05-08 08:37:05]

On 2017/05/05 23:47:11, brettw (busy this week) wrote:
> On 2017/05/05 21:20:50, Robert Sesek wrote:
> > > Do you want to address these issues in this CL as well or should I file
> > another
> > > bug?
> > 
> > Yes I noticed the issue with CBU8_NEXT when working on this fix. It's to do
> with
> > the fact that it's pre-increment. Feel free to file a bug.
> 
> This is used by all of our UTF conversion routines, and is copied from ICU
which
> is used everywhere, so if there is a bug here we should treat it as high
> priority. Or is this a bug in the way we call it?

I think the actual routine is fine, however currently it breaks the |pos_ ==
start_pos_ + index_| contract we use in this file
(https://codesearch.chromium.org/chromium/src/base/json/json_parser.h?l=231).
This is because we don't properly update pos_ following the calls to CBU8_NEXT
in lines 468 and 474. This however can be fixed locally and is not an error in
CBU8_NEXT.

[jdoerrie, 2017-05-08 08:38:25]

LGTM, I will file a bug for the issues with the calls to CBU8_NEXT.

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

https://codereview.chromium.org/2859513002/diff/40001/base/json/json_parser.c...
base/json/json_parser.cc:534: if (!CanConsume(2)) {
Could you change this to CanConsume(3)? That fixes the parser for invalid inputs
like '"\x0'.

[Robert Sesek, 2017-05-08 19:33:26]

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

[commit-bot: I haz the power, 2017-05-08 19:34:48]

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-05-08 21:10:49]

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

https://codereview.chromium.org/2859513002/diff/40001/base/json/json_parser.c...
base/json/json_parser.cc:534: if (!CanConsume(2)) {
On 2017/05/08 08:38:25, jdoerrie (slow this week) wrote:
> Could you change this to CanConsume(3)? That fixes the parser for invalid
inputs
> like '"\x0'. 

Done.

[commit-bot: I haz the power, 2017-05-08 21:18:28]

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

[commit-bot: I haz the power, 2017-05-08 21:18:29]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-05-09 18:58:14]

brettw: ping

[brettw, 2017-05-09 19:53:14]

LGTM, some optional/future suggestions:

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

https://codereview.chromium.org/2859513002/diff/60001/base/json/json_parser.c...
base/json/json_parser.cc:282: NextChar();
Did you consider making NextChar() return void? It seems that this is a pretty
dangerous function to call. I'd prefer "void Advance()". Anyway, doesn't need to
block this...

https://codereview.chromium.org/2859513002/diff/60001/base/json/json_parser_u...
File base/json/json_parser_unittest.cc (right):

https://codereview.chromium.org/2859513002/diff/60001/base/json/json_parser_u...
base/json/json_parser_unittest.cc:419: for (unsigned int i = 0; i <
arraysize(kCases); ++i) {
Random optional thought: Does C++11 let you do:
  for (const char* test_case : kCases)
here? I thought begin() and end() worked for static arrays. Maybe I'm dreaming.

[Robert Sesek, 2017-05-09 21:27:37]

Thanks.

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

https://codereview.chromium.org/2859513002/diff/60001/base/json/json_parser.c...
base/json/json_parser.cc:282: NextChar();
On 2017/05/09 19:53:14, brettw (behind--catching up) wrote:
> Did you consider making NextChar() return void? It seems that this is a pretty
> dangerous function to call. I'd prefer "void Advance()". Anyway, doesn't need
to
> block this...

Yeah, I was thinking about this because result of NextChar does seem to be
generally dangerous. I'll look at renaming and re-typing in the follow-up.

https://codereview.chromium.org/2859513002/diff/60001/base/json/json_parser_u...
File base/json/json_parser_unittest.cc (right):

https://codereview.chromium.org/2859513002/diff/60001/base/json/json_parser_u...
base/json/json_parser_unittest.cc:419: for (unsigned int i = 0; i <
arraysize(kCases); ++i) {
On 2017/05/09 19:53:14, brettw (behind--catching up) wrote:
> Random optional thought: Does C++11 let you do:
>   for (const char* test_case : kCases)
> here? I thought begin() and end() worked for static arrays. Maybe I'm
dreaming.

The |for (:)| construct does work, but I use |i| in the SCOPED_TRACE.

[Robert Sesek, 2017-05-09 21:27:43]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2017-05-09 21:27:44]

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

[commit-bot: I haz the power, 2017-05-09 21:29:18]

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-05-09 22:57:58]

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

[commit-bot: I haz the power, 2017-05-09 22:57:59]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Robert Sesek, 2017-05-10 01:19:13]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2017-05-10 01:20:51]

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-05-10 03:23:52]

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

[commit-bot: I haz the power, 2017-05-10 03:23:53]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))

[Robert Sesek, 2017-05-10 12:48:54]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2017-05-10 12:49:18]

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-05-10 13:49:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494420534219510,
"parent_rev": "710a8606de93326c2121a1c6970af7a9e800c258", "commit_rev":
"20abc56a850ed875b6be6d5ed2f8eeafeca5371a"}

[commit-bot: I haz the power, 2017-05-10 13:50:01]

Description was changed from

==========
Fix potential buffer over-read errors for un-terminated JSON strings and
comments.

BUG=698693
TEST=base_unittests --gtest_filter=JSON* under MSan
==========

to

==========
Fix potential buffer over-read errors for un-terminated JSON strings and
comments.

BUG=698693
TEST=base_unittests --gtest_filter=JSON* under MSan

Review-Url: https://codereview.chromium.org/2859513002
Cr-Commit-Position: refs/heads/master@{#470555}
Committed:
https://chromium.googlesource.com/chromium/src/+/20abc56a850ed875b6be6d5ed2f8...
==========

[commit-bot: I haz the power, 2017-05-10 13:50:03]

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