Repository navigation
fix(sdk): missed one on the previous pull - #415
Merged
Merged
Conversation
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe CI workflow now passes ChangesMaven Source Generation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The source-generation workflow uses the supported Enforcer skip flag, with no identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the Maven line, Comment |
elizabethhealy
approved these changes
Oct 5, 2026
|
dmihalcik-virtru
approved these changes
Oct 5, 2026
Contributor
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 8, 2026
…d, add zip seeds (#416) Related: [DSPX-4589](https://virtru.atlassian.net/browse/DSPX-4589) (zip reader hardening, #398), [DSPX-5070](https://virtru.atlassian.net/browse/DSPX-5070) (fuzzing in CI; this PR adds the CI job, and the PR replay and alerting are still to do there), [DSPX-5073](https://virtru.atlassian.net/browse/DSPX-5073) (the `fuzzTDF` NPE) Runs the Jazzer fuzz targets after merges to `main` that touch the fuzzed code. Also tightens the `fuzzZipRead` target and gives it seeds for zip structures the existing corpus never reaches. No SDK code changes. ## Fuzzing in CI (`.github/workflows/fuzz.yaml`) - **When:** on pushes to `main` that touch `sdk/src/**`, any `pom.xml`, or the workflow itself, plus `workflow_dispatch`. Merges that can't change the fuzzed code, such as docs, `cmdline`, or examples, don't spend runner time. A `concurrency` group means a burst of merges queues a single run of the latest commit, and a run that's already fuzzing isn't cancelled. **Not on pull requests:** a real fuzz run takes the full 10-minute `maxDuration` per target, which is too slow for the payoff on every change. - **What:** one matrix job per `@FuzzTest` (`fuzzZipRead`, `fuzzTDF`), because Jazzer fuzzes one target per run. `fail-fast: false` keeps one target's result from cancelling the other. - **Corpus:** the corpus Jazzer generates (`sdk/.cifuzz-corpus`) is cached per target. It is saved even when a run fails, so each run continues from the last. - **Findings:** the job fails, and the reproducing `crash-*` input plus the surefire reports are uploaded as `fuzz-findings-<target>`. Fix the bug, then commit the input under `FuzzingInputs/<target>/` so that it replays. - **Build:** same setup as `checks.yaml` (buf auth, `sdk-fips-bc` installed for the default non-fips profile), with the same pinned actions. `upload-artifact` is newly pinned to v4.6.2. I ran the job's Maven commands locally. On a finding, the job fails and Jazzer writes the `crash-*` file where the upload glob looks for it. The corpus lands at the cached path. **`fuzzTDF` will be red until the existing `NullPointerException` ([DSPX-5073](https://virtru.atlassian.net/browse/DSPX-5073), see Caveats) is fixed.** Jazzer replays the checked-in inputs before it starts fuzzing, and one of them already hits it. ## Zip fuzz target - **`fuzzZipRead` no longer swallows `IllegalArgumentException`.** The harness used to catch it along with `InvalidZipException`, so a reader that seeks to a corrupt offset instead of rejecting the archive passed silently. It now expects only: - `InvalidZipException`; - `IOException`; - `JsonParseException`, which comes from parsing the `.json` entries, not from the zip itself. Anything else fails the target. - **Seven seed archives** in `FuzzingInputs/fuzzZipRead/`: - `seed-zip64`: zip64 end of central directory record and locator; - `seed-zip64-entry-count-only`: zip64 record where only the entry count needs it; - `seed-zip64-trailing-data`: zip64 archive followed by trailing bytes; - `seed-zip64-comment-with-decoy-signature`: a stray `PK\5\6` inside the comment; - `seed-plain-utf8-names`: non-ASCII entry names with general purpose bit 11 set; - `seed-empty`: a bare end of central directory record; - `seed-commons-zip64-always`: written by commons-compress with `Zip64Mode.Always`. - **`.cifuzz-corpus/` is ignored.** Jazzer writes its generated corpus there during fuzzing runs. ## Results On `main` (#415) with this change: - **Real fuzzing:** `JAZZER_FUZZ=1 mvn -pl sdk -am test -Dtest='Fuzzing#fuzzZipRead'` ran for 10 minutes, about 426k executions, with no findings. - **Regression replay:** `mvn -pl sdk -am test -Dtest='Fuzzing#fuzzZipRead'` passed 11/11 (the existing inputs plus the new seeds). It also passed 122/122 with the roughly 110 inputs generated by a 10-minute run against #398's reader added. With #398's reader, a 10-minute run (about 130k executions) also found nothing. Main is still exposed to the bugs #398 fixes, but they don't show up as exceptions, so this harness can't see them: - a stalled channel needs a channel that returns empty reads, which the in-memory channel never does; - silently short entries and negative entry counts produce a wrong result without throwing. #398's unit tests cover those. ## Caveats - **PR checks never run `Fuzzing`.** Surefire only picks up classes named like `*Test`, and the pom doesn't add an include. So outside the fuzz workflow, these seeds and the existing `crash-*` inputs replay only when you run `-Dtest=Fuzzing` yourself. Once `fuzzTDF` is fixed, a cheap follow-up would replay the corpus on PRs, which takes about 3 seconds. It would need either renaming the class to `FuzzingTest` or adding a Surefire include. - **The existing `fuzzTDF` corpus fails on `main`.** Running `-Dtest='Fuzzing#fuzzTDF'` on `main` hits a `NullPointerException` at `Manifest.readManifest` (`Manifest.java:673`). `gson.fromJson` returns `null` for a manifest whose JSON is `null` or empty, and `readManifest` dereferences it before its own null checks. CI hides this for the reason above. It isn't fixed here; [DSPX-5073](https://virtru.atlassian.net/browse/DSPX-5073) tracks it. Until it is fixed, the `fuzzTDF` job fails, and because the workflow runs on push, that shows as a red check on the `main` commits that trigger it. [DSPX-4589]: https://virtru.atlassian.net/browse/DSPX-4589?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [DSPX-5070]: https://virtru.atlassian.net/browse/DSPX-5070?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [DSPX-5073]: https://virtru.atlassian.net/browse/DSPX-5073?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [DSPX-5073]: https://virtru.atlassian.net/browse/DSPX-5073?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Automated fuzz-testing checks now run for SDK ZIP and TDF input handling when relevant source files change, or when started manually. The checks run independently, preserve their test corpora, and collect failure reports and crash inputs. * Malformed-input exceptions are now surfaced to fuzz testing instead of being ignored. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary by CodeRabbit