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 — Add Submodules Fuzz Target by DaveLak · Pull Request #1919 · gitpython-developers/GitPython

Conversation

@DaveLak

@DaveLak DaveLak commented May 30, 2024

Copy link
Copy Markdown
Contributor

Fuzz Introspector heuristics suggest the Submodule API code represent "optimal analysis targets" that should yield a meaningful increase in code coverage. The changes here introduce a first pass at implementing a fuzz harness that cover the primary APIs/methods related to Submodules. Of particular interest to me is the Submodule.config_writer() coverage.

Please note however, there is likely plenty of room for improvement in this harness in terms of both code coverage as well as performance; the latter of which will see significant benefit from a well curated seed corpus of .gitmodules file like inputs. The ParsingError raised by the fuzzer without a good seed corpus hinders test efficacy significantly.

I have a draft PR up with a seed corpus here: gitpython-developers/qa-assets#5

Sorry, something went wrong.

Uh oh!

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

All reactions
@DaveLak
Add Submodules Fuzz Target
6d52bdb
Fuzz Introspector heuristics suggest the Submodule API code represent
"optimal analysis targets" that should yield a meaningful increase in
code coverage. The changes here introduce a first pass at implementing a
fuzz harness that cover the primary APIs/methods related to Submodules.
Of particular interest to me is the `Submodule.config_writer()`
coverage.

Please note however, there is likely plenty of room for improvement in
this harness in terms of both code coverage as well as performance; the
latter of which will see significant benefit from a well curated seed
corpus of `.gitmodules` file like inputs. The `ParsingError` raised by
the fuzzer without a good seed corpus hinders test efficacy
significantly.
@DaveLak
DaveLak force-pushed the improve-fuzzer-coverage branch from fa52ea2 to 6d52bdb Compare May 30, 2024 05:08
@DaveLak DaveLak mentioned this pull request May 30, 2024
Add initial corpus and dictionary for the fuzz_submodule target gitpython-developers/qa-assets#5
Merged
Byron
Byron approved these changes May 30, 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 lot for taking such great care!

Part of me thinks that the submodule implementation is so riddled with inaccuracies and and incorrectness that fuzzing it seems like a waste. The fuzzer can only try to find unexpected exceptions, and maybe that's a small win, but at what cost?

Part of that feeling also stems for the incredible sluggishness of Python in general, so any fuzzing feels wasteful. But that's besides the point I suppose, apologies for the ramblings.

Sorry, something went wrong.

Uh oh!

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

All reactions
@Byron
Byron merged commit 2345c1a into gitpython-developers:main May 30, 2024
@DaveLak
DaveLak deleted the improve-fuzzer-coverage branch May 30, 2024 14:33
@DaveLak

DaveLak commented May 30, 2024

Copy link
Copy Markdown
Contributor Author

Please don't apologize, and definitely do not hesitate to reject or push back on any of my PRs! (especially considering that my last few PRs came out of the blue without prior discussion about whether they're even wanted -- sorry about that 😅)

Part of me thinks that the submodule implementation is so riddled with inaccuracies and and incorrectness that fuzzing it seems like a waste. The fuzzer can only try to find unexpected exceptions, and maybe that's a small win, but at what cost?
Part of that feeling also stems for the incredible sluggishness of Python in general, so any fuzzing feels wasteful.

I think your points are perfectly reasonable. Here is how I've been thinking of the value in fuzzing GitPython:

  • Hopelessly broken or not, GitPython is widely used, so I tend to believe (perhaps too optimistically) that even small wins which result in improved stability can have an outsized impact for some n of users over time. Doing the wrong thing right is better than doing it wrong + unexpected behavior someone will eventually need to debug. That said, I fully respect and understand if you don't think the value justifies the CPU cycles, maintenance burden, or any other reason for that matter.

  • Similar to the above, I believe continuous fuzzing is well-positioned to identify regressions that traditional unit tests may miss. Even without a CI action run on PRs, the accumulated corpus in ClusterFuzz should be able to identify unexpected exceptions reasonably quickly after they're introduced. But, of course, that is just my hypothesis, which has yet to be validated.

  • Finally, perhaps from a somewhat naive perspective, I believe that incorporating well-documented and effective fuzzing into GitPython has the potential to benefit the wider Python community. Now, I fully recognize how presumptuous that sounds but here me out. Historically, fuzzing has been used by security experts on lower-level languages with great success, but a lack of easy to use tooling, terse documentation, and few quality examples to emulate has hindered wider adoption in higher-level languages like Python. So I think integrating fuzz testing into a widely-used and complex Python project like GitPython, paired with some quality documentation, can lower the barrier for entry for folks that may be seeking to learn more or just happen to stumble across it (like I did lol.)

But that's besides the point I suppose, apologies for the ramblings.

I think everything you said is very much on-point regarding any of the fuzzing work in this repo. Moreover, I really appreciate hearing your thoughts, so thanks!

In case it isn't clear, I won't be offended if you feel the juice isn't worth the squeeze, and would rather me hold off on any non-maintenance type fuzzing work. Frankly, if you decided you'd rather it all removed ASAP, I'd help remove it. I've learned a lot about Git, Python, fuzzing, and more working on these, so I wouldn't consider it a wasted effort even if the changes never made it to PR, So thanks, @Byron, for the support along the way! 🙂


And now, it's my turn to apologize for the ramblings 😅

All reactions

Sorry, something went wrong.

Uh oh!

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

@Byron

Byron commented May 31, 2024

Copy link
Copy Markdown
Member

(especially considering that my last few PRs came out of the blue without prior discussion about whether they're even wanted -- sorry about that 😅)

That's perfectly alright - no need to make it more complicated, just do what you think is right, you are driving this.

  • That said, I fully respect and understand if you don't think the value justifies the CPU cycles, maintenance burden, or any other reason for that matter.

I am also clearly biased and think that everybody should use gitoxide, so it helps to see your reasoning to unbias me a bit. After all, GitPython has it's value and given its usage, maybe it's probably a good idea to invest in any measure that can make it a little better, maybe particularly due to its many flaws.

  • Finally, perhaps from a somewhat naive perspective, I believe that incorporating well-documented and effective fuzzing into GitPython has the potential to benefit the wider Python community.

Spreading fuzzing as a technique through GitPython is a great thought and I am fully behind that - if nothing else comes out of it, more Python projects might adopt it which could be a net-win. And even if not, people learn how to use a fuzzer which will help in any programming environment eventually.

In case it isn't clear, I won't be offended if you feel the juice isn't worth the squeeze, and would rather me hold off on any non-maintenance type fuzzing work. Frankly, if you decided you'd rather it all removed ASAP, I'd help remove it. I've learned a lot about Git, Python, fuzzing, and more working on these, so I wouldn't consider it a wasted effort even if the changes never made it to PR, So thanks, @Byron, for the support along the way! 🙂

I'd never do that, and don't feel that way at all. But I do admit that I'd love to see you eventually move to gitoxide - you do tremendous work here and even if it's just for spreading good fuzzing setups, gitoxide could certainly be a good vessel for that.

Eventually.

No pressure :D.

PS: There I'd definitely have more opinions on what to fuzz as well, which might make it more interesting for you.

DaveLak reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

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

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.

2 participants

@DaveLak @Byron
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.