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

Switch to cargo-fuzz #261

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

uncomputable
Copy link
Collaborator

Adapt the fuzzing infrastructure to use cargo-fuzz. I keep the existing shell scripts because they turned out to be useful.

Everything should work as before, except that the fuzzing flag is producing warnings when the tests are run. Let's discuss solutions here.

@apoelstra
Copy link
Collaborator

Can you update the lockfiles as suggested by CI?

Everything should work as before, except that the fuzzing flag is producing warnings when the tests are run. Let's discuss solutions here.

What kind of warnings?

@uncomputable
Copy link
Collaborator Author

warning: unexpected `cfg` condition name: `fuzzing`
 --> fuzz/fuzz_targets/parse_human.rs:3:13
  |
3 | #![cfg_attr(fuzzing, no_main)]
  |             ^^^^^^^
  |
  = help: expected names are: `clippy`, `debug_assertions`, `doc`, `docsrs`, `doctest`, `feature`, `fmt_debug`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `rustfmt`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `test`, `ub_checks`, `unix`, and `windows`
  = help: consider using a Cargo feature instead
  = help: or consider adding in `Cargo.toml` the `check-cfg` lint config for the lint:
           [lints.rust]
           unexpected_cfgs = { level = "warn", check-cfg = ['cfg(fuzzing)'] }
  = help: or consider adding `println!("cargo::rustc-check-cfg=cfg(fuzzing)");` to the top of the `build.rs`
  = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration
  = note: `#[warn(unexpected_cfgs)]` on by defaul

@uncomputable
Copy link
Collaborator Author

I fixed the normal CI, but the fuzz test CI seems to be disabled. Investigating.

@uncomputable uncomputable force-pushed the 2025-01-cargo-fuzz branch 3 times, most recently from 32591e2 to 3625437 Compare January 23, 2025 14:16
The file is not executed yet.

I upgraded upload-artifact and download-artifact from v3 to v4 because
v3 has been deprecated and automatically fails.

I upgraded cache from v3 to v4 because v3 is scheduled for deprecation
on 2025-02-01.
@uncomputable
Copy link
Collaborator Author

Fuzzing CI works now :) I had to update the upload-artifact and download-artifact actions because of deprecation. I also updated the cache action because it will be deprecated soon.

@apoelstra
Copy link
Collaborator

Gonna try to run this in my local CI. It may fail because the first two commits need to be squashed (OTOH it might not because I might not be checking that generate-files.sh is correct here..).

I hope that the 1.84 MSRV-aware resolver works! Will be nice to be able to cronjob that in rust-bitcoin and elsewhere, so that our "recent" lockfile starts staying reasonably up to date.

Would be nice to add some comments saying that set -o errexit and set -o xtrace mean the same thing as set -e and set -x since the latter are familiar to everybody and the former I've never heard of. But that's just a nit.

utACK 4837317

@apoelstra
Copy link
Collaborator

To deal with the cfg(fuzzing) error add a line to Cargo.toml like

[lints.rust]
unexpected_cfgs = { level = "deny", check-cfg = ['cfg(fuzzing)'] }

`bash fuzz/generate.sh`

`cargo update && cp Cargo.lock Cargo-recent.lock`
using the MSRV-aware resolver from Rust 1.84.0:
https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html

Rewrite each fuzz target to work with cargo-fuzz. Crash seeds are
displayed in base 64, so use that inside duplicate_crash() tests.

cargo-fuzz has special entry points for its fuzz tests, which is why
every fuzz target is prefixed with #![no_main]. However, this breaks
our unit tests that live inside the same file, presumably because unit
tests need a normal entry point. My solution is conditionally include
the fuzzing code when the `fuzzing` flag is set. Meanwhile, clippy needs
a main function in each target to work, so I conditionally add that too.

The `fuzzing` flag is nonstandard and produces a warning. It needs to be
allowed in Cargo.toml
Use cargo-fuzz instead of hongfuzz.
Make the scripts more readable.
We switched to cargo-fuzz and no longer need this.
@uncomputable
Copy link
Collaborator Author

I fixed all warnings and lints. We can run clippy over the entire workspace without issues: cargo clippy --all-targets --workspace -- --deny warnings.

@apoelstra
Copy link
Collaborator

It'll be tough for me to compile this. It puts libfuzzer-sys 0.4.8 into the lockfiles but this doesn't build with any stable Rust compiler, and my local CI wants to be able to build everything in the lockfile.

@apoelstra
Copy link
Collaborator

Oh, maybe it's just a MSRV thing, not a "needs nightly" thing, and maaybe it applies only to the fuzz workspace. Will need to experiment more.

@uncomputable
Copy link
Collaborator Author

For what it's worth, I pinned version 2024-07-01 of the nightly compiler. You can use that version in your local CI.

@apoelstra
Copy link
Collaborator

It's not worth a lot -- having things in the lockfile that stable Rust can't build is a problem.

But having said that, somehow cargo build works in the fuzz/ directory even though the fuzz crate has a direct depenedency on libfuzzer-sys. So I think this may be some quirk of crate2nix. But I need to spend some time digging into it to move forward.

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