Add fuzzer test for preg_parser

Add fuzzer test for preg_parser

The fuzzer does not hook into ReadFile since that reads files and
uses PolicyLoadStatusSample, which in turn creates a histogram entry.
This leads to OOM quicky since the samples accumulate. Thus, this CL
adds a new equivalent ReadData that reads data from memory and writes
status to an PolicyLoadStatus enum.

BUG=660007
TEST=Compiled, ran unit tests and fuzzer

Review-Url: https://codereview.chromium.org/2791193005
Cr-Commit-Position: refs/heads/master@{#465179}
Committed: https://chromium.googlesource.com/chromium/src/+/479c5684bd7b22ab6305c20ae7266506f39149f5

[ljusten (tachyonic), 2017-04-04 11:58:38]

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

[commit-bot: I haz the power, 2017-04-04 11:58:49]

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

[ljusten (tachyonic), 2017-04-04 12:00:30]

ljusten@chromium.org changed reviewers:
+ mnissler@chromium.org, tnagel@chromium.org

[ljusten (tachyonic), 2017-04-04 12:00:31]

Hey, PTAL.

[commit-bot: I haz the power, 2017-04-04 12:30:00]

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

[commit-bot: I haz the power, 2017-04-04 12:30:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ljusten (tachyonic), 2017-04-04 13:36:12]

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

[commit-bot: I haz the power, 2017-04-04 13:36:28]

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-04-04 15:15:09]

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

[commit-bot: I haz the power, 2017-04-04 15:15:10]

Dry run: This issue passed the CQ dry run.

[Mattias Nissler (ping if slow), 2017-04-07 08:13:26]

Nice! Did the fuzzer find anything? How long have you been running it?

I'd personally have preferred to split this into two changes, one for fixing the
prefix handling and one for adding the fuzzer. But I ain't got no powers here
anyways ;-)

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
File components/policy/core/common/preg_parser.cc (right):

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
components/policy/core/common/preg_parser.cc:316: // "someroot" would trigger in
key_name = "somerooting\data".
Alternatively you could enforce that the prefix we test against ends with a
backslash, or that the matching prefix is followed by a backslash, both of which
seem simpler?

[Thiemo Nagel, 2017-04-07 09:39:57]

> I'd personally have preferred to split this into two changes,
> one for fixing the prefix handling and one for adding the
> fuzzer. But I ain't got no powers here anyways ;-)

I agree, especially since we'll have to add a fuzzer reviewer and we shouldn't
burden them with going through refactoring.

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
File components/policy/core/common/preg_parser.h (right):

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
components/policy/core/common/preg_parser.h:45: POLICY_EXPORT bool
ReadData(const uint8_t* data,
I don't think it is fair to export an interface with raw pointers just to suit
libfuzzer.

[Thiemo Nagel, 2017-04-07 09:55:41]

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
File components/policy/core/common/preg_parser.h (right):

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
components/policy/core/common/preg_parser.h:45: POLICY_EXPORT bool
ReadData(const uint8_t* data,
On 2017/04/07 09:39:57, Thiemo Nagel wrote:
> I don't think it is fair to export an interface with raw pointers just to suit
> libfuzzer.

As discussed offline, imho it would be fair to add "Internal" to the name and
add a comment that it must only be used by ReadFile() or for testing.

[ljusten (tachyonic), 2017-04-10 10:14:25]

Description was changed from

==========
Add fuzzer test for preg_parser

The fuzzer does not hook into ReadFile since that reads files and
uses PolicyLoadStatusSample, which in turn creates a histogram entry.
This leads to OOM quicky since the samples accumulate. Thus, this CL
adds a new equivalent ReadData that reads data from memory and writes
status to an PolicyLoadStatus enum.

Fixes an issue found during fuzzing where root was tested against
key_name using StartsWith, so that root= "A/B/C" test positive
against key_name="A\B\Caesar", sending "aesar" to HandleRecord. This
could happen in practice since we have now have both
Software\Policies\Chromium and Software\Policies\ChromiumOS keys.
Also adds a test that checks this case.

BUG=660007
TEST=Compiled, ran unit tests and fuzzer
==========

to

==========
Add fuzzer test for preg_parser

The fuzzer does not hook into ReadFile since that reads files and
uses PolicyLoadStatusSample, which in turn creates a histogram entry.
This leads to OOM quicky since the samples accumulate. Thus, this CL
adds a new equivalent ReadData that reads data from memory and writes
status to an PolicyLoadStatus enum.

BUG=660007
TEST=Compiled, ran unit tests and fuzzer
==========

[ljusten (tachyonic), 2017-04-10 10:22:51]

PTAL. I've split the change up and will commit the second part (proper root key
handling) soon.

>> Did the fuzzer find anything?
Yes! A bug I introduced in the same CL:-) And one I thought was a memory leak,
but it turned out that PolicyLoadStatusSample just accumulates UMA samples, so I
changed the signature to use PolicyLoadStatus.

> How long have you been running it?
A couple of hours until the coverage got to 920, whatever that means...

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
File components/policy/core/common/preg_parser.cc (right):

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
components/policy/core/common/preg_parser.cc:316: // "someroot" would trigger in
key_name = "somerooting\data".
On 2017/04/07 08:13:26, Mattias Nissler (ping if slow) wrote:
> Alternatively you could enforce that the prefix we test against ends with a
> backslash, or that the matching prefix is followed by a backslash, both of
which
> seem simpler?

I thought about that, I was a bit scared by the number of cases (including root
having a backslash in the end or not), but it's probably still simpler. I'll
give it a shot.

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
File components/policy/core/common/preg_parser.h (right):

https://codereview.chromium.org/2791193005/diff/20001/components/policy/core/...
components/policy/core/common/preg_parser.h:45: POLICY_EXPORT bool
ReadData(const uint8_t* data,
On 2017/04/07 09:39:57, Thiemo Nagel wrote:
> I don't think it is fair to export an interface with raw pointers just to suit
> libfuzzer.

As discussed offline, I'll add Internal to the name and a comment.

[Mattias Nissler (ping if slow), 2017-04-12 08:26:58]

LGTM - you need Thiemo's OWNERS stamp though.

[Thiemo Nagel, 2017-04-12 17:10:05]

tnagel@chromium.org changed reviewers:
+ ochang@chromium.org

[Thiemo Nagel, 2017-04-12 17:10:06]

Lgtm % nits.  Adding ochang for the fuzzer parts, please wait for his approval.

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/preg_parser.h (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser.h:17: #include
"components/policy/core/common/policy_load_status.h"
Why have you removed the forward declaration of PolicyLoadStatusSample? -- I'd
have thought that should cover you.

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser.h:44: // with error messages. Used
internally only for testing. For production code,
Nit: "only for testing" seems not correct as it's used for the implementation of
ReadFile() as well.

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser.h:50: PolicyLoadStatus* status,
Nit: Returning a bool *and* updating |status| seems redundant.

[Oliver Chang, 2017-04-13 00:25:23]

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/BUILD.gn (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/BUILD.gn:352: if (is_win || is_chromeos) {
I wasn't aware that libFuzzer is supported on either of these platforms... How
did you run this?

ClusterFuzz also won't be able to pick up these fuzzers and fuzz them
continuously -- is it possible to build this on linux?

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/preg_parser_fuzzer.cc (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser_fuzzer.cc:35: const base::string16
root = base::ASCIIToUTF16(kRegistryChromePolicyKey);
nit: it might be a little more efficient if base::ASCIIToUTF16() is only done
once rather than every time LLVMFuzzerTestOneInput is called.

[ljusten (tachyonic), 2017-04-13 11:17:15]

Oliver, PTAL. Side question, should I check in the corpus that is generated
during a local run?

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/BUILD.gn (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/BUILD.gn:352: if (is_win || is_chromeos) {
> I wasn't aware that libFuzzer is supported on either of these platforms... How
> did you run this?
> ClusterFuzz also won't be able to pick up these fuzzers and fuzz them
> continuously -- is it possible to build this on linux?

ChromeOS is still Linux, confusingly they're not mutually exclusive. The flag is
set if you add target_os = "chromeos" to args.gn. I was hoping that ClusterFuzz
compiled in that configuration as well. If not, I can change preg_parser to
compile on Linux instead of ChromeOS, but I'm hesitant since it's not used.

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/preg_parser.h (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser.h:17: #include
"components/policy/core/common/policy_load_status.h"
On 2017/04/12 17:10:06, Thiemo Nagel wrote:
> Why have you removed the forward declaration of PolicyLoadStatusSample? -- I'd
> have thought that should cover you.

The internal method takes a PolicyLoadStatus, which is an enum in this .h file.
PolicyLoadStatusSample sends off a UMA sample at destruction, which fills up
memory in the fuzzer.

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser.h:44: // with error messages. Used
internally only for testing. For production code,
On 2017/04/12 17:10:06, Thiemo Nagel wrote:
> Nit: "only for testing" seems not correct as it's used for the implementation
of
> ReadFile() as well.

Clarified the wording.

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser.h:50: PolicyLoadStatus* status,
PolicyLoadStatus is used as UMA stat and does not have an 'OK' status. Adding an
OK status at the end of the list or using POLICY_LOAD_STATUS_SIZE to signal 'OK'
would feel a bit hacky.

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/preg_parser_fuzzer.cc (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser_fuzzer.cc:35: const base::string16
root = base::ASCIIToUTF16(kRegistryChromePolicyKey);
On 2017/04/13 00:25:23, Oliver Chang wrote:
> nit: it might be a little more efficient if base::ASCIIToUTF16() is only done
> once rather than every time LLVMFuzzerTestOneInput is called.

I put it in env, but it's going to leak. Do you generally not care in fuzzers?

[Oliver Chang, 2017-04-13 19:58:24]

fuzzer LGTM % comment about making this buildable on linux

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/BUILD.gn (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/BUILD.gn:352: if (is_win || is_chromeos) {
On 2017/04/13 11:17:14, ljusten wrote:
> > I wasn't aware that libFuzzer is supported on either of these platforms...
How
> > did you run this?
> > ClusterFuzz also won't be able to pick up these fuzzers and fuzz them
> > continuously -- is it possible to build this on linux?
> 
> ChromeOS is still Linux, confusingly they're not mutually exclusive. The flag
is
> set if you add target_os = "chromeos" to args.gn. I was hoping that
ClusterFuzz
> compiled in that configuration as well. If not, I can change preg_parser to
> compile on Linux instead of ChromeOS, but I'm hesitant since it's not used.
> 
> 
> 

thanks for explaining. we don't have any libFuzzer builders that build with
target_os = "chromeos", so please let this compile on Linux.

If always building preg_parser on Linux is a concern, maybe this can be done
only if (use_libfuzzer || use_afl) ?

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
File components/policy/core/common/preg_parser_fuzzer.cc (right):

https://codereview.chromium.org/2791193005/diff/60001/components/policy/core/...
components/policy/core/common/preg_parser_fuzzer.cc:35: const base::string16
root = base::ASCIIToUTF16(kRegistryChromePolicyKey);
On 2017/04/13 11:17:15, ljusten wrote:
> On 2017/04/13 00:25:23, Oliver Chang wrote:
> > nit: it might be a little more efficient if base::ASCIIToUTF16() is only
done
> > once rather than every time LLVMFuzzerTestOneInput is called.
> 
> I put it in env, but it's going to leak. Do you generally not care in fuzzers?

That's fine! It's only allocated once during fuzzing and we only consider
something to be a real leak if we lose the reference to it after we exit.

[ljusten (tachyonic), 2017-04-18 07:47:31]

Thanks, it's compiling on Linux now. It's probably a good idea to compile and
test it on a slightly wider range, anyway.

[ljusten (tachyonic), 2017-04-18 07:54:43]

The CQ bit was checked by ljusten@chromium.org

[ljusten (tachyonic), 2017-04-18 07:54:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
mnissler@chromium.org, ochang@chromium.org
Link to the patchset: https://codereview.chromium.org/2791193005/#ps100001
(title: "Compile preg parser on Linux")

[commit-bot: I haz the power, 2017-04-18 07:55:02]

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-04-18 08:50:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492502083138150,
"parent_rev": "4b963a8aa430d5937da3f3cdc7bafcd614d36877", "commit_rev":
"479c5684bd7b22ab6305c20ae7266506f39149f5"}

[commit-bot: I haz the power, 2017-04-18 08:50:36]

Description was changed from

==========
Add fuzzer test for preg_parser

The fuzzer does not hook into ReadFile since that reads files and
uses PolicyLoadStatusSample, which in turn creates a histogram entry.
This leads to OOM quicky since the samples accumulate. Thus, this CL
adds a new equivalent ReadData that reads data from memory and writes
status to an PolicyLoadStatus enum.

BUG=660007
TEST=Compiled, ran unit tests and fuzzer
==========

to

==========
Add fuzzer test for preg_parser

The fuzzer does not hook into ReadFile since that reads files and
uses PolicyLoadStatusSample, which in turn creates a histogram entry.
This leads to OOM quicky since the samples accumulate. Thus, this CL
adds a new equivalent ReadData that reads data from memory and writes
status to an PolicyLoadStatus enum.

BUG=660007
TEST=Compiled, ran unit tests and fuzzer

Review-Url: https://codereview.chromium.org/2791193005
Cr-Commit-Position: refs/heads/master@{#465179}
Committed:
https://chromium.googlesource.com/chromium/src/+/479c5684bd7b22ab6305c20ae726...
==========

[commit-bot: I haz the power, 2017-04-18 08:50:37]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/479c5684bd7b22ab6305c20ae726...

[brucedawson, 2017-04-20 18:18:12]

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

[brucedawson, 2017-04-20 18:18:12]

I noticed this while looking at new warnings from the experimental VC++ /analyze
builder.

https://codereview.chromium.org/2791193005/diff/100001/components/policy/core...
File components/policy/core/common/preg_parser.cc (right):

https://codereview.chromium.org/2791193005/diff/100001/components/policy/core...
components/policy/core/common/preg_parser.cc:280: POLICY_EXPORT bool
ReadDataInternal(const uint8_t* data,
Having a function argument named |data| is confusing when there is also (see
line 321) a local variable called data, with different type.

Consider renaming one of them to avoid future confusion?

It would be nice if we could make variable shadowing an error but there is too
much of it to make that practical at the moment.

[ljusten (tachyonic), 2017-04-20 20:16:32]

Thanks, I'll take care of it. We've enabled -Wextra in our ChromeOS project,
which includes shadowing. I'd be all for enabling this for everything. It also
catches unused function parameters. I was surprised to learn that -Wall doesn't
include these warning.