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

Make RUST_LIBFUZZER_DEBUG_PATH "free" for non-users #81

Merged
merged 2 commits into from
May 26, 2021

Conversation

elichai
Copy link
Contributor

@elichai elichai commented May 20, 2021

Currently if someone doesn't use RUST_LIBFUZZER_DEBUG_PATH they still need to pay an allocation (to convert to CString) a Mutex lock, and a getenv(3) call for every fuzzing input.

Instead we can check and load the environment variable at initialization and from then it will be almost free for users that don't set the environment variable.

(this is slightly a behavior change as before someone could attach a debugger and modify the enviroment variable while it's fuzzing but now they can't, but I don't think it matters)

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR!

I like pulling this out to the init function. I didn't realize all the layers surrounding env vars, and assumed they would just be a cheap getenv that doesn't require any system calls.

However, I'd like to avoid all this unsafe. I'm not arguing that it is incorrect, its just a lot of open-coded unsafe stuff that adds that much overhead to all future maintenance. Instead let's either

  • use lazy_static
  • make RUST_LIBFUZZER_DEBUG_PATH an AtomicBool that is set during init if the env var is set, and then checked during the main function where, if it is set, we can afford to pay for the std::env::var call again.

@elichai
Copy link
Contributor Author

elichai commented May 23, 2021

However, I'd like to avoid all this unsafe. I'm not arguing that it is incorrect, its just a lot of open-coded unsafe stuff that adds that much overhead to all future maintenance. Instead let's either

* use `lazy_static`

* make `RUST_LIBFUZZER_DEBUG_PATH` an `AtomicBool` that is set during init if the env var is set, and then checked during the main function where, if it is set, we can afford to pay for the `std::env::var` call again.

Yeah I also really wanted to avoid unsafe, but couldn't find a good way to do it (too bad there's no AtomicStaticRef<T> type that can give us the same as AtomicPtr but without using unsafe)

I don't mind using lazy_static or once_cell(the latter will allow us to move to libstd provided variant in the future more easily - See rust-lang/rust#74465) but I wanted to avoid introducing another dependency.

the 2nd option sounds interesting, it's still a little more expensive, but only for people who use this feature and they then do actual I/O so it's probably negligible compared to that.

So for me either is fine, which one do you prefer?

@fitzgen
Copy link
Member

fitzgen commented May 24, 2021

Good point about OnceCell eventually moving into std. Let's go with that: initializing the OnceCell inside initialize and then doing

if let Some(path) = the_once_cell.get() {
    // ...
}

in the main function.

@elichai elichai force-pushed the no-env-check branch 2 times, most recently from 59059d0 to f1e699b Compare May 25, 2021 09:11
@elichai
Copy link
Contributor Author

elichai commented May 25, 2021

Good point about OnceCell eventually moving into std. Let's go with that: initializing the OnceCell inside initialize and then doing

if let Some(path) = the_once_cell.get() {
    // ...
}

in the main function.

Done

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great! If you could merge in my suggestions below into your PR -- apologies, I'm nitpicky about punctuation/capitalization in code comments -- then I'll merge the PR. Thanks again!

src/lib.rs Outdated
@@ -52,6 +56,13 @@ pub fn initialize(_argc: *const isize, _argv: *const *const *const u8) -> isize
default_hook(panic_info);
::std::process::abort();
}));

// initialize the RUST_LIBFUZZER_DEBUG_PATH cell with the path so it can be reused with little overhead
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// initialize the RUST_LIBFUZZER_DEBUG_PATH cell with the path so it can be reused with little overhead
// Initialize the `RUST_LIBFUZZER_DEBUG_PATH` cell with the path so it can be
// reused with little overhead.

src/lib.rs Outdated
@@ -130,7 +141,9 @@ macro_rules! fuzz_target {
// When `RUST_LIBFUZZER_DEBUG_PATH` is set, write the debug
// formatting of the input to that file. This is only intended for
// `cargo fuzz`'s use!
if let Ok(path) = std::env::var("RUST_LIBFUZZER_DEBUG_PATH") {

// RUST_LIBFUZZER_DEBUG_PATH is set in initialization
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// RUST_LIBFUZZER_DEBUG_PATH is set in initialization
// `RUST_LIBFUZZER_DEBUG_PATH` is set in initialization.

src/lib.rs Outdated
@@ -169,7 +182,9 @@ macro_rules! fuzz_target {
// When `RUST_LIBFUZZER_DEBUG_PATH` is set, write the debug
// formatting of the input to that file. This is only intended for
// `cargo fuzz`'s use!
if let Ok(path) = std::env::var("RUST_LIBFUZZER_DEBUG_PATH") {

// RUST_LIBFUZZER_DEBUG_PATH is set in initialization
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// RUST_LIBFUZZER_DEBUG_PATH is set in initialization
// `RUST_LIBFUZZER_DEBUG_PATH` is set in initialization.

@elichai
Copy link
Contributor Author

elichai commented May 26, 2021

@fitzgen I fixed the comments :)

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!!

@fitzgen fitzgen merged commit 0417c4f into rust-fuzz:master May 26, 2021
@elichai elichai deleted the no-env-check branch May 26, 2021 21:55
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