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 — Fuzzer Migration Follow-ups by DaveLak · Pull Request #1903 · gitpython-developers/GitPython

Conversation

@DaveLak

@DaveLak DaveLak commented Apr 16, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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

  • Makes structural improvements to how the local instructions for running OSS-Fuzz are presented after removing undefined as a valid option in google/oss-fuzz@b210fb2 (#11803) (Context: Comment in OSS-Fuzz PR #11803)

TODO / Pending Further Discussion

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

  • Add GitPython's standard license header comments to non-Apache 2.0 licensed files (Comment) Done in Add GitPython's Standard License Header Comments to Shell Scripts #1907
  • fuzzing/fuzz-targets/fuzz_tree.py refactoring (Comment 1 & Comment 2) Workaround for the primary concern introduced in Dockerize "Direct Execution of Fuzz Targets" #1904
  • Figuring out a better way to deal with OSS-Fuzz provided environment variables ($SRC, $OUT, & $WORK) in fuzzing/oss-fuzz-scripts (Comment)

Sorry, something went wrong.

Uh oh!

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

All reactions
DaveLak added 7 commits April 16, 2024 14:41
@DaveLak
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
@DaveLak
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
@DaveLak
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
@DaveLak
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
@DaveLak
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
@DaveLak
Remove unnecessary semicolon for consistent script formatting
e038526
Based @EliahKagan's suggestion and feedback on:
gitpython-developers#1901
@DaveLak
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 DaveLak mentioned this pull request Apr 16, 2024
Initial Migration of Fuzz Tests & Integration Scripts From the OSS-Fuzz Project Repo #1901
Merged
@DaveLak
Merge branch 'gitpython-developers:main' into fuzzing-integration-fol…
a30f9b9
…low-ups
@DaveLak
DaveLak marked this pull request as ready for review April 17, 2024 16:06
EliahKagan
EliahKagan approved these changes Apr 17, 2024
View reviewed changes

@EliahKagan EliahKagan 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.

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).

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread fuzzing/oss-fuzz-scripts/container-environment-bootstrap.sh Resolved

Uh oh!

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

@DaveLak

DaveLak commented Apr 17, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

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.)

EliahKagan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

DaveLak added 2 commits April 17, 2024 20:15
@DaveLak
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)
@DaveLak
Remove unintentional leading space from comment
1d54d4b
EliahKagan
EliahKagan approved these changes Apr 18, 2024
View reviewed changes
Byron
Byron approved these changes Apr 18, 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 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 :).

Sorry, something went wrong.

Uh oh!

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

DaveLak reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@Byron
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
@DaveLak
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
DaveLak deleted the fuzzing-integration-follow-ups branch April 22, 2024 20:15
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.