← 返回首页
Fuzzer Migration Follow-ups by DaveLak · Pull Request #1903 · gitpython-developers/GitPython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

Fuzzer Migration Follow-ups#1903

Merged
Byron merged 10 commits into
gitpython-developers:mainfrom
DaveLak:fuzzing-integration-follow-ups
Apr 18, 2024
Merged

Fuzzer Migration Follow-ups#1903
Byron merged 10 commits into
gitpython-developers:mainfrom
DaveLak:fuzzing-integration-follow-ups

Conversation

Copy link
Copy Markdown
Contributor

DaveLak commented Apr 16, 2024 •
edited
Loading

This PR addresses most unresolved review comments from #1901. It also updates the README

Addressed in This PR

  • Remove shebangs from fuzz harnesses (Comment 1 & Comment 2)
  • Replace shebang in build.sh with ShellCheck directive (Comment)
  • Set executable bit on fuzzing/oss-fuzz-scripts/container-environment-bootstrap.sh to mark it executable in Git (Comment)
  • Make the link text in fuzzing/README.md for the OSS-Fuzz test status URL more descriptive (Comment)
  • Fix capitalization of GitPython repository name in fuzzing/README.md (Comment)
  • Simplify read delimiter to use empty string in build.sh's build fuzz harness loop (Comment)
  • Remove unnecessary semicolons in fuzzing/oss-fuzz-scripts/container-environment-bootstrap.sh for consistent script formatting (Comment)
  • Fix various misspellings of "corpora" (Comment)

Misc

TODO / Pending Further Discussion

I felt that these items are better addressed in separate PRs.

DaveLak added 7 commits April 16, 2024 14:41
Remove shebangs from fuzz harnesses …
68194a9
Prefer executing these files using the OSS-Fuzz or `python` command methods outlined in the `fuzzing/README`. Based on feedback and discussion on: gitpython-developers#1901
Replace shebang in build.sh with ShellCheck directive …
8954c71
This script is meant to be sourced by the OSS-Fuzz file of the same name, rather than executed directly. The shebang may lead to the incorrect assumption that the script is meant for direct execution. Replacing it with this directive instructs ShellCheck to treat the script as a Bash script, regardless of how it is executed. Based @EliahKagan's suggestion and feedback on: gitpython-developers#1901
Set executable bit on container-environment-bootstrap.sh …
b0a5b8e
This script is executed directly, not sourced as is the case with `build.sh`, so it should have an executable bit set to avoid ambiguity. Based @EliahKagan's suggestion and feedback on: gitpython-developers#1901
Minor clarity improvements in fuzzing/README.md …
25f3600
- Make the link text for the OSS-Fuzz test status URL more descriptive - Fix capitalization of GitPython repository name Based @EliahKagan's suggestion and feedback on: gitpython-developers#1901
Simplify read delimiter to use empty string in fuzz harness loop …
d79c176
Replaces the null character delimiter `-d $'\0'` with the simpler empty string `-d ''` in the fuzzing harness build loop. This changes leverages the Bash `read` builtin behavior to avoid unnecessary complexity and improving script readability. Based @EliahKagan's suggestion and feedback on: gitpython-developers#1901
Remove unnecessary semicolon for consistent script formatting …
e038526
Based @EliahKagan's suggestion and feedback on: gitpython-developers#1901
Fix various misspellings of "corpora" & improve script comments …
d25ae2d
A misspelling in the https://github.com/gitpython-developers/qa-assets repository is still present here. It will need to be fixed in that repository first. "corpora" is a difficult word to spell consistently I guess. This made for a good opportunity to improve the phrasing of two other comments at at least. Based @EliahKagan's suggestion and feedback on: gitpython-developers#1901
DaveLak mentioned this pull request Apr 16, 2024
Merged
DaveLak marked this pull request as ready for review April 17, 2024 16:06
EliahKagan approved these changes Apr 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

EliahKagan left a comment

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

This looks good.

I wonder if corpra should be renamed to corpora in that path as well, or if that would cause problems.

If and only if that is changed, this line would then need to be changed accordingly:

GitPython/fuzzing/oss-fuzz-scripts/container-environment-bootstrap.sh

Line 40 in e3fb1f2

rsync -avc qa-assets/gitpython/corpra/ "$SEED_DATA_DIR/" &&

Unrelated to that, there is a tiny style nit commented on below (which you may even decide to leave as-is).

Comment thread fuzzing/oss-fuzz-scripts/container-environment-bootstrap.sh Outdated Show resolved Hide resolved
Copy link
Copy Markdown
Contributor Author

DaveLak commented Apr 17, 2024 •
edited
Loading

I wonder if corpra should be renamed to corpora in that path as well, or if that would cause problems.

It definitely should but among everything else related to the fuzzing work, updating that repo feels like the lowest priority. Especially considering right now it works exactly as we need it to, I've opted to defer any changes for now.

After the outstanding change requests are addressed, I would like (and plan) to revisit https://github.com/gitpython-developers/qa-assets. I think it would benefit from a structure similar to https://github.com/bitcoin-core/qa-assets. Specifically:

  1. A simple README with context about what's there, why, and how to use/improve it.
  2. Removal of the assets related to the unrelated projects.
  3. Updating it so the contents of each corpus are stored in a directory named for the fuzz harness they're used by instead of opaque zip blobs. That would require updating the container bootstrap script to zip them too.

I'm happy to discuss further if you would like but that feels like it would be best in a discussion thread (or an issue on https://github.com/gitpython-developers/qa-assets, but I'm not sure there is a desire to turn issues on there.)

DaveLak added 2 commits April 17, 2024 20:15
Remove comment suggesting the undefined sanitizer is a valid option …
23a505f
Also makes come structural improvements to how the local instructions for running OSS-Fuzz are presented now that only the single `address` sanitizer is a valid option. The `undefined` sanitizer was removed from GitPython's `project.yaml` OSS-Fuzz configuration file at the request of OSS-Fuzz project reviewers in google/oss-fuzz#11803. The `undefined` sanitizer is only useful in Python projects that use native exstensions (such as C, C++, Rust, ect.), which GitPython does not currently do. This commit updates the `fuzzing/README` reference to that sanitizer accoirdingly. See: - google/oss-fuzz@b210fb2 - google/oss-fuzz#11803 (comment)
Remove unintentional leading space from comment
1d54d4b
Byron approved these changes Apr 18, 2024
View reviewed changes
Copy link
Copy Markdown
Member

Byron left a comment

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Thanks a lot for this follow-up and for sharing your plan of the additional improvements.

Not using zip blobs stored in the qa-assets repository is my favorite :).

Byron merged commit 00bc707 into gitpython-developers:main Apr 18, 2024
DaveLak added a commit to DaveLak/oss-fuzz that referenced this pull request Apr 18, 2024
Simplify Dockerfile RUN command …
94466ee
The upstream GitPython repository merged a change that set the executable bit on `container-environment-bootstrap.sh` in Git, so the `chmod` is no longer needed and the `RUN` instruction can execute the script directly. See: - gitpython-developers/GitPython#1903 - gitpython-developers/GitPython@b0a5b8e
DaveLak deleted the fuzzing-integration-follow-ups branch April 22, 2024 20:15
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Byron Byron approved these changes

EliahKagan EliahKagan approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Footer

© 2026 GitHub, Inc.