Page MenuHomePhabricator

[libFuzzer] Error and exit if user supplied fuzzer writeable directories don't exist
ClosedPublic

Authored by dgg5503 on Jul 28 2020, 4:06 PM.

Details

Summary

Currently, libFuzzer will exit with an error message if a non-existent
corpus directory is provided. However, if a user provides a non-existent
directory for the artifact_prefix, exact_artifact_path, or
features_dir, libFuzzer will continue execution but silently fail to
write artifacts/features.

To improve the user experience, this PR adds validation for the existence of
all user supplied directories before executing the main fuzzing loop. If they
don't exist, libFuzzer will exit with an error message.

Diff Detail

Event Timeline

dgg5503 created this revision.Jul 28 2020, 4:06 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptJul 28 2020, 4:06 PM
dgg5503 requested review of this revision.Jul 28 2020, 4:06 PM
dgg5503 added a reviewer: filcab.
dgg5503 updated this revision to Diff 281422.Jul 28 2020, 5:01 PM

Applied clang-format to some areas of the code but left others alone to be consistent with the rest of the libFuzzer code base. Please let me know if it's appropriate to apply all suggested clang-tidy and clang-format changes.

kcc added a comment.Aug 4 2020, 1:21 PM

I'd rather fail instead of silently creating new dirs, to be consistent with the other behavior

In D84808#2194438, @kcc wrote:

I'd rather fail instead of silently creating new dirs, to be consistent with the other behavior

Are you referring to the non-existent corpus case? If so, that's also changed in this PR to have its directory created if it doesn't exist. Please let me know if I've overlooked additional cases where directories are expected to exist.

kcc added a comment.Aug 4 2020, 4:11 PM

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

In D84808#2194844, @kcc wrote:

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

I can make this change, but is there a reason why this shouldn't be done? It seems more convenient for the end user but perhaps I'm overlooking a larger issue.

kcc added a comment.Aug 6 2020, 10:55 AM
In D84808#2194844, @kcc wrote:

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

I can make this change, but is there a reason why this shouldn't be done? It seems more convenient for the end user but perhaps I'm overlooking a larger issue.

First reason is consistency. We currently don't create DIRs if they don't exist.
The second reason is avoiding surprise.
Suppose a user has a typo in a directory name.
Crashing because a wrongly named dir doesn't exist is not a surprise.
Working and using a wrongly named dir *is* a surprise.

In D84808#2200407, @kcc wrote:
In D84808#2194844, @kcc wrote:

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

I can make this change, but is there a reason why this shouldn't be done? It seems more convenient for the end user but perhaps I'm overlooking a larger issue.

First reason is consistency. We currently don't create DIRs if they don't exist.
The second reason is avoiding surprise.
Suppose a user has a typo in a directory name.
Crashing because a wrongly named dir doesn't exist is not a surprise.
Working and using a wrongly named dir *is* a surprise.

Thank you for the elaboration @kcc . Your reasoning makes sense to me given the current design of my patch. Before I make the appropriate changes, would it be acceptable to have an environment variable or launch parameter that could allow the silent creation of these directories? The reason I'm so keen on this functionality is that I'm working with a remotely accessible embedded environment where it's a bit of a pain to create these directories by hand, especially in CI situations.

kcc added a comment.Aug 10 2020, 5:50 PM

would it be acceptable to have an environment variable or launch parameter that could allow the silent creation of these directories?

That would be a very reasonable thing to do (command line flag, not env var)!
Like, -create-nonexistent-dirs=1 (0 by default) or some such.
But please make it a separate patch to simplify the review.

dgg5503 updated this revision to Diff 286070.Aug 17 2020, 10:36 AM
dgg5503 retitled this revision from [libFuzzer] Create user supplied fuzzer writeable directories if they don't exist to [libFuzzer] Error and exit if user supplied fuzzer writeable directories don't exist.
dgg5503 edited the summary of this revision. (Show Details)
dgg5503 updated this revision to Diff 286072.Aug 17 2020, 10:39 AM

Fixed the diff

morehouse accepted this revision.Aug 24 2020, 5:25 PM
morehouse added inline comments.
compiler-rt/lib/fuzzer/FuzzerDriver.cpp
692

If it is a file, should we print an error message?

708

Nit: remove this space.

This revision is now accepted and ready to land.Aug 24 2020, 5:25 PM
dgg5503 added inline comments.Aug 25 2020, 5:39 PM
compiler-rt/lib/fuzzer/FuzzerDriver.cpp
692

The first input can also be a test file rather than a directory. An example of this can be seen in fuzzer-runs.test.

I also want to point out that Options.OutputCorpus is no longer being set if a file is provided as (*Inputs)[0] unlike the old behavior. All fuzzer tests & unit tests pass on my local Linux machine so I don't think this is having any adverse effect.

dgg5503 updated this revision to Diff 287809.Aug 25 2020, 5:53 PM

Address @morehouse comment.

dgg5503 marked an inline comment as done.Aug 25 2020, 5:53 PM