Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(155)

Issue 2852393002: Fix fuzzer crash for preg_parser (Closed)

Created:
3 years, 7 months ago by ljusten (tachyonic)
Modified:
3 years, 7 months ago
Reviewers:
Thiemo Nagel
CC:
chromium-reviews, fuzzing_chromium.org, jshin+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix fuzzer crash for preg_parser Preg files with strings containing valid code points, but invalid characters (e.g. 65535) triggered a DCHECK because base::UTF16ToUTF8 (called from DecodePRegStringValue) accepts invalid characters, but base::IsStringUTF8 (DCHECK'ed in base::Value) does not. This CL rejects these invalid strings before putting them into base::Values. The crash was found in a libfuzzer test. BUG=714432 TEST=Added and checked test case to verify fix. Review-Url: https://codereview.chromium.org/2852393002 Cr-Commit-Position: refs/heads/master@{#468704} Committed: https://chromium.googlesource.com/chromium/src/+/ee7474ea14ebc2764e277fdf03557851c1976038

Patch Set 1 #

Total comments: 2

Patch Set 2 : Nit fix. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -25 lines) Patch
A chrome/test/data/policy/gpo/fuzzer_corpus/invalid_encoding.pol View Binary file 0 comments Download
A chrome/test/data/policy/gpo/invalid_encoding/registry.pol View Binary file 0 comments Download
A + chrome/test/data/policy/gpo/parser_test/registry.pol View Binary file 0 comments Download
D chrome/test/data/policy/registry.pol View Binary file 0 comments Download
M components/policy/core/common/preg_parser.cc View 1 2 chunks +36 lines, -15 lines 0 comments Download
M components/policy/core/common/preg_parser_unittest.cc View 5 chunks +31 lines, -10 lines 0 comments Download

Messages

Total messages: 15 (9 generated)
ljusten (tachyonic)
Thiemo, PTAL.
3 years, 7 months ago (2017-05-02 15:07:21 UTC) #4
Thiemo Nagel
lgtm with option nit https://codereview.chromium.org/2852393002/diff/1/components/policy/core/common/preg_parser.cc File components/policy/core/common/preg_parser.cc (right): https://codereview.chromium.org/2852393002/diff/1/components/policy/core/common/preg_parser.cc#newcode151 components/policy/core/common/preg_parser.cc:151: return false; Optional nit: Maybe ...
3 years, 7 months ago (2017-05-02 15:48:31 UTC) #5
ljusten (tachyonic)
Thanks, nit fixed. https://codereview.chromium.org/2852393002/diff/1/components/policy/core/common/preg_parser.cc File components/policy/core/common/preg_parser.cc (right): https://codereview.chromium.org/2852393002/diff/1/components/policy/core/common/preg_parser.cc#newcode151 components/policy/core/common/preg_parser.cc:151: return false; On 2017/05/02 15:48:31, Thiemo ...
3 years, 7 months ago (2017-05-02 15:58:32 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2852393002/20001
3 years, 7 months ago (2017-05-02 15:59:23 UTC) #11
Thiemo Nagel
Thanks! Still lgtm.
3 years, 7 months ago (2017-05-02 16:05:00 UTC) #12
commit-bot: I haz the power
3 years, 7 months ago (2017-05-02 17:51:21 UTC) #15
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ee7474ea14ebc2764e277fdf0355...

Powered by Google App Engine
This is Rietveld 408576698