feat(vcd): embed the BDMAssault variant pairs -- equip with zero user-supplied files (POPSLoader parity) - #251
Conversation
…O user-supplied files (maintainer directive, POPSLoader parity) Maintainer, after the ATA BDMA equip silently failed on an MC boot: "POPSLoader has modules embedded in the elf, and then pastes them according to the VCD device if needed." Implemented exactly that: - modules/bdmassault/: the 7 UNIQUE variant blobs (usbexfat/mx4sio/mmce/ata pairs; usbd.irx.mx4sio is byte-identical to usbd.irx.usbexfat and shipped once -- sha256-verified), with the AFL v2.0 LICENSE and a full PROVENANCE.md (pinned sources, sha256 table, the mutable-ATA-Assault-tag warning, the unpinned-mmce-lineage caveat). All sources are AFL -- same family as OPL; POPSLoader (GPL-3) shipping these identical bytes embedded is direct precedent. - Build: each blob gzipped (-9 -n, reproducible) + BIN2C'd (~194 KB raw -> ~half embedded); zlib is already linked via libpng, so the inflate costs zero new dependencies. - vcdEquipBdma: the seek order is UNCHANGED (Custom POPSTARTER dir -> boot device POPS/ -> game device family) so user-supplied newer files still WIN -- the embedded pair is the FINAL FALLBACK, filling what used to be a hard -4 "source files absent" failure (the guaranteed outcome of an MC boot, where no device carries a POPS folder). Unpack is sequential (one blob inflated at a time, ~48.5 KiB transient cap), staged through the existing tmp -> commit machinery, torn-pair handling identical. - The #243 no-marker manual-pair fast path now LOGS its skip (it silently ate the maintainer's expected ATA equip on a card left over from manual testing). Behavior deliberately unchanged -- a manual pair is still trusted, never clobbered; one Settings-screen equip writes the marker and restores auto-correction. With this + the vendored-atad PR, an out-of-the-box MC boot can equip ANY variant with nothing pre-placed on any device. Builds clean (make opl.elf, exit 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces embedded BDMAssault variant driver pairs as a final fallback when no user-supplied drivers are found on any seek-path device. The drivers are gzipped at build time, embedded using bin2c, and inflated at runtime using zlib. Feedback on the changes suggests using the existing vcdSafeWriteFile function instead of introducing a redundant vcdWriteBufFile helper, along with adding defensive size checks. Additionally, it is recommended to propagate specific error codes (such as out-of-space or IO errors) during staging rather than unconditionally returning a generic error code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Write a memory buffer to path (O_CREAT|O_TRUNC). Removes the partial file on any failure -- | ||
| // same contract as vcdSafeCopyFile, for the private staging names only. | ||
| static int vcdWriteBufFile(const char *path, const unsigned char *buf, unsigned int len) | ||
| { | ||
| int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); | ||
| if (fd < 0) | ||
| return -3; | ||
| int wr = write(fd, buf, len); | ||
| int cr = close(fd); | ||
| if (wr != (int)len || cr < 0) { | ||
| unlink(path); | ||
| return -3; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| // Unpack the embedded pair for `mode` into the two staging paths, SEQUENTIALLY (one blob inflated at | ||
| // a time caps the transient heap at ~48.5 KiB + zlib state). Any failure leaves no partial staging | ||
| // file behind (vcdWriteBufFile removes its own partials; the caller unlinks both names regardless). | ||
| static int vcdStageEmbeddedPair(int mode, const char *tmp0, const char *tmp1) | ||
| { | ||
| if (mode <= VCD_BDMA_FAT32 || mode >= VCD_BDMA_MODE_COUNT || vcdBdmaEmbedded[mode].usbdGz == NULL) | ||
| return -1; | ||
| unsigned char *buf = NULL; | ||
| unsigned int len = 0; | ||
| if (vcdInflateGzip((const unsigned char *)vcdBdmaEmbedded[mode].usbdGz, (unsigned int)*vcdBdmaEmbedded[mode].usbdGzLen, &buf, &len) != 0) | ||
| return -1; | ||
| int r = vcdWriteBufFile(tmp0, buf, len); | ||
| free(buf); | ||
| if (r != 0) | ||
| return r; | ||
| buf = NULL; | ||
| if (vcdInflateGzip((const unsigned char *)vcdBdmaEmbedded[mode].hdfsdGz, (unsigned int)*vcdBdmaEmbedded[mode].hdfsdGzLen, &buf, &len) != 0) | ||
| return -1; | ||
| r = vcdWriteBufFile(tmp1, buf, len); | ||
| free(buf); | ||
| return r; | ||
| } |
There was a problem hiding this comment.
The helper function vcdWriteBufFile is redundant because vcdSafeWriteFile already exists and provides robust free-space checking (vcdMcHasSpace) and a proper write loop to handle partial writes. Additionally, checking the embedded sizes defensively before casting them to unsigned int prevents potential out-of-bounds reads if the sizes are somehow invalid or negative.
// Unpack the embedded pair for `mode` into the two staging paths, SEQUENTIALLY (one blob inflated at
// a time caps the transient heap at ~48.5 KiB + zlib state). Any failure leaves no partial staging
// file behind (vcdSafeWriteFile removes its own partials; the caller unlinks both names regardless).
static int vcdStageEmbeddedPair(int mode, const char *tmp0, const char *tmp1)
{
if (mode <= VCD_BDMA_FAT32 || mode >= VCD_BDMA_MODE_COUNT || vcdBdmaEmbedded[mode].usbdGz == NULL)
return -1;
int usbdLen = *vcdBdmaEmbedded[mode].usbdGzLen;
int hdfsdLen = *vcdBdmaEmbedded[mode].hdfsdGzLen;
if (usbdLen < 18 || hdfsdLen < 18)
return -1;
unsigned char *buf = NULL;
unsigned int len = 0;
if (vcdInflateGzip((const unsigned char *)vcdBdmaEmbedded[mode].usbdGz, (unsigned int)usbdLen, &buf, &len) != 0)
return -1;
int r = vcdSafeWriteFile(tmp0, buf, len);
free(buf);
if (r != 0)
return r;
buf = NULL;
if (vcdInflateGzip((const unsigned char *)vcdBdmaEmbedded[mode].hdfsdGz, (unsigned int)hdfsdLen, &buf, &len) != 0)
return -1;
r = vcdSafeWriteFile(tmp1, buf, len);
free(buf);
return r;
}| if (useEmbedded) { | ||
| r = vcdStageEmbeddedPair(mode, tmp0, tmp1); | ||
| if (r != 0) { | ||
| unlink(tmp0); | ||
| unlink(tmp1); | ||
| if (diag != NULL && diagSize > 0) | ||
| snprintf(diag, diagSize, "No %s BDMA files on any device, and the built-in pair failed to unpack.", suffix); | ||
| return -4; | ||
| } |
There was a problem hiding this comment.
When vcdStageEmbeddedPair fails, returning -4 unconditionally misreports the error to the user. If the staging fails due to the memory card being full (-2) or an IO error (-3), returning the actual error code r allows vcdEnsureBdmaForLaunch to display the correct warning (e.g., _STR_BDMA_ERR_SPACE or _STR_BDMA_ERR_IO) instead of incorrectly claiming that the source files are absent.
if (useEmbedded) {
r = vcdStageEmbeddedPair(mode, tmp0, tmp1);
if (r != 0) {
unlink(tmp0);
unlink(tmp1);
if (diag != NULL && diagSize > 0)
snprintf(diag, diagSize, "No %s BDMA files on any device, and the built-in pair failed to unpack.", suffix);
return (r == -2 || r == -3) ? r : -4;
}
}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (20)
🔇 Additional comments (2)
📝 WalkthroughWalkthroughBDMAssault driver variants and provenance records are added, Makefile rules embed deterministic gzip-compressed blobs, and ChangesBDMAssault embedded fallback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant vcdEquipBdma
participant EmbeddedBDMA
participant zlib
participant MemoryCard
vcdEquipBdma->>EmbeddedBDMA: Select variant gzip blobs
vcdEquipBdma->>zlib: Inflate both driver blobs
zlib-->>vcdEquipBdma: Return uncompressed driver data
vcdEquipBdma->>MemoryCard: Write staged driver pair
MemoryCard-->>vcdEquipBdma: Confirm staged files
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vcdsupport.c`:
- Around line 1001-1019: Update the return-code documentation for vcdEquipBdma
in its header comment to describe -4 as the case where no device has the BDMA
pair and the embedded fallback also fails, rather than simply source variant
files being absent. Keep the existing meanings for the other return codes
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6227a9f1-dfec-4430-a2ef-63189825aec6
📒 Files selected for processing (12)
Makefileinclude/bdma_embed.hmodules/bdmassault/LICENSEmodules/bdmassault/PROVENANCE.mdmodules/bdmassault/usbd.irx.atamodules/bdmassault/usbd.irx.mmcemodules/bdmassault/usbd.irx.usbexfatmodules/bdmassault/usbhdfsd.irx.atamodules/bdmassault/usbhdfsd.irx.mmcemodules/bdmassault/usbhdfsd.irx.mx4siomodules/bdmassault/usbhdfsd.irx.usbexfatsrc/vcdsupport.c
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: build-variants (EXTRA_FEATURES=1, PADEMU=0)
- GitHub Check: build-debug-ps2dev-latest (iopcore_debug)
- GitHub Check: build-debug-ps2dev-latest (ingame_debug)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=0, PADEMU=0)
- GitHub Check: build-debug (iopcore_debug, :v20250725-2)
- GitHub Check: build-debug-ps2dev-latest (ingame_ppctty_debug)
- GitHub Check: build-debug (eesio_debug, :v20250725-2)
- GitHub Check: build-variants (EXTRA_FEATURES=0, PADEMU=1)
- GitHub Check: build-debug (iopcore_ppctty_debug, :v20250725-2)
- GitHub Check: build-debug (ingame_debug, :v20250725-2)
🧰 Additional context used
🪛 ast-grep (0.44.1)
src/vcdsupport.c
[warning] 796-796: This call makes a world-writable file which allows any user on a machine to write to the file. This may allow attackers to influence the behaviour of this process by writing to the file.
Context: 0666
Note: [CWE-732]: Incorrect Permission Assignment for Critical Resource
(world-writable-file-c)
🪛 LanguageTool
modules/bdmassault/PROVENANCE.md
[style] ~35-~35: Consider an alternative for the overused word “exactly”.
Context: ... WARNING: that tag is MUTABLE, which is exactly why the bytes are vendored here with sh...
(EXACTLY_PRECISELY)
🔇 Additional comments (17)
modules/bdmassault/LICENSE (1)
1-167: LGTM!modules/bdmassault/PROVENANCE.md (1)
1-49: LGTM!modules/bdmassault/usbd.irx.ata (1)
1-117: LGTM!modules/bdmassault/usbd.irx.mmce (1)
1-48: LGTM!modules/bdmassault/usbd.irx.usbexfat (1)
1-79: LGTM!modules/bdmassault/usbhdfsd.irx.ata (1)
1-95: LGTM!modules/bdmassault/usbhdfsd.irx.mmce (1)
1-48: LGTM!modules/bdmassault/usbhdfsd.irx.mx4sio (1)
1-38: LGTM!modules/bdmassault/usbhdfsd.irx.usbexfat (1)
1-102: LGTM!Makefile (2)
129-129: LGTM!
843-867: 📐 Maintainability & Code QualityNo action needed:
gzipis already installed in the supported CI Alpine environments and listed in the host-tool check, so this doesn’t add a new build-tool requirement.> Likely an incorrect or invalid review comment.include/bdma_embed.h (1)
1-20: LGTM!src/vcdsupport.c (5)
27-28: LGTM!
758-791: LGTM! Correct ISIZE-based sizing, sane cap, and correctinflateInit2(&z, 15 + 16)gzip-only windowBits usage (confirmed against the zlib manual: adding 16 decodes gzip-only, matching data produced bygzip -9 -n -c). No leaks on any failure path.
795-807: 🔒 Security & Privacy | 💤 Low valueStatic analysis flags world-writable file creation (0666).
On the PS2 memory-card/IOP filesystem POSIX permission bits aren't enforced, so this is very unlikely to matter in practice; flagging only because static analysis surfaced it and per its own severity classification.
Source: Linters/SAST tools
809-830: LGTM! Sequential inflate correctly bounds transient heap use as documented, and cleanup on any staging failure is handled by the caller's unconditionalunlink(tmp0)/unlink(tmp1).
1101-1109: LGTM! Pure logging addition on the existing no-marker manual-pair fast path; no behavior change.
- vcdWriteBufFile deleted: the EXISTING vcdSafeWriteFile (free-space check + partial-write cleanup) stages the embedded pair (Gemini -- real, the helper was redundant and weaker). - Staging failures propagate their REAL class: -2 card-full / -3 IO keep their specific toasts; only no-pair/unpack-failure maps to -4 (Gemini -- real, a full card mis-toasted as 'source absent'). - The -4 contract comment in vcdsupport.h updated for the embedded-fallback era (CodeRabbit -- real). #250's Gemini findings (NULL param + device bounds in the vendored atad) are DECLINED under the vendoring doctrine: all target upstream-verbatim code, byte-faithful to the pin except the four documented FORK hunks; no OPL caller passes invalid units, and drive-by hardening of vendored source makes every re-vendor a conflict festival. Recorded in ORIGIN.txt's spirit -- upstream fixes belong upstream. Builds clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Done, byte-for-byte the same architecture:
modules/bdmassault/) with sha256-pinned provenance — the usbexfat/mx4siousbd.irxis one shared blob (verified identical). All AFL-licensed (our license family); POPSLoader shipping these exact bytes is precedent. Full source table + caveats inPROVENANCE.md.Paired with #250 (vendored atad): an out-of-the-box MC boot can now equip any variant with nothing pre-placed anywhere.
Builds clean.
🤖 Generated with Claude Code
Summary by CodeRabbit
-4outcome when fallback installation also can’t proceed.