Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mini CmpLog #324

Merged
merged 7 commits into from
May 9, 2023
Merged

Add mini CmpLog #324

merged 7 commits into from
May 9, 2023

Conversation

louismerlin
Copy link
Contributor

As suggested in #323, CmpLog could be added to afl.rs to improve effectiveness.

Testing this change on the following example will find the crash immediately instead of after a minute.

I can see no performance drop, some more benchmarking might be needed.

You need to specify the -c 0 option (or -c path/to/your/binary).

fn main() {
    afl::fuzz!(|data: &[u8]| {
        if data.len() != 6 {return}
        if data[0] != b'q' {return}
        if data[1] != b'w' {return}
        if data[2] != b'e' {return}
        if data[3] != b'r' {return}
        if data[4] != b't' {return}
        if data[5] != b'y' {return}
        panic!("BOOM")
    });
}

I also tried adding the following flags but they resulted in compilation errors of the target.

 -C llvm-args=-sanitizer-coverage-trace-geps \
 -C llvm-args=-sanitizer-coverage-trace-loads \
 -C llvm-args=-sanitizer-coverage-trace-stores \

@vanhauser-thc
Copy link
Contributor

vanhauser-thc commented Apr 26, 2023

-c 0 is the easier way to do that, had forgotten to write that in the issue.
I recommend to add this directly to the cargo run in this PR.

there is a performance loss, about 10% lower execution speed, that could be circumvented by compiling the target twice, once normally and once for minicmplog, and passing the binary of minicmplog via -c minicmplogcompiledtarget, but that just complicates things.

The performance loss in execution speed is overall less that the effectiveness gained by enabling this feature.

@smoelius
Copy link
Member

You need to specify the -c 0 option (or -c path/to/your/binary).

What do these options do?

@vanhauser-thc
Copy link
Contributor

You need to specify the -c 0 option (or -c path/to/your/binary).

What do these options do?

Both do the same thing (-c0 is easier) - they tell afl-fuzz that cmplog data is available. Otherwise the data is ignored.

@smoelius smoelius mentioned this pull request Apr 26, 2023
@louismerlin
Copy link
Contributor Author

I added an example in the examples directory which demonstrates the capabilities of this mini CmpLog.

@vanhauser-thc
Copy link
Contributor

isnt it possible to automatically pass -c 0 to afl-fuzz when doing cargo fuzz? the user will very likely not know to put -c 0 as command line parameter on his own.

@louismerlin
Copy link
Contributor Author

isnt it possible to automatically pass -c 0 to afl-fuzz when doing cargo fuzz? the user will very likely not know to put -c 0 as command line parameter on his own.

Added in 4a60756

src/bin/cargo-afl.rs Outdated Show resolved Hide resolved
@vanhauser-thc
Copy link
Contributor

LGTM, can be merged in my opinion.

examples/cmplog.rs Outdated Show resolved Hide resolved
@smoelius
Copy link
Member

smoelius commented May 5, 2023

I'm trying to test this, and I'm getting the following error for some projects:

undefined reference to '__sanitizer_cov_trace_div8'

It appears to have something to do with build scripts.

For example, cargo afl build in the root of this repository gives me that error: https://github.com/paholg/typenum

Does it work for you?

Removing this line makes the problem go away:

-C llvm-args=-sanitizer-coverage-trace-divs \

@vanhauser-thc
Copy link
Contributor

I'm trying to test this, and I'm getting the following error for some projects:

undefined reference to '__sanitizer_cov_trace_div8'

It appears to have something to do with build scripts.

For example, cargo afl build in the root of this repository gives me that error: https://github.com/paholg/typenum

Does it work for you?

Removing this line makes the problem go away:

-C llvm-args=-sanitizer-coverage-trace-divs \

that is weird as this parameter is already present in ancient llvm 9...
clang-9 -o test-instr -fsanitize-coverage=trace-div test-instr.c

trace-div is not that important of an option, it has more value for a technique called value profiling which afl++ does not use (because better technique exists, but libfuzzer uses that).

@smoelius
Copy link
Member

smoelius commented May 6, 2023

Does your comment imply you were able to reproduce the error, @vanhauser-thc?

EDIT: That is not to insinuate that you must try.

@louismerlin
Copy link
Contributor Author

I was able to reproduce the bug, I've removed the trace-divs flag. Thanks!

@smoelius smoelius merged commit d111311 into rust-fuzz:master May 9, 2023
@smoelius
Copy link
Member

smoelius commented May 9, 2023

@louismerlin Thanks very much for this. And, @vanhauser-thc, thank you for the review, and for sharing your expertise.

In the interest of caution, I am going to treat this as a breaking change. There is at least one other thing I wanted to include with the next breaking change (#276). So it will be a few days before I publish this. But I promise to try to push a new release soon.

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants