-
Notifications
You must be signed in to change notification settings - Fork 85
test(fuzz): incorporate fuzzing crates into a single workspace, and improve fuzzing docs #437
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
base: master
Are you sure you want to change the base?
Conversation
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule:
FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
Summary of ChangesHello @cosmicexplorer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the project's fuzzing infrastructure and documentation. It consolidates the main Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Summary
This PR successfully consolidates the fuzzing infrastructure into a unified workspace and significantly improves the fuzzing documentation. The changes are well-structured and follow most project guidelines.
Key Improvements
- Workspace Consolidation: Properly unifies the main crate and fuzzing crates into a single workspace, eliminating duplicate dependency declarations
- Feature Flag Implementation: Correctly introduces the
arbitrary
feature flag to gate fuzzing-related code, following project guidelines - Documentation Enhancement: Substantially improves fuzzing instructions with clear, actionable commands and stdio testing capabilities
- Code Organization: Good refactoring of fuzzing code to support both AFL fuzzing and stdin input for debugging
Critical Issue
There's a compilation error in both fuzzing crates where use afl::fuzz;
is conditionally compiled with #[cfg(fuzzing)]
but the fuzz!
macro is still referenced in the conditional blocks. This needs to be fixed before merging.
Minor Note
The PR title should follow Conventional Commits format (e.g., chore: incorporate fuzzing crates into single workspace and improve docs
) as required by project guidelines.
Overall, this is a valuable contribution that will make fuzzing more accessible and maintainable. Once the compilation issue is resolved, this should be ready to merge.
# Any change to rust-version must be reflected also in `README.md` and `.github/workflows/ci.yaml`. | ||
# The MSRV policy is documented in `README.md`. | ||
rust-version = "1.83.0" | ||
categories = ["compression", "filesystem", "parser-implementations"] |
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.
Good addition of categories for better discoverability on crates.io. The selected categories "compression", "filesystem", and "parser-implementations" are appropriate for a ZIP library.
Cargo.toml
Outdated
[workspace] | ||
members = [".", "fuzz_read", "fuzz_write"] | ||
default-members = ["."] | ||
resolver = "2" |
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 workspace configuration looks well-structured. Using default-members = ["."]
ensures that cargo build
in the root only builds the main crate by default, which is the expected behavior for users.
clap = { version = "=4.4.18", features = ["derive"] } | ||
tempfile = "3.15" | ||
|
||
[features] |
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 new arbitrary
feature flag follows the project guidelines for feature-gating new functionality. This properly gates the Arbitrary
trait implementations behind a feature flag as required.
/// [`crate::write::FileOptions::compression_method`] | ||
#[derive(Copy, Clone, PartialEq, Eq, Debug)] | ||
#[cfg_attr(fuzzing, derive(arbitrary::Arbitrary))] | ||
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] |
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.
Good change to use the feature flag instead of the cfg(fuzzing)
attribute. This properly gates the Arbitrary
derive behind the arbitrary
feature as intended by the workspace changes.
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.
Code Review
This pull request effectively unifies the fuzzing crates into a single workspace, which simplifies dependency management and the overall build process. The improvements to the fuzzing documentation, including instructions for using stdin, are a valuable addition for developers. The code changes are generally well-structured and align with the PR's goals. I've identified a few areas for improvement, including a potential panic in a drop
implementation, an opportunity to simplify a function signature for better readability, and a minor formatting issue in the README. Overall, this is a solid contribution that enhances the project's testing infrastructure.
abc2d00
to
043efbb
Compare
36d6af6
to
8cd6810
Compare
Want to note that the changes to And as an aside, I understand changes to CI will require more careful review, so I'm not at all pushing for this PR to be merged soon. This PR is mostly about documenting how to fuzz (after fixing the documentation we link to), and the workspace separation is I think within scope. I don't believe this workspace separation reduces any unwanted dependencies or anything--the current code did that already. The |
9841c29
to
2cd3089
Compare
- add publish = false to fuzz subcrates - move fuzzing to a subdirectory in order to share a workspace - break out separate shards for specific named features, but only for the top-level workspace
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.
Looks good; just one question.
tempfile = "3.15" | ||
|
||
[features] | ||
arbitrary = ["dep:arbitrary"] |
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.
Naming this feature wiithout an underscore prefix will make it part of the public API, meaning e.g. that we can't delete it without bumping the major version. Are you sure that's wise? It seems to me that even if someone needs the implementation for the purpose of fuzzing a downstream crate, then they should either copy it or accept the risk of an incompatible change.
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 previous review comment.
Problem
categories
for discoverability, but then started wondering why we had vestigial[workspace]
declarations in our top-levelCargo.toml
and in the fuzz subcrates.Our current fuzzing instructions do not initiate an afl fuzzing analysis, nor can they be used for one-off testing over stdin.
We need the fuzz crates to be in a distinct workspace from the top-level
zip
crate, because thecargo afl
dependency requires the Cargo 2024 edition functionality. However, we don't need each fuzz crate to be in its own workspace.As we already know #235 will require creating new workspaces for the CLI tools, it would make sense to use this opportunity to do some prep work to support multiple workspaces in CI separately from the CLI workflow.
Solution
categories
strings to ourCargo.toml
for greater discoverability on crates.io.fuzz_read
andfuzz_write
, so the fuzzing logic can be triggered for specific inputs, and not just from AFL itself.#
-style headers in order to enable subsection nesting.cfg(feature = "arbitrary")
to triggerArbitrary
implementations and to disable deprecation warnings, as that usually signals being employed in randomized testing.This next part contributed to the very large number of changed files, and I'm highlighting it here because it was an intentional decision:
fuzz_read
, andfuzz_write
into a single workspace to declare dependencies likeafl
in one place.fuzz/
, which is not a package in itself but just aCargo.toml
.zip-cli
andzip-clite
, which will also require this sort of special handling.Result
Our fuzzing instructions actually work to fuzz! And we are more prepared for adding the multiple CLI workspaces from #235.