Use const to Inform CmpLog Replacements

[DanBlackwell]

SanitizerCoverage TraceCmp has 2 sets of callbacks; the below is taken verbatim from the documentation:

// Called before a comparison instruction.
// Arg1 and Arg2 are arguments of the comparison.
void __sanitizer_cov_trace_cmp1(uint8_t Arg1, uint8_t Arg2);
void __sanitizer_cov_trace_cmp2(uint16_t Arg1, uint16_t Arg2);
void __sanitizer_cov_trace_cmp4(uint32_t Arg1, uint32_t Arg2);
void __sanitizer_cov_trace_cmp8(uint64_t Arg1, uint64_t Arg2);

// Called before a comparison instruction if exactly one of the arguments is constant.
// Arg1 and Arg2 are arguments of the comparison, Arg1 is a compile-time constant.
// These callbacks are emitted by -fsanitize-coverage=trace-cmp since 2017-08-11
void __sanitizer_cov_trace_const_cmp1(uint8_t Arg1, uint8_t Arg2);
void __sanitizer_cov_trace_const_cmp2(uint16_t Arg1, uint16_t Arg2);
void __sanitizer_cov_trace_const_cmp4(uint32_t Arg1, uint32_t Arg2);
void __sanitizer_cov_trace_const_cmp8(uint64_t Arg1, uint64_t Arg2);

Currently, libafl redirects the __sanitizer_cov_trace_const_cmpX functions to call __sanitizer_cov_trace_cmpX. Knowing that an argument is constant, can however inform the replacement process as there is no value in finding a compile time constant in the input and replacing it with a runtime value. Consider the following program:

int LLVMFuzzerTestOneInput(u8 *input, u32 len) {
    if (input[3] == 16) {
        abort();
    }
    return 0;
}

If we had the input [16, 16, 16, 8], and we received the callback __sanitizer_cov_trace_const_cmp1(16, 8), we would currently try to first match the 1st argument and then replace it with the 2nd - then try to match the 2nd argument and replace it with the 1st. Assuming we start scanning for matches at index 0, we would immediately match the 16 and replace it with 8 resulting in the input [8, 16, 16, 8]. We don't get into the branch.

In this patch, we pass through the const information so that we don't try to match on the const and replace it. For this example that means that we only look for 8, and if we find it then replace with 16 - for the given input this has 100% success rate.

There is currently an experiment comparing this to the baseline for fuzzbench here. I expect the results may be 1% improvement at most, but still I think it makes sense to use all of the information available to the fuzzer.

[tokatoka]

looks great 👍

[tokatoka]

this "extended" one is only used for llvm pass instrumentation (which is not using sancov stuff so you don't add it for this.
(but it's doable to add the const checking logic to the llvm pass

[tokatoka]

line 222, too

[DanBlackwell]

Yeah, hence it is set to 0 for now (unless you wanted to create a separate CmpLog struct without the v0_is_const member). I think that for longer comparisons (such as u64 or memcmps), the chance of conincidentally matching a const value is lower (apart from values like 0); so it's probably not worth the extra effort.

[tokatoka]

what i mean is struct CmpLogInstructionExtended doesn't have v0_is_const member

[DanBlackwell]

True

[tokatoka]

this callback is for routines. you don't need it here

[DanBlackwell]

Again, this is basically an unused struct member - I still think it's worth making sure that it's initialised, even if it is always 0.

[tokatoka]

hmm but this is a union. so you shouldn't be touching operand side of this struct.

[DanBlackwell]

Good spot!

[DanBlackwell]

Looks like it makes some progress on mbedtls:

Nothing too significant on other programs from fuzzbench, but I think this is a 'safe' optimisation in that it's not making any assumptions, just using more of the feedback available to it.

[DanBlackwell]

These macros are a bit DRY; I should pull out the repeating stuff.

[tokatoka]

it's ok

[DanBlackwell]

Have tidied them up a bit now

[DanBlackwell]

Ok, requested a new fuzzbench experiment with the fixes in place here. If that runs fine I'll mark this ready for review.

[domenukk]

Status? Got a link to the fuzzbench? :)

[DanBlackwell]

Status? Got a link to the fuzzbench? :)

Just running it again with the fixes that toka pointed out. The fuzzbench run is in progress here.

[domenukk]

The trial doesn't seem to run the normal version(?)

[DanBlackwell]

The trial doesn't seem to run the normal version(?)

The results get merged at the end of the run so it will have the baseline libafl (on whatever commit its currently pinned to) appear after that. Unfortunately it looks like almost all the runs have been getting pre-empted, so the outcome isn't really going to be reliable enough to draw any conclusions :/

@tokatoka has experience with fuzzbench's reliability as of late so may be able to comment more on what the best next steps are.

[tokatoka]

we can just merge this

[DanBlackwell]

I agree, I don't see how this PR could have a negative effect provided the implementation doesn't have any flaws.