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

Improved trdelnik-tests folder structure for PoC and Fuzz Tests #109

Merged
merged 22 commits into from
Jan 29, 2024

Conversation

lukacan
Copy link
Collaborator

@lukacan lukacan commented Dec 28, 2023

  • Purpose of this PR is to improve structure of trdelnik-tests folder

  • Currently the structure is as follows:

  • trdelnik-tests

    • Cargo.toml
    • src/bin
      • fuzz_target.rs
    • tests
      • test.rs
  • Incoming structure

  • trdelnik-tests

    • poc_tests
      • Cargo.toml
      • tests
        • test.rs
    • fuzz_tests
      • fuzz_0
        • test_fuzz.rs
        • Cargo.toml
      • fuzz_1
      • fuzz_2
      • fuzzing
        • hfuzz_workspace
        • hfuzz_target
  • commands can be executed:

    • trdelnik fuzz run fuzz_0
    • trdelnik fuzz run fuzz_1
    • etc.
    • trdelnik test
      • will call 'cargo test --package poc_tests'
  • Each fuzz test has its own directory that has to be added into project root Cargo.toml as a new member. This is done automatically during trdelnik fuzz add command. Further this command does not expect any additional arguments, that means no custom fuzz test names.

  • Both types of the tests (Fuzz/PoC) now has their own Cargo.toml template.

  • Examples within the examples folder were updated to the incoming trdelnik-test folder structure

  • Note: based on the incoming changes, following points were accordingly updated

    • fuzz run expects target dir from CARGO_TARGET_DIR env variable and workspace dir from HFUZZ_WORKSPACE env variable, if these are not explicitly set, default directory is set to fuzzing/hfuzz_target and fuzzing/hfuzz_workspace.
    • fuzz with exit code has set default workspace directory to the fuzzing/hfuzz_workspace.
    • fuzz debug has explicitly set target dir either from CARGO_TARGET_DIR env variable or default to the fuzzing/hfuzz_target folder
    • clean command is accordingly set to look for default location = fuzzing/hfuzz_target folder
    • Fuzzing feature is hardcoded into the manifest template for fuzz tests

@lukacan lukacan requested a review from Ikrk December 28, 2023 15:56
Copy link
Contributor

@Ikrk Ikrk left a comment

Choose a reason for hiding this comment

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

Do I understand it correctly that your proposed structure will have separate Cargo.toml files for each fuzz test? I think this is quite sub-optimal as it effectively introduces new rust project for each and every fuzz test and it will blow the target folder size and compilation times as the compilation artifacts cannot be shared between tests. I think the two options that we spoke about (and are noted in Jira) would be a better way to go...

@lukacan
Copy link
Collaborator Author

lukacan commented Jan 3, 2024

I went through some research and testing.
Based on Workspace All packages share a common Cargo.lock and I consider that it is the case also for Virtual Workspace. Next all packages share a common output directory (target) in this case hfuzz_target. Based on this and based on the testing, all fuzz tests share same build\deps folders within the hfuzz_target. Even though I used exactly the same project for both structures the one with "package for every fuzz test" has slightly smaller build/deps folders. Next thing I noticed is that for Incremental folder, also the option "package for every fuzz test" has less snapshots within the incremental folder. Based on the previous two comparisons total size for hfuzz_target is slightly higher ~200MB for option where fuzz tests are one big package. This was tested on multiple runs of multiple fuzz_targets, mixed with adding/removing new fuzz_tests + adding/removing dependencies.

Next thing I noticed (which is probably not likely to happen often), but let`s say if we need to modify Cargo files. for example if we add new program inside programs folder. We need to add something like:

[dependencies.fuzzer_copy_copy]
path = "../../programs/fuzzer_copy_copy"

Into the Cargo files. If we use one Package for all fuzz tests that would mean every fuzz test would need to be re-built, on the other hand if we use "package for every fuzz test" only the fuzz tests where we include the program dep for the new program would need to be re-built. And same goes for some other Cargo manifest modifications. However the one that is most likely to happen is dependency modifications (let`s say adding anchor-spl) as mentioned above, all packages in the workspace share common Cargo.lock and new dep will modify this Cargo that means all fuzz tests would need to be re-built for both options.

Finally, I will refactor PR to use one Cargo = fuzz tests as one package, which I eventually find simpler to use and read.

@lukacan
Copy link
Collaborator Author

lukacan commented Jan 9, 2024

Comment on the latest commits

  • Structure was updated as mentioned within my last comment so we will for now use one Cargo.toml with multiple bin files
  • Correspondingly, the fuzzer example was updated for the new structure
  • --with_exit_code has to check for crash folder contents
    • for this purpose, I just moved the folder contents checks to the commander
      • to have it on one place
    • in short, we load parameters from env variables HFUZZ_WORKSPACE/HFUZZ_RUN_ARGS
    • based on values (--crashdir > --workspace > HFUZZ_WORKSPACE ) we obtain desired crash directory
    • removed current_dir check and cargo current_dir arguments as I think we do not need them
      • If the folder does not exist but is a member within Virtual Manifest, it will not even build
      • if it is not even a Workspace member, honggfuzz will throw that the target does not exists
    • added some tests for the test_get_crash_dir_and_ext

Copy link
Contributor

@Ikrk Ikrk left a comment

Choose a reason for hiding this comment

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

Good job, only minor question about the gitignore file and we can merge it afterwards. I created issues in Jira regarding the other two comments.

crates/client/src/test_generator.rs Show resolved Hide resolved
crates/cli/src/command/fuzz.rs Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@Ikrk Ikrk merged commit 3b52ac8 into develop Jan 29, 2024
7 checks passed
@lukacan lukacan deleted the feat/fuzz-directory branch February 1, 2024 12:56
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