Repository navigation
Fuzz test with byte[] parameter with mutation annotation is treated as classic fuzz test / annotations are ignored #1022
Description
Activity
So far we have avoided addressing this, because solving this properly will be a breaking change for most OSS-Fuzz fuzz tests.
Your proposal will not cause breaking change in OSS-Fuzz, but will have the following consequences:
- adding a
@NotNullannotation suddenly also forces the byte array into the range of 0-1000 bytes--the default in the mutation framework - adding only
@WithLengthsuddenly allows the byte array to benull(in raw mode the byte array is never null). - switching between both raw/mutation framework might invalidate the corpus and set back the coverage
I think, it's better to be consistent and always use mutation framework by default for any number of parameters.
Users can still opt-out to raw fuzzing through a configuration optionThis will 1) break most fuzz tests in OSS-Fuzz because in raw mode the byte array is never
null, but in the mutation framework it can, unless annotated with@NotNull; 2) adding@NotNullas a countermeasure will invalidate all the corpus so far (because ofwritevs.writeExclusivethat is used for the last param)The mutation framework is currently already inconsistent:
- the most annoying for me personally: adding another parameter will switch to using mutation framework that will now force its limits on the first parameter (byte [] can suddenly be
nulland only have length between 0-1000 bytes) - adding another parameter invalidates the corpus for the butlast parameter (mutation framework writes the last param with
writeExclusive, and all others withwrite, which leads to this inconsistency). - adding
@NotNullannotation to the last parameter invalidates the corpus entries for it because of the samewrite/writeExclusiveissue
To work around these issues, I use a dummy boolean as last parameter in most of my fuzz tests, such that adding annotations/additional parameters will not invalidate my corpus.
- adding a
You could add a compatibility mode and enable it globally in OSS-Fuzz via a flag in the Jazzer wrapper. But you would need to be careful and monitor Monorail 🙂
Reacted by Peter SamarinYour proposal will not cause breaking change in OSS-Fuzz, but will have the following consequences: [...]
But it would be a deliberate decision then by the user, since they explicitly added mutation framework annotations.
adding a
@NotNullannotation suddenly also forces the byte array into the range of 0-1000 bytesThere might not be any need for the user to do this though since (as you mention) the array is always non-
nullin classic mode. So adding@NotNullmight only be relevant if the user wants to add other annotations as well.
switching between both raw/mutation framework might invalidate the corpus and set back the coverage
Isn't that a general problem when you change the signature of a fuzz test?
Though you have a good point regarding the remaining inconsistencies, and I guess you both have more insight into the internals and which solution might be best.
Maybe as first step the proposed solution of detecting the annotations could work though? And the other inconsistencies / larger issues could be solved afterwards?
Just looking at the two functions below, I would expect the fuzzer to give me values from the same domain for the two byte arrays. But surprisingly, their domains are completely different. Your proposal would leave this behavior the same.
@FuzzTest public fuzzMe(byte[] data){} @FuzzTest public fuzzMe2(byte[] data, String format){}
And here, assuming we implement your proposal, the byte array of the first fuzz test cannot be
null, but in the second it can. It is again very surprising.@FuzzTest public fuzzMe(byte[] data){} @FuzzTest public fuzzMe(byte @WithLength(max=10) [] data){}
Users have to learn these surprises the hard way.
IMO the domain shouldn't depend on the number of parameters in the fuzz test, with and without annotations.The deliberate decision to use raw
byte[]fuzzing should be done by setting an already available option/env var/command line arg insteadmutator_framework=falseIf they have fuzz tests with a classic
byte[]signature they would be affected, but I'm hoping that these are easy enough to detect that you could set the compatibility flag for them and keep the current behavior for them.The length handling for a classic signature vs any other is just inconsistent from the perspective of someone not familiar with libFuzzer-style targets.
writeExclusivewould still be useful for fuzz tests that just consume binary data, but if I remember correctly they could adopt@NotNull byte[]with an appropriately long max length to keep the same corpus representation below that max size.I may very well be missing an unintended consequence though, it's been a long time.
Lets pick this issue back up again after the winter holidays...
A legacy mode that retains the current behavior for OSS Fuzz projects is definitely a good idea. That should give us some freedom to potentially introduce some breaking changes to the default
byte[]fuzz test handling.I agree with @oetr that switching between "classic" mode and the mutation framework by adding parameters or annotations can be quite counter intuitive. Ideally users should not have to know that there are these two different modes. The challenge is that simply turning on the mutator framework for
byte[]fuzz tests would likely perform worse than the "classic" mode due to the already mentioned default size limit andnullvalues. Even with@NotNulland a sufficiently large@WithLength(max=...)the corpus format would be different to the classic mode because PrimitiveArrayMutatorFactory.java does not implementreadExclusiveandwriteExclusive.IMO goals for a
@NotNull byte[]fuzz tests should be:- it respects libFuzzer length control handling -> better defaults, adjusts to the corpus and would respect libFuzzer flags
- corpus format without extra length byte -> important for external corpora or other test data as seeds without having to modify it
The raw binary corpus format should be doable by implementing
read/writeExclusiveforPrimitiveArrayMutatorFactory.java. This of course would break the corpus format when adding new parameters to the fuzz test (as @oetr mentioned) but IMO that is preferable to not being able to use plain files as inputs for simple fuzz tests.
To solve the max length issue: Could we pass the libFuzzer max length to the last mutator if applicable and no@WithLengthannotation is set? A "quick fix" could also be to raise the default max size from 1000 to 4096 in line with the libFuzzer default max length (on an empty corpus).Still, we would change fuzz tests to suddenly produce
nullvalues and invalidate any existing corpus until the user adds@NotNull. Is this to much of a breaking change? Personally I would much prefer arrays being notnullby default and having a@Nullableannotation. 😄Reacted by Fabian MeumertzheimRe null/not null by default: Something I considered supporting back then but never got to was support for JSpecify annotations, which in particular comes with
@NullMarked. That could be applied to an entire class, package or even module, which means that it's no longer necessary to mark each fuzz test parameter individually. (The mid-term future will also bring!as part of Valhalla)Personally I would much prefer arrays being not null by default and having a
@Nullableannotation. 😄Perhaps we should do it at the same time, and this change won't be so breaking anymore?
I just checked some of my recent fuzz tests, and most of them have a@NotNullon every non-primitive object!Just a thought (from me as external person): On the other hand, having to explicitly use
@Nullablein the fuzz test will likely result in users forgetting to fuzz withnull, similar to how they might forget to properly handlenullin their code in the first place.But I also think that fuzzing null handling might often not be interesting since it usually does not lead to high severity issues, so fuzz tests become quite verbose due to
@NotNullusage.Would supporting
@NotNulladditionally as method annotation where it applies to all parameters (and their nested types) be useful maybe?
Version
jazzer-junit 0.28.0
Description
When a
@FuzzTesttakes only a singlebyte[]parameter but that parameter is annotated with mutation framework annotations (e.g.@WithLength), it is nonetheless treated as "classic" fuzz test and the annotations are ignored.This can be quite irritating, especially if a user is unaware of the "classic" fuzz test mode, or when they change the signature of a mutation framework fuzz test to only have a
byte[]parameter and suddenly the fuzz test turns into a "classic" one.How to reproduce
Run this fuzz test in regression mode:
❌ Problem: The test fails for the "<empty input>" run because the
@WithLengthannotation was ignored and the array is empty.Expected behavior
If a fuzz test takes only a single
byte[]parameter but that parameter is annotated (potentially nested, e.g. for arrays or parameterized types) with mutation framework annotations, then the fuzz test should be executed by the mutation framework.Might also need some tweaks to the https://github.com/CodeIntelligenceTesting/jazzer/blob/main/docs/mutation-framework.md documentation.