This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Replace -seed_corpus to better support fork mode on Win
ClosedPublic

Authored by metzman on Apr 22 2019, 2:42 PM.

Details

Summary

Pass seed corpus list in a file to get around argument length limits on Windows.
This limit was preventing many uses of fork mode on Windows.

Diff Detail

Repository
rL LLVM

Event Timeline

metzman created this revision.Apr 22 2019, 2:42 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 22 2019, 2:42 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
metzman planned changes to this revision.Apr 22 2019, 2:46 PM

I'm going to add a test for this.

metzman updated this revision to Diff 196154.Apr 22 2019, 4:16 PM
  • improve comment
Harbormaster completed remote builds in B30858: Diff 196154.
metzman updated this revision to Diff 196165.Apr 22 2019, 6:22 PM
metzman marked 2 inline comments as done.
  • undo accidental
compiler-rt/lib/fuzzer/FuzzerDriver.cpp
771 ↗(On Diff #196154)

Removing the file is somewhat hostile to users but is the best way to prevent the accumulation of files in fork mode.
Since the primary purpose of -seed_inputs_file is for fork mode, I think this is acceptable. Thoughts?

compiler-rt/test/fuzzer/cross_over.test
18 ↗(On Diff #196154)

The reason why I do this hacky python thing is because echo leaves a trailing newline and printf didn't work well with the percent formatting.
Is it OK to use python for this purpose (I think python is necessary to run lit commands anyway)?
If not, I can use echo so long as I end with a trailing ,, that way the last seed input is "\n" instead of a newline being added to a filename we actually care about. Does this sound better than the python hack?

PTAL.
This change allows fork mode to be used reasonably on Win.

metzman retitled this revision from [fuzzer] Replace -seed_corpus with -seed_corpus_file to [libFuzzer] Replace -seed_corpus with -seed_corpus_file.Apr 23 2019, 5:12 AM
metzman edited the summary of this revision. (Show Details)
metzman retitled this revision from [libFuzzer] Replace -seed_corpus with -seed_corpus_file to [libFuzzer] Replace -seed_corpus to better support fork mode on Win.Apr 24 2019, 8:51 AM
metzman edited the summary of this revision. (Show Details)
kcc added inline comments.Apr 25 2019, 5:56 PM
compiler-rt/lib/fuzzer/FuzzerDriver.cpp
771 ↗(On Diff #196154)

A bit too hostile indeed, and in this case the file is deleted by a process that didn't create it, making it more confusing.
I'd prefer to delete the file in the same place we delete other temporary files for a child.

compiler-rt/lib/fuzzer/FuzzerFlags.def
23 ↗(On Diff #196165)

I found this flag to be useful by itself, outside the fork mode, so instead of replacing it with a new flag,
I'd suggest enhancing it's behavior:

  • --seed_inputs=aaa,bbb keeps working as is
  • --seed_inputs=@file_path reads the list from the file
compiler-rt/lib/fuzzer/FuzzerFork.cpp
126 ↗(On Diff #196165)

for readability, I'd prefer to introduce another variant of WriteToFile:

void WriteToFile(const std::string &Str, const std::string &Path);
compiler-rt/test/fuzzer/cross_over.test
18 ↗(On Diff #196154)

no need to change this test with the change I proposed.

compiler-rt/test/fuzzer/seed_inputs_file.test
4 ↗(On Diff #196165)

will echo -n work in this case?
python is indeed a bit unwelcome where.

metzman marked an inline comment as done.Apr 29 2019, 10:17 AM
metzman added inline comments.
compiler-rt/lib/fuzzer/FuzzerFlags.def
23 ↗(On Diff #196165)

So that "@" will be necessary to distinguish between a case where we want to use one seed and a case where we want to use the file as the seed list?

kcc added inline comments.Apr 29 2019, 11:25 AM
compiler-rt/lib/fuzzer/FuzzerFlags.def
23 ↗(On Diff #196165)

Yes.
W/o @ the current behavior is preserved: the argument is a comma-separated list of seeds.
W/ @ the argument (with @ stripped) will be treated as a file name, which contains the comma-separated list of seeds

metzman updated this revision to Diff 197345.Apr 30 2019, 8:36 AM
metzman marked 10 inline comments as done.
  • change name of test
  • Get list argument working again
  • combine code
  • rename
  • Use old method
metzman updated this revision to Diff 197346.Apr 30 2019, 8:39 AM
  • improve comments
metzman added inline comments.Apr 30 2019, 8:57 AM
compiler-rt/lib/fuzzer/FuzzerDriver.cpp
771 ↗(On Diff #196154)

Done.

compiler-rt/lib/fuzzer/FuzzerFlags.def
23 ↗(On Diff #196165)

Done. Please let me know if you think the help message needs work.

compiler-rt/lib/fuzzer/FuzzerFork.cpp
126 ↗(On Diff #196165)

Done.

compiler-rt/test/fuzzer/cross_over.test
18 ↗(On Diff #196154)

Undid this change and the one in len_control.

compiler-rt/test/fuzzer/seed_inputs_file.test
4 ↗(On Diff #196165)

Yeah good suggestion, that's much better. For some reason I thought it wouldn't work on Windows.

kcc accepted this revision.Apr 30 2019, 9:41 AM

LGTM with several nits.

compiler-rt/lib/fuzzer/FuzzerDriver.cpp
771 ↗(On Diff #197346)

plz make this if-else more compact (no {}, comments on the same line)

compiler-rt/lib/fuzzer/FuzzerFlags.def
24 ↗(On Diff #197346)

"of input files" repeated twice?

compiler-rt/test/fuzzer/seed_inputs.test
3 ↗(On Diff #197346)

replace with CHECK: then remove all --check-prefix

13 ↗(On Diff #197346)

terminate the file with a newline

This revision is now accepted and ready to land.Apr 30 2019, 9:41 AM
metzman updated this revision to Diff 197369.Apr 30 2019, 10:29 AM
metzman marked 5 inline comments as done.
  • fmt
  • fix nits
  • fmt
metzman updated this revision to Diff 197372.Apr 30 2019, 10:40 AM
metzman marked an inline comment as done.
  • ideal but test failing
  • fix issue
metzman updated this revision to Diff 197373.Apr 30 2019, 10:42 AM
  • remove extra newline
Harbormaster completed remote builds in B31173: Diff 197373.
metzman added inline comments.Apr 30 2019, 10:43 AM
compiler-rt/test/fuzzer/seed_inputs.test
3 ↗(On Diff #197346)

I added a test to ensure we handle a single file correctly instead.

metzman updated this revision to Diff 197375.Apr 30 2019, 10:53 AM
  • use new format
metzman updated this revision to Diff 197389.Apr 30 2019, 11:37 AM
  • only use @ in argument
metzman updated this revision to Diff 197391.Apr 30 2019, 11:42 AM
This comment was removed by metzman.
metzman updated this revision to Diff 197393.Apr 30 2019, 11:44 AM
  • fix bug
metzman updated this revision to Diff 197396.Apr 30 2019, 11:57 AM
  • Make LF fail if no seed list
metzman updated this revision to Diff 197397.Apr 30 2019, 12:00 PM
  • Add more tests to verify we catch empty lists

@kcc I've changed things so that libFuzzer will fail if the argument to -seed_inputs is a non existent file or is empty? What do you think of this change?

metzman updated this revision to Diff 197399.Apr 30 2019, 12:03 PM
  • add newline
kcc accepted this revision.Apr 30 2019, 12:07 PM

LGTM

compiler-rt/lib/fuzzer/FuzzerDriver.cpp
776 ↗(On Diff #197399)

use exit(1) instead

metzman updated this revision to Diff 197400.Apr 30 2019, 12:09 PM
  • improve error message and look for it in tests
metzman updated this revision to Diff 197401.Apr 30 2019, 12:11 PM
  • improve message
metzman updated this revision to Diff 197404.Apr 30 2019, 12:13 PM
  • use exit(1)
metzman updated this revision to Diff 197405.Apr 30 2019, 12:15 PM
  • add missing period
This revision was automatically updated to reflect the committed changes.