|
|
Created:
3 years, 7 months ago by Robert Sesek Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix 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
Patch Set 1 #Patch Set 2 : test message fix #
Total comments: 2
Patch Set 3 : ConsumeLiteral #
Total comments: 2
Patch Set 4 : Address comments #
Total comments: 4
Messages
Total messages: 44 (27 generated)
The CQ bit was checked by rsesek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by rsesek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rsesek@chromium.org changed reviewers: + brettw@chromium.org
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 ==========
rsesek@chromium.org changed reviewers: + jdoerrie@chromium.org
ping
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
The CQ bit was checked by rsesek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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?
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.
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'.
The CQ bit was checked by rsesek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
brettw: ping
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.
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.
The CQ bit was checked by rsesek@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-...)
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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?))
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494420534219510, "parent_rev": "710a8606de93326c2121a1c6970af7a9e800c258", "commit_rev": "20abc56a850ed875b6be6d5ed2f8eeafeca5371a"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/20abc56a850ed875b6be6d5ed2f8... |