This is an archive of the discontinued LLVM Phabricator instance.

[LibFuzzer] Tweak `MutationDispatcher::Mutate_CopyPart` mutation.
ClosedPublic

Authored by delcypher on Apr 16 2018, 9:38 AM.

Details

Summary

[LibFuzzer] Tweak MutationDispatcher::Mutate_CopyPart mutation.

It doesn't make sense to non-deterministically choose between
CopyPart(..) and InsertPart(..) when it is known that
InsertPart(..) will fail.

This upstream's a change from JFS solver's fork of LibFuzzer.

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Apr 16 2018, 9:38 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptApr 16 2018, 9:38 AM
kcc accepted this revision.Apr 16 2018, 1:06 PM

LGTM
I wonder how you can observe the change?
It's just a slight change in probabilities.
Or not slight?

I prefer to test similar changes in lib/fuzzer/tests/FuzzerUnittest.cpp
but I don't see how to test this one...

This revision is now accepted and ready to land.Apr 16 2018, 1:06 PM
In D45693#1069081, @kcc wrote:

LGTM
I wonder how you can observe the change?
It's just a slight change in probabilities.
Or not slight?

To give some context, this patch is actually part of a larger change made to JFS's copy of LibFuzzer. For JFS, inputs never change size, so it's a waste of time to perform mutations that change the input size.
To fix this in JFS we added a command line option to disable all mutations that change the input size. I don't think our approach is general enough to be upstreamed. However the change in this patch
seemed general enough to be upstreamed. At some point I plan to start a discussion on how to fix JFS's problem in a more general way on the LibFuzzer mailing list. I just haven't got around to posting yet.

I prefer to test similar changes in lib/fuzzer/tests/FuzzerUnittest.cpp
but I don't see how to test this one...

A possible way to test this might be to call Mutate_CopyPart directly with a input that is MaxSize in size and make sure that it returns MaxSize (i.e. input size is not changed).

kcc added a comment.Apr 18 2018, 12:07 PM

Ok, sounds good.
Yes, please add a test that calls Mutate_CopyPart many times and always sees it return MaxSize

Add unit test.

In D45693#1071306, @kcc wrote:

Ok, sounds good.
Yes, please add a test that calls Mutate_CopyPart many times and always sees it return MaxSize

@kcc: I've added a test. Is this good enough?

kcc accepted this revision.Apr 20 2018, 8:21 AM

LGTM with a nit.

lib/fuzzer/tests/FuzzerUnittest.cpp
394 ↗(On Diff #143238)

Why unsigned int?

This code uses 'int' or 'size_t', depending on the context.
size_t for something that is a size, 'int' for just iterations.

unsigned int is longer to spell and is inconsistent.

delcypher added inline comments.Apr 23 2018, 3:41 AM
lib/fuzzer/tests/FuzzerUnittest.cpp
394 ↗(On Diff #143238)

The only reason was I was worried about someone writing the loop bound to be so large that it became negative when using int. This isn't is a particularly good reason because there are a plethora of other ways that someone can still screw up using unsigned int so I'll switch to using int and then commit.

s/unsigned int/int/ in test case.

delcypher marked 2 inline comments as done.Apr 23 2018, 11:33 PM
This revision was automatically updated to reflect the committed changes.