[libFuzzer] Mutation tracking and logging implemented.
ClosedPublic

Authored by kodewilliams on Jun 11 2018, 2:48 PM.

Details

Summary

Code now exists to track number of mutations that are used in fuzzing in total
and ones that produce new coverage. The stats are currently being dumped to the
command line.

Patch by Kodé Williams (@kodewilliams).

Diff Detail

There are a very large number of changes, so older changes are hidden. Show Older Changes
morehouse added inline comments.Mon, Jun 25, 4:52 PM
lib/fuzzer/FuzzerMutationStats.cpp
18 ↗(On Diff #152774)

Can we just do MutationStats MStats instead of making this on the heap?

25 ↗(On Diff #152798)

Can we just do:

double UsefulPercentage = TotalMutations.at(i) ? ... : 0
26 ↗(On Diff #152798)

The output will be easier to understand if you also print the mutation names.

test/fuzzer/fuzzer-mutationstats.test
6

I think this regex can be simplified to {{[0-9]+\.[0-9]+}}

kodewilliams added inline comments.Mon, Jun 25, 5:11 PM
lib/fuzzer/FuzzerLoop.cpp
374

Mutation stats relies on final stats. This way, the same line doesnt have to be put into the code 10 times (like PrintFinalStats() is).

morehouse added inline comments.Mon, Jun 25, 5:15 PM
lib/fuzzer/FuzzerLoop.cpp
374

I don't think moving this above the short circuit should affect that. It will just make your flag have the same behavior as -print_coverage, -dump_coverage, etc. which don't require -print_final_stats=1 to work.

kodewilliams marked 11 inline comments as done.
kodewilliams added inline comments.
lib/fuzzer/FuzzerMutationStats.cpp
26 ↗(On Diff #152798)

I was told to remove the names from the output for parsing purposes, as it will make it easier to use in the latter phase of the project. Does this still stand?

morehouse added a subscriber: kcc.Tue, Jun 26, 12:46 PM
morehouse added inline comments.
lib/fuzzer/FuzzerMutationStats.cpp
26 ↗(On Diff #152798)

I was interpreting this patch as an option to print human-readable stats similar to -print_final_stats.

@kcc, @Dor1s: Do we care if this information is human readable or not?

25 ↗(On Diff #152923)

Please remove dead code.

27 ↗(On Diff #152923)

This formatting looks odd. Please use clang-format on your changes.

metzman added inline comments.Tue, Jun 26, 12:50 PM
lib/fuzzer/FuzzerMutationStats.cpp
26 ↗(On Diff #152798)

@Dor1s is OOO for 2 weeks.
But I think he was more concerned that the output be easy for libFuzzer to consume than human readable.
I believe he wanted it output as a CSV without any column names.

kodewilliams marked 5 inline comments as done.
morehouse accepted this revision.Tue, Jun 26, 2:48 PM
morehouse added inline comments.
test/fuzzer/fuzzer-mutationstats.test
3

You should be able to remove -print_final_stats=1 now.

kcc added inline comments.Tue, Jun 26, 5:21 PM
lib/fuzzer/FuzzerLoop.cpp
704

Shouldn't we call this near the call to RecordSuccessfulMutationSequence, or even inside it?
Also, I think it's find to collect the stats even when printing stats is not requested,
as soon as the stats collection is cheap. This way will have less code.

lib/fuzzer/FuzzerMutate.h
22

ouch. please find a way w/o introducing this enum

lib/fuzzer/FuzzerMutationStats.h
1 ↗(On Diff #152956)

this is a bit over-engineered.
Just put this into FuzzerMutate.h and please make it simpler.

I guess it could be two extra Vector<uint64_t> objects inside MutationDispatcher
which you resize in the MutationDispatcher CTOR.

kodewilliams marked 2 inline comments as done.

Removed FuzzerMutationStats.* files and simplified implementation inside FuzzerMutate.

kodewilliams marked an inline comment as done.Wed, Jun 27, 6:08 PM
kcc added inline comments.Wed, Jun 27, 6:17 PM
lib/fuzzer/FuzzerMutate.cpp
33–34

This is still pretty gross. You don't need to assign numbers to mutations.
When you add a mutation to DefaultMutators that becomes the mutation's number.
And so you also should not need "int Identifier;" above.

66

you should use Mutators.size() here in stead of kNumMutationTypes

524

instead of having M.Identifier, change the line above

auto M = Mutators[Rand(Mutators.size())];

to something like

size_t MutatorIdx = Rand(Mutators.size());   
auto M = Mutators[MutatorIdx];

and then use MutatorIdx

lib/fuzzer/FuzzerMutate.h
22

you should not need this.

152

check text alignment.

lib/fuzzer/FuzzerOptions.h
56

stale?

kodewilliams marked 6 inline comments as done.Wed, Jun 27, 7:35 PM
kodewilliams added inline comments.
lib/fuzzer/FuzzerMutate.cpp
524

@kcc I had been using the mutation identifier to increment useful counts but now since it is gone, the only way I can think of getting the correct index inside CountCurrentMutatorSequence is by looking up each element in the vector to get it's index then using that, which is expensive. Since this is not done a very large number of times, is it acceptable?

kodewilliams marked an inline comment as done.
kodewilliams marked an inline comment as done.Fri, Jun 29, 8:58 AM
kcc added a comment.Fri, Jun 29, 11:38 AM

the current patch is hard to review as it contains tons of formatting (or otherwise unrelated) fixed.
Plz don't mix formatting/refactoring with meaningful changes in one patch.

FuzzerDefs.h
19 ↗(On Diff #153415)

plz don't include unrelated changes, they make the review harder.

FuzzerMutate.cpp
68 ↗(On Diff #153415)

again, don;t include unrelated changes.
here and below.

kodewilliams marked 2 inline comments as done.

@kcc Insignificant changes are undone. PTAL.

kcc added a comment.Fri, Jun 29, 1:18 PM

add a .lit test, please

FuzzerMutate.cpp
539 ↗(On Diff #153559)

still an unrelated change?

FuzzerMutate.h
98 ↗(On Diff #153559)

do you still need to move "private:"

134 ↗(On Diff #153559)

Now that you have CurrentMutatorIdxSequence you probably can get rid of CurrentMutatorSequence

kodewilliams marked 3 inline comments as done.

Re-added test and removed other insignificant changes.

kcc added a comment.Mon, Jul 2, 6:04 PM

a couple of nits, then good to go.

lib/fuzzer/FuzzerMutate.cpp
544

the loop bound is a size_t, so please use size_t

lib/fuzzer/FuzzerMutate.h
91

Maybe try to find a better name.
(Nitpicking, feel free to ignore)

kodewilliams marked 2 inline comments as done.

Fixed final nits, good to go @kcc

kcc accepted this revision.Tue, Jul 3, 3:56 PM

LGTM

kodewilliams added a comment.EditedMon, Jul 9, 1:02 PM

@kcc @morehouse requesting these changes be landed.

kcc added a comment.Mon, Jul 9, 1:13 PM

Matt, please land it

morehouse closed this revision.Mon, Jul 9, 1:26 PM

Landed in r336597.

morehouse reopened this revision.Mon, Jul 9, 3:38 PM

Reverted due to bot breakage in r336616.

FAIL: libFuzzer :: fuzzer-custommutator.test (6330 of 6439)
******************** TEST 'libFuzzer :: fuzzer-custommutator.test' FAILED ********************
Script:
--
: 'RUN: at line 1';     /b/sanitizer-x86_64-linux/build/llvm_build64/bin/clang  --driver-mode=g++ -std=c++11 -lstdc++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer -m64 /b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/fuzzer/CustomMutatorTest.cpp -o /b/sanitizer-x86_64-linux/build/compiler_rt_build/test/fuzzer/Output/fuzzer-custommutator.test.tmp-CustomMutatorTest
: 'RUN: at line 2';   not  /b/sanitizer-x86_64-linux/build/compiler_rt_build/test/fuzzer/Output/fuzzer-custommutator.test.tmp-CustomMutatorTest 2>&1 | FileCheck /b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/fuzzer/fuzzer-custommutator.test --check-prefix=LLVMFuzzerCustomMutator
--
Exit Code: 1

Command Output (stderr):
--
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/fuzzer/fuzzer-custommutator.test:4:26: error: expected string not found in input
LLVMFuzzerCustomMutator: BINGO
                         ^
<stdin>:8:1: note: scanning from here
=================================================================
^
<stdin>:9:9: note: possible intended match here
==57316==ERROR: AddressSanitizer: attempting double-free on 0x602000000050 in thread T0:
        ^

See http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fuzzer/builds/16996.

This revision is now accepted and ready to land.Mon, Jul 9, 3:38 PM

@morehouse I have been looking over the code and I cannot seem to find a reason why that single test is failing. PTAL.

@kodewilliams have you rerun the tests to find the failing one?

I have seen the same failure when the commit was landed and I debugged it a little bit. The problem was that CustomMutatorTest.cpp's LLVMFuzzerCustomMutator implementation calls LLVMFuzzerMutate, which uses the default set of mutators. This causes a discrepancy: MutateImpl will log into CurrentMutatorIdxSequence that a mutator with an index between 0..12 (from the default set) was used, but when printing the sequence (in PrintMutationSequence), we only expect to find indexes from MutationDispatcher::Mutators. When using a custom mutator (like in this test), MutationDispatcher::Mutators only has one element. So PrintMutationSequence overflows the vector when it tries to use an index from 0..12. Add a if (M >= Mutators.size()) abort(); line into PrintMutationSequence and run the test to reproduce this.

@morehouse I have been looking over the code and I cannot seem to find a reason why that single test is failing. PTAL.

I suspect this has something to do with LLVMFuzzerMutate being called from CustomMutatorTest.cpp, which uses DefaultMutators instead of Mutators. So your CurrentMutatorIdxSequence vector gets indexes from both DefaultMutators and Mutators.

@morehouse tests are passing now.

morehouse added inline comments.Wed, Jul 11, 2:09 PM
lib/fuzzer/FuzzerMutate.cpp
523–524

I don't like this solution. If we don't want to support mutation stats for custom mutators, we should print an error and exit before any fuzzing happens.

Otherwise, we should make stats work with custom mutators too.

metzman added inline comments.Wed, Jul 11, 2:12 PM
lib/fuzzer/FuzzerMutate.cpp
523–524

+1 for exit.

kodewilliams marked 2 inline comments as done.

Tests now all pass and custom mutations can be tracked.

Dor1s updated this revision to Diff 155719.Mon, Jul 16, 10:54 AM

Rebase and getting ready to commit.

Dor1s retitled this revision from [libFuzzer] Mutation tracking and logging implemented to [libFuzzer] Mutation tracking and logging implemented..Mon, Jul 16, 10:55 AM
Dor1s edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
Dor1s added a comment.Mon, Jul 16, 1:00 PM

I'm seeing a build failure on one of the bots http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/13625/steps/build%20with%20ninja/logs/stdio :

[3726/4950] Building CXX object projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerMutate.cpp.o
FAILED: projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerMutate.cpp.o 
/b/sanitizer-x86_64-linux/build/clang_build/bin/clang++   -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/fuzzer -I/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer -I/usr/include/libxml2 -Iinclude -I/b/sanitizer-x86_64-linux/build/llvm/include -gmlt -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -std=c++11 -Wno-unused-parameter -O3    -UNDEBUG  -gmlt -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Werror -std=c++11 -Wno-unused-parameter -m64 -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -nostdinc++ -D_LIBCPP_ABI_VERSION=Fuzzer -fno-omit-frame-pointer -isystem /b/sanitizer-x86_64-linux/build/llvm_build_ninja/projects/compiler-rt/lib/fuzzer/libcxx_fuzzer_x86_64/include/c++/v1 -MD -MT projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerMutate.cpp.o -MF projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerMutate.cpp.o.d -o projects/compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerMutate.cpp.o -c /b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:33:64: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_EraseBytes, "EraseBytes"},
                                                               ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:34:64: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_InsertByte, "InsertByte"},
                                                               ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:36:33: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
           "InsertRepeatedBytes"},
                                ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:37:64: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_ChangeByte, "ChangeByte"},
                                                               ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:38:62: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_ChangeBit, "ChangeBit"},
                                                             ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:39:68: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_ShuffleBytes, "ShuffleBytes"},
                                                                   ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:40:76: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_ChangeASCIIInteger, "ChangeASCIIInt"},
                                                                           ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:41:75: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_ChangeBinaryInteger, "ChangeBinInt"},
                                                                          ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:42:60: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_CopyPart, "CopyPart"},
                                                           ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:43:62: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
          {&MutationDispatcher::Mutate_CrossOver, "CrossOver"},
                                                             ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:45:24: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
           "ManualDict"},
                       ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:47:26: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
           "PersAutoDict"},
                         ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:51:60: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
        {&MutationDispatcher::Mutate_AddWordFromTORC, "CMP"});
                                                           ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:54:69: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
    Mutators.push_back({&MutationDispatcher::Mutate_Custom, "Custom"});
                                                                    ^
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/lib/fuzzer/FuzzerMutate.cpp:60:72: error: missing field 'UsefulCount' initializer [-Werror,-Wmissing-field-initializers]
        {&MutationDispatcher::Mutate_CustomCrossOver, "CustomCrossOver"});
                                                                       ^
15 errors generated.
Dor1s reopened this revision.Mon, Jul 16, 1:12 PM

Reverted in https://reviews.llvm.org/rCRT337206.

Kode, please add -DLLVM_ENABLE_WERROR=ON flag to your cmake command to reproduce the failure locally.

This revision is now accepted and ready to land.Mon, Jul 16, 1:12 PM
Dor1s added a comment.Mon, Jul 16, 1:19 PM

Actually, I think you might've already used that flag (it is used in LLVM Dev Starting Kit). In that case, you would need to add -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++, I think.

Had to initialize the counters to silence build bot warnings and errors.

morehouse accepted this revision.Tue, Jul 17, 10:44 AM

Hey Matt, do you think we can land this or should also wait for Kostya's approval?

I think it's fine to land. Kostya approved the previous version, and he probably won't mind the extra 0's in the initialization lists.

It isn't as clean looking, but it silences the warning, and it's probably good practice to specify all fields in the list anyway.

This revision was automatically updated to reflect the committed changes.