GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Replace the Suboptimal `fuzz_tree.py` Harness With a Better Alternative by DaveLak · Pull Request #1910 · gitpython-developers/GitPython

Conversation

@DaveLak

@DaveLak DaveLak commented Apr 29, 2024

Copy link
Copy Markdown
Contributor

As discussed in the initial fuzzing integration PR1, fuzz_tree.py's implementation was not ideal in terms of coverage and its reading/writing to hard-coded paths inside /tmp was problematic as (among other concerns), it causes intermittent crashes on ClusterFuzz2 when multiple workers execute the test at the same time on the same machine.

The changes here replace fuzz_tree.py completely with a completely new fuzz_repo.py fuzz target which:

  • Uses tempfile.TemporaryDirectory() to safely manage tmpdir creation and tear down, including during multi-worker execution runs.
  • Retains the same feature coverage as fuzz_tree.py, but it also adds considerably more from much smaller data inputs and with less memory consumed (and it doesn't even have a seed corpus or target specific dictionary yet.)
  • Can likely be improved further in the future by exercising additional features of Repo to the harness.

Here are some very rough stats to give a very rough idea of the difference:

Metric fuzz_repo.py (my local testing) fuzz_tree.py (most recent successful CF run)
Coverage 789 163
Features 1023 269
Corpus 19 entries / 57 bytes 30 entries / 1872 bytes

Note on License

Because fuzz_tree.py was removed and fuzz_repo.py was not derived from it, the Apache License call outs in the docs were also updated as they only apply to the singe fuzz_config.py file now.

Footnotes

  1. https://github.com/gitpython-developers/GitPython/pull/1901#discussion_r1565001609 ↩

  2. https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68355 ↩

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@DaveLak
Replace the suboptimal fuzz_tree harness with a better alternative
c84e643
As discussed in the initial fuzzing integration PR[^1], `fuzz_tree.py`'s
implementation was not ideal in terms of coverage and its reading/writing to
hard-coded paths inside `/tmp` was problematic as (among other concerns), it
causes intermittent crashes on ClusterFuzz[^2] when multiple workers execute
the test at the same time on the same machine.

The changes here replace `fuzz_tree.py` completely with a completely new
`fuzz_repo.py` fuzz target which:

- Uses `tempfile.TemporaryDirectory()` to safely manage tmpdir creation and
  tear down, including during multi-worker execution runs.
- Retains the same feature coverage as `fuzz_tree.py`, but it also adds
  considerably more from much smaller data inputs and with less memory consumed
  (and it doesn't even have a seed corpus or target specific dictionary yet.)
- Can likely be improved further in the future by exercising additional features
  of `Repo` to the harness.

Because `fuzz_tree.py` was removed and `fuzz_repo.py` was not derived from it,
the Apache License call outs in the docs were also updated as they only apply to
the singe `fuzz_config.py` file now.

[^1]: gitpython-developers#1901 (comment)
[^2]: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68355
@DaveLak

DaveLak commented Apr 29, 2024

Copy link
Copy Markdown
Contributor Author

CC @EliahKagan

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DaveLak

DaveLak commented Apr 29, 2024

Copy link
Copy Markdown
Contributor Author

and FWIW, if removing fuzz_tree is not desired for any reason, I did test out tempfile.TemporaryDirectory() there and it works well enough to address the primary concerns raised. My fork has the commit with it: DaveLak@3a04959

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

EliahKagan
EliahKagan approved these changes Apr 29, 2024
View reviewed changes
Byron
Byron approved these changes Apr 29, 2024
View reviewed changes

@Byron Byron left a comment

Copy link
Copy Markdown
Member

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.

Thanks a million!

More efficient fuzzing is great!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Byron
Byron merged commit 5f26779 into gitpython-developers:main Apr 29, 2024
@DaveLak
DaveLak deleted the replace-flaky-fuzz-tree-test branch May 15, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Byron Byron Byron approved these changes
@EliahKagan EliahKagan EliahKagan approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@DaveLak @Byron @EliahKagan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.