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

Do not pass -C passes=... when nightly is used and plugins are compiled #449

Merged
merged 6 commits into from
Jan 16, 2024

Conversation

vanhauser-thc
Copy link
Contributor

@vanhauser-thc vanhauser-thc commented Jan 15, 2024

Bug 1

This PR fixes just the major issue: when nightly is used and plugins are compiled - then still the default bad sancov of LLVM is used, because -C sancov is always passed.

I sadly have no time to fix the other two bugs, too time constraint :(

Bug 2
Another issue I found and not fixed in this PR:
AFL++'s LLVM plugins are not built by default.

And even if the user wants to build them the check is faulty:

$ cargo-afl afl config --plugins
AFL LLVM runtime was already built for Rust rustc-1.75.0-nightly-42b1224; run `cargo afl config --build --force` to rebuild it.
$ ls /home/marc/.local/share/afl.rs/rustc-1.75.0-nightly-42b1224/afl.rs-0.15.1/afl-llvm
libafl-llvm-rt.a  libafl-llvm-rt.o

The message says "runtime" which would be correct, because the runtime is there, but what we want are the --plugins which are not.

As a workaround --force can be added which will enforce building the plugins.

Bug 3
And finally: there is not way for a user to see if AFL++'s plugins will be used or not. My original change to the version command made this visible, now the only way to see it to use -vv while builind a target.

@smoelius
Copy link
Member

I sadly have no time to fix the other two bugs, too time constraint :(

No worries. Thank you very much for reporting the issues.

when nightly is used and plugins are compiled - then still the default bad sancov of LLVM is used, because -C sancov is always passed.

This is not a regression, correct? I just checked, and it appears that -C passes=sancov-module was passed immediately after #392 was merged.

Also, why does AFL++ need to be updated? I would very much prefer to use stable releases.

I have a few other line-specific comments.

@@ -253,7 +252,7 @@ where
environment_variables.insert("ASAN_OPTIONS", asan_options);
environment_variables.insert("TSAN_OPTIONS", tsan_options);

if plugins_available() {
if plugins_available() && is_nightly() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was is_nightly added here? Note that the assert two lines below checks that the compiler is nightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested something else and forgot to remove it, on the other hand it does not hurt either :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the other hand it does not hurt either :)

I'm not sure I agree with this.

The is_nightly check is to ensure that a case not expected to occur in principle does not occur in practice. That case is that the plugins were built with a non-nightly compiler.

Suppose that case were to occur in practice. Without the change, a panic occurs. With the change, cargo-afl silently reverts to the non-nightly (i.e., non-plugin) behavior.

To me, the former sounds preferable. But you don't agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both are good and bad.
with the panic - does a user know what to do to fix it? if not then silently taking the alternative is IMHO better. If the error message clearly states how ti fix it, then panic is better.
as I said, no time currently :(

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

well it is a regression for me because I worked with the fork of bruno before it was merged here, and there everything was fine :)

I updated AFL++ because when nightly moves to LLVM 18 building the plugins will fail.

@smoelius
Copy link
Member

I updated AFL++ because when nightly moves to LLVM 18 building the plugins will fail.

Gotcha.

Assuming this Reddit thread is accurate, it sounds like we have a couple more months until that becomes an issue: https://www.reddit.com/r/rust/comments/183ufxn/rusts_llvm_roadmap/

I would prefer that afl.rs users not be forced to use bleeding-edge software when stable software would work.

Could we please delay the AFL++ update until it becomes necessary?

@smoelius
Copy link
Member

well it is a regression for me because I worked with the fork of bruno before it was merged here, and there everything was fine :)

Also, is there a test we could add that would prevent this problem from recurring for you?

@vanhauser-thc
Copy link
Contributor Author

if you can merge this and make a new release latest tomorrow that would be great as I will be giving a training involving afl.rs and that when this issue is gone that would be less trouble .... thanks!

@smoelius
Copy link
Member

if you can merge this and make a new release latest tomorrow that would be great as I will be giving a training involving afl.rs and that when this issue is gone that would be less trouble .... thanks!

OK. NP. I'm going to remove the added is_nightly check though.

@smoelius smoelius changed the title Major regressions Do not pass -C passes=... when nightly is used and plugins are compiled Jan 16, 2024
@smoelius smoelius added this pull request to the merge queue Jan 16, 2024
Merged via the queue into rust-fuzz:master with commit 2f55ff1 Jan 16, 2024
12 checks passed
@smoelius
Copy link
Member

Version 0.15.2 has the fix for Bug 1.

@vanhauser-thc
Copy link
Contributor Author

Thanks!

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.

2 participants