-
Notifications
You must be signed in to change notification settings - Fork 108
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 plugins feature #392
Add plugins feature #392
Conversation
Pipelines seems to break at |
Thank you very much for this PR. It's probably going to take a few rounds of review for me to understand all of what has changed, so please be patient with me. I will start on this soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for doing this. I like the approach of using a Cargo feature.
Again, I may need to look at this a few times. So thanks in advance for your patience.
afl/examples/cmplog.rs
Outdated
@@ -26,6 +26,13 @@ fn main() { | |||
return; | |||
}; | |||
|
|||
let slice = &data[16..27]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let slice = &data[16..27]; | |
let slice = &data[16..]; |
I think the 27 version could panic(?), because data
is only checked to be of length >= 24
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in last commit
cargo-afl/build.rs
Outdated
|
||
let status = command | ||
.status() | ||
.expect("could not run 'git submodule update'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the use of git submodule init
added just as a convenience? Or is the answer more complicated than that?
I'm not 100% certain, but I think think will break when others try to download this from crates.io. As I learned recently, when you publish to crates.io, cargo takes a snapshot of the submodule directory, and then the fact that it is a submodule is forgotten.
Also, it looks like a bunch of code was deleted, e.g., code related to building on docs.rs. Was that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment on line 99
cargo-afl/src/common.rs
Outdated
let llvm_version = version_meta.llvm_version.unwrap().major.to_string(); | ||
let mut llvm_config = "llvm-config-".to_string(); | ||
llvm_config.push_str(&llvm_version); | ||
llvm_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work?
format!("llvm-config-{llvm_version}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in the last commit
// skip the checks for the legacy x86 afl-gcc compiler | ||
.env("AFL_NO_X86", "1") | ||
// build just the runtime to avoid troubles with Xcode clang on macOS | ||
.env("NO_BUILD", "1") | ||
//.env("NO_BUILD", "1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self to revisit this.
cargo-afl/src/bin/cargo-afl.rs
Outdated
@@ -343,6 +376,9 @@ where | |||
.env("RUSTDOCFLAGS", &rustdocflags) | |||
.env("ASAN_OPTIONS", asan_options) | |||
.env("TSAN_OPTIONS", tsan_options) | |||
.env("AFL_LLVM_INSTRUMENT", "PCGUARD") | |||
.env("AFL_LLVM_CMPLOG", "1") | |||
.env("AFL_QUIET", "1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be guarded by the cmplog
feature?
If that's correct, then I would set these in the same if
block above where the -Z
flags were added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitted this into a Hashmap to be included in the cmplog feature, let me know if that's more adequate
Thanks a lot for the review! I'll take a look at it next week and make the requested changes |
Some context why this is something someone would want: it allows solving eg let slice16 = &data[20..36]; Plus the string matching you see added in the example plus floating point solving - all things that libfuzzer and honggfuzz can’t do. |
Did a first round of fixes for most of the conversations, let me know if I missed something. Could you take another look? |
cargo-afl/build.rs
Outdated
if cfg!(feature = "cmplog") { | ||
llvm_config = common::get_llvm_config(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this logic be moved inside build_afl
(since llvm_config
is not needed elsewhere)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the latest commit
cargo-afl/build.rs
Outdated
@@ -14,67 +13,75 @@ static AR_CMD: &str = "ar"; | |||
mod common; | |||
|
|||
fn main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still appears to me that a bunch of logic was removed from this function:
Lines 17 to 51 in 777a97b
let installing = home::cargo_home() | |
.map(|path| Path::new(env!("CARGO_MANIFEST_DIR")).starts_with(path)) | |
.unwrap(); | |
let out_dir = env::var("OUT_DIR").unwrap(); | |
// smoelius: Build AFLplusplus in a temporary directory when installing or when building on docs.rs. | |
let work_dir = if installing || env::var("DOCS_RS").is_ok() { | |
let tempdir = tempfile::tempdir_in(&out_dir).unwrap(); | |
if Path::new(AFL_SRC_PATH).join(".git").is_dir() { | |
let status = Command::new("git") | |
.args(["clone", AFL_SRC_PATH, &*tempdir.path().to_string_lossy()]) | |
.status() | |
.expect("could not run 'git'"); | |
assert!(status.success()); | |
} else { | |
fs_extra::dir::copy( | |
AFL_SRC_PATH, | |
tempdir.path(), | |
&fs_extra::dir::CopyOptions { | |
content_only: true, | |
..Default::default() | |
}, | |
) | |
.unwrap(); | |
} | |
tempdir.into_path() | |
} else { | |
PathBuf::from(AFL_SRC_PATH) | |
}; | |
let base = if env::var("DOCS_RS").is_ok() { | |
Some(PathBuf::from(out_dir)) | |
} else { | |
None | |
}; |
I assume that was a mistake. Could you please re-add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was initially because of the compilation errors we had and discussed before, all these have been reverted to the master branch's way. Done in the latest commit.
cargo-afl/build.rs
Outdated
"cargo-afl must be compiled with nightly for the cmplog feature" | ||
); | ||
|
||
// check if llvm tools are installed and with the good version for the plugin compilation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and with the good version
What does this comment refer to? It suggests the output of llvm-config --version
should be checked. Is that missing?
cargo-afl/build.rs
Outdated
.arg(common::object_file_path(base)) | ||
.status() | ||
.expect("could not run 'ar'"); | ||
assert!(status.success()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this use of ar
was copied from build_afl_llvm_runtime
by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, good catch, removed
cargo-afl/src/bin/cargo-afl.rs
Outdated
// Make sure we are on nightly for the -Z flags | ||
assert!( | ||
rustc_version::version_meta().unwrap().channel == rustc_version::Channel::Nightly, | ||
"cargo-afl must be compiled with nightly for the cmplog feature" | ||
); | ||
|
||
let llvm_config = common::get_llvm_config(); | ||
|
||
// check if llvm tools are installed and with the good version for the plugin compilation | ||
let mut command = Command::new(llvm_config.clone()); | ||
command.args(["--version"]); | ||
let status = command | ||
.status() | ||
.unwrap_or_else(|_| panic!("could not run {llvm_config} --version")); | ||
assert!(status.success()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code appears nearly verbatim in build.rs
. I think it is fine to perform these checks in just build.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you prefer, we wanted to make sure that the environment is the same when using it, let me remove that. I'll also put it in the build.rs instead of common.rs in that case
cargo-afl/build.rs
Outdated
@@ -94,6 +101,33 @@ fn build_afl_llvm_runtime(work_dir: &Path, base: Option<&Path>) { | |||
assert!(status.success()); | |||
} | |||
|
|||
fn build_afl_llvm_plugins(work_dir: &Path, base: Option<&Path>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn build_afl_llvm_plugins(work_dir: &Path, base: Option<&Path>) { | |
fn copy_afl_llvm_plugins(work_dir: &Path, base: Option<&Path>) { |
Nit, since the pluigns were actually built in build_afl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the latest commit
cargo-afl/src/bin/cargo-afl.rs
Outdated
-Z llvm-plugins={p}/cmplog-routines-pass.so \ | ||
-Z llvm-plugins={p}/cmplog-switches-pass.so \ | ||
-Z llvm-plugins={p}/SanitizerCoveragePCGUARD.so | ||
" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" | |
" |
Nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the latest commit
Thanks for your continued patience, @brunoproduit. |
No problem! @smoelius If you could take yet another look, I should have resolved all the new conversations |
I'm likely going to request more changes, but I want to be sure this is going to work in CI. So, I'm going to push to your branch. But I won't touch any files that you've modified. Ok? |
I need to find the right syntax for the GitHub matrix and macOS's sed. I'll get back to this soon. |
When I Mac is a platform that we support, and I expect many Mac users will install llvm with Homebrew. Could you please adjust the build script so that |
We do need to check that the major version matches the rustc llvm version, else it won't work. I'll set an if that sets |
That sounds perfect. Thank you! |
Did the change and refactored a bit around it. Also it now only sets LLVM_CONFIG if cmplog is activated |
I'm going to push more changes to rust.yml, but I will stay out of your code. |
btw I added support for two advanced features:
also in tests I see that using the AFL++ instrumentation module vs. the llvm sancov creates a x2 speed increase (without any drawbacks and an even better coverage gathering). |
@brunoproduit I'm pushing to your branch to get the fix for AFLplusplus/AFLplusplus#1898. |
Should we change the feature name to something else now that this activates more than cmplog? |
Oh, I didn't make that connection. What is the full list of "features"? (I'm wondering if it would make sense to enable them independently.) Separately, I am trying to diagnose why the shared libraries cannot be loaded on macOS (see here). From what I can tell, it's not a path problem, i.e., I expect that Please share if you know what the cause could be. |
Features are
IMO, we shouldn't separate as they all have the same requirements. Either you want to add addons from AFL, or not. Haven't had time to take a look at the macos issue yet, sorry |
Thanks.
I'm not sure I agree, but we don't have to decide this now. I agree that "llvm-afl-plugins" would be a better feature name. |
Can we please shorten the flag to “plugins”? Imho it is too long otherwise |
That works for me. |
This is the error I get locally:
|
@vanhauser-thc Can I ask why AFLplusplus's macos-latest tests are disabled? |
Because there are often unpredictable changes. Eg Apple change how to preload libraries and supporting this would be quite an effort. Or the he crippled Xcode clang. And brew llvm also changes from time to time how it behaves. Finally GitHub’s mac container also changes sometimes where otherwise it works perfectly on my Mac mini. |
In CLANG_LFL += -Wl,-flat_namespace -Wl,-undefined,suppress to this one: CLANG_LFL += -Wl,-flat_namespace -Wl,-undefined,suppress `$(LLVM_CONFIG) --libs` I get a different error:
Does this mean anything to you? @brunoproduit @vanhauser-thc |
MacOS is difficult. brew needs to install the right llvm version, there all the time changes in brew and Mac - for me as a non-user of MacOS it is very difficult to be on top of this. I would simply propose to not support the plugin feature for MacOS, someone can do it by hand if they want to,. |
@smoelius please note that I changed the output of the --version command. IHMO it is important to see if afl.rs was compiled normal or with the plugins feature. if you want it to look different, just edit away or tell me how else you would like it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think once these changes are made, this should be mergeable.
cargo-afl/src/bin/cargo-afl.rs
Outdated
concat!($s1, $s2) | ||
}; | ||
} | ||
|
||
#[allow(clippy::too_many_lines)] | ||
fn clap_app() -> clap::Command { | ||
use clap::{value_parser, Arg, Command}; | ||
|
||
let help = "In addition to the subcommands above, Cargo subcommands are also \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let help = "In addition to the subcommands above, Cargo subcommands are also \ | |
const HELP: &str = "In addition to the subcommands above, Cargo subcommands are also \ |
And then change help
to HELP
below. That should make Clippy happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo-afl/src/bin/cargo-afl.rs
Outdated
#[allow(clippy::too_many_lines)] | ||
fn clap_app() -> clap::Command { | ||
use clap::{value_parser, Arg, Command}; | ||
|
||
let help = "In addition to the subcommands above, Cargo subcommands are also \ | ||
supported (see `cargo help` for a list of all Cargo subcommands)."; | ||
|
||
const VERSION: &'static str = if cfg!(feature = "plugins") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const VERSION: &'static str = if cfg!(feature = "plugins") { | |
const VERSION: &str = if cfg!(feature = "plugins") { |
Again, make Clippy happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo-afl/build.rs
Outdated
if cfg!(feature = "plugins") { | ||
let llvm_config = check_llvm_and_get_config(); | ||
environment_variables.insert("LLVM_CONFIG".to_string(), llvm_config); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than create a hash table, please move this if
-statement below and modify command
directly. Something like:
if cfg!(feature = "plugins") {
let llvm_config = check_llvm_and_get_config();
command.env("LLVM_CONFIG", llvm_config);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo-afl/build.rs
Outdated
|
||
let status = command | ||
.status() | ||
.expect("could not run 'make clean, make install'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.expect("could not run 'make clean, make install'"); | |
.expect("could not run 'make clean install'"); |
Nit. Technically, it's one command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo-afl/build.rs
Outdated
"split-compares-pass.so", | ||
"split-switches-pass.so", | ||
"SanitizerCoveragePCGUARD.so", | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all of the .so
files that are built, correct? Would it make sense to loop over all of the files using read_dir
, and then copy those with an so
extension? Or, is there a good argument for listing the files explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to fix those for readability and future-proofing, like if we want to exclude certain so's in the future. I changed this in the last commit
cargo-afl/src/bin/cargo-afl.rs
Outdated
#[allow(clippy::too_many_lines)] | ||
fn clap_app() -> clap::Command { | ||
use clap::{value_parser, Arg, Command}; | ||
|
||
let help = "In addition to the subcommands above, Cargo subcommands are also \ | ||
supported (see `cargo help` for a list of all Cargo subcommands)."; | ||
|
||
const VERSION: &'static str = if cfg!(feature = "plugins") { | ||
concat_str!(env!("CARGO_PKG_VERSION"), " [feature=plugins]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concat_str!(env!("CARGO_PKG_VERSION"), " [feature=plugins]") | |
concat!(env!("CARGO_PKG_VERSION"), " [feature=plugins]") |
And remove the concat_str
macro above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in the last commit
cargo-afl/build.rs
Outdated
} | ||
|
||
fn build_afl(work_dir: &Path, base: Option<&Path>) { | ||
let mut environment_variables = HashMap::<String, String>::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut environment_variables = HashMap::<String, String>::new(); |
See comment just below this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in the last commit
cargo-afl/build.rs
Outdated
"DESTDIR".to_string(), | ||
common::afl_dir(base).to_str().unwrap().to_string(), | ||
); | ||
environment_variables.insert("PREFIX".to_string(), String::new()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please set these directly on command
(like before).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in the last commit
cargo-afl/src/bin/cargo-afl.rs
Outdated
); | ||
let mut environment_variables = HashMap::<String, String>::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut environment_variables = HashMap::<String, String>::new(); | |
let mut environment_variables = HashMap::<&str, String>::new(); |
I think all of the keys are str
s, so this should work. Note that the insert
s below will need to be adjusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in the last commit
Applied the requested changes in the last commit, could take a look @smoelius ? |
@brunoproduit Could you please resolve the conflicts? If it's easiest, you can squash all of the commits down to one. (I would like to squash before rebasing anyway.) |
@smoelius done, you can go ahead and merge/squash it |
Thank you, @brunoproduit. |
@vanhauser-thc @brunoproduit #421 impacts the feature introduced by this PR. Could I impose on one or both of you try #421, and let me know if there are any suprises? |
I'll try it out this week, I like the idea |
it would be super helpful if this stuff was in the README or somewhere prominent. |
Hey. I'm having the same error here. How did you fix it, do you remember ? |
@kevin-valerio I never was able to resolve this issue. (See #392 (comment).) That is why plugins are not tested on macOS in CI: afl.rs/.github/workflows/rust.yml Lines 38 to 39 in 425b419
I just checked and I still experience the issue. FWIW, I would love to have help with this. |
Are you sure the plugins are compiled with the correct clang from brew? Xcode clang is crippled and the brew llvm version must match the version rust is based on (currently 18) |
Everything seem to match for me unfortunately |
Then likely brew updated llvm to a slightly newer minor revision at a later point so you need to recompile the plugins … cargo afl config —plugins —force |
Another possibility is that brew llvm and macOS rustc llvm are compiled very differently and the plugins are incompatible. You could experiment switching to a different nightly |
This PR activates full cmplog by compiling the afl++ llvm plugins and linking them into the target binaries.
This is activated via an added features "cmplog" as it needs nightly and llvm tools. It is not activated by default.
This fixes #323 .