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 --release flag #517

Merged
merged 6 commits into from
Feb 13, 2025
Merged

Conversation

YanVictorSN
Copy link
Contributor

  • Add --release flag

Close #20

@smoelius

Copy link
Collaborator

@smoelius smoelius left a comment

Choose a reason for hiding this comment

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

@YanVictorSN Thanks very much for doing this.

Do you have any interest in writing a test for the new flag?

I realize I didn't say anything about that in #20. So "no" is a perfectly acceptable answer.

@YanVictorSN
Copy link
Contributor Author

@YanVictorSN Thanks very much for doing this.

Do you have any interest in writing a test for the new flag?

I realize I didn't say anything about that in #20. So "no" is a perfectly acceptable answer.

Yes, sure! Is it possible for me to do that in another PR? Also, I'm just learning Rust—if you can point me to examples in the code or share any tips, I’d really appreciate it.

@smoelius
Copy link
Collaborator

Yes, sure! Is it possible for me to do that in another PR?

Sure.

Also, I'm just learning Rust—if you can point me to examples in the code or share any tips, I’d really appreciate it.

I'll just record some notes here.

I imagine building a test target whose behavior changes based on whether --release is passed.

The easiest way I can think to do this is:

#[test]
fn test() {
    target();
}

#[test_fuzz::test_fuzz]
fn target() {
    #[cfg(not(debug_assertions))]
    assert(false);
}

This should work because debug_assertions is off when building in release mode.

The above target code should go in a file in the examples/tests directory.

Then there should be a test in the cargo-test-fuzz/tests/integration directory that exercises the target code as follows:

  • test it with cargo test
  • run cargo test-fuzz on it in two ways:
    • without --release and verify success (i.e., no crash)
    • with --releaseand verify failure (i.e., crash)

The "run cargo test-fuzz" step should use this command:

let mut command = examples::test_fuzz(krate, "target").unwrap();

krate will be the name of the file containing the target code.

Finally, the "run cargo test-fuzz" step should specify a timeout so that if the target code doesn't crash, the test doesn't run forever.

I think 80% of this could be achieved by copying one of the fuzz_* tests in the cargo-test-fuzz/tests/integration directory. But I am happy to answer questions if anything is unclear.

@YanVictorSN
Copy link
Contributor Author

@smoelius Thank you so much. I’ll do that. And if I have any questions, I will definitely reach out to you.

@smoelius
Copy link
Collaborator

I've done some experiments locally, and I think this is working as expected. I'm going to hold off on releasing this until we have a test in CI, though.

Thanks again!

@smoelius smoelius added this pull request to the merge queue Feb 13, 2025
Merged via the queue into trailofbits:master with commit 4ab277f Feb 13, 2025
20 checks passed
@YanVictorSN
Copy link
Contributor Author

Thank you! @smoelius I'm trying to create the test, but I'm having some trouble. Should I open a draft or can we chat here?

@YanVictorSN YanVictorSN deleted the feature-release-flag branch February 13, 2025 12:59
@smoelius
Copy link
Collaborator

Either is fine. Whatever is easiest for you.

@YanVictorSN
Copy link
Contributor Author

@smoelius Do I need to pass any value in the target? I know I need to test the release tag, but I need to initialize the corpus directory like in the fuzz file to check if it will crash, right?

@smoelius
Copy link
Collaborator

Direct answer to your question: yes, I was making a mistake, because AFL++ doesn't like and rejects empty corpus files, and "no arguments" would serialize to an empty corpus file.

But I just realized that my debug_assertions idea won't work, because cargo-afl spefically enables debug_assertions, regardless of whether building in release mode: https://github.com/rust-fuzz/afl.rs/blob/e586a66aadc36977501257ee8b8201d61a452021/cargo-afl/src/main.rs#L285

Here are the things that are enabled in release mode: https://doc.rust-lang.org/cargo/reference/profiles.html#release

Hmm... What else in there could be used to produce a crash in release mode? (It's not immediately obvious to me.)

@smoelius
Copy link
Collaborator

Maybe there's a way to turn the absence of overflow checks into a crash?

@smoelius
Copy link
Collaborator

Something along these lines maybe? https://stackoverflow.com/a/73224634

Is this something you would like to play with?

I realize I've imposed upon you by asking you to write this test. If this has become too much of a hassle, I completely understand.

@YanVictorSN
Copy link
Contributor Author

Yeah, sure. Don’t worry, I’m learning a lot. You make things easier for a beginner like me. So, the idea here is to try to force an overflow to check if the release tag is working or not, right?

@smoelius
Copy link
Collaborator

Overflow checks will be enabled in debug mode, but not in release mode.

On the face of it, that means one could get a panic/crash in debug mode, but not in release mode. But we want the other way around.

So, I'm wondering, could we catch the panic with std::panic::catch_unwind, and then do the opposite, i.e.:

  • if a panic is caught, silently return.
  • if no panic is caught, panic.

Does that make sense?

@smoelius
Copy link
Collaborator

@smoelius
Copy link
Collaborator

Ok, this has become more complicated than I expected.

It might be possible to use the PROFILE environment variable: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts

But the documentation suggests that variable is set only for build scripts. In other words, writing a test that relies on that variable would require writing a build script. That could be done, but I can't quite see how all of the pieces would fit together.

I'm going to have to thing about this some more, unless you can see a solution.

@YanVictorSN
Copy link
Contributor Author

Not yet. I’ll think more about it and, in the meantime, look for another good_first_issue in the repo. 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.

Add --release flag
2 participants