fix: handle PlayStation ISO edge cases#13
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds chunked ISO9660 PVD scanning, stricter path-table validation, callback-based file walking, and path-based reads. PSP and PlayStation identifiers now use WalkFiles and SYSTEM.CNF parsing. A new internal testiso package builds minimal ISO images for tests. ChangesISO9660 Core Traversal and Validation
Identifier Serial and Root-File Discovery
Test ISO Fixture Builder
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant identifyPlayStation
participant playStationRootInfo
participant playStationRootInfoWalk
participant serialFromSystemCNFFile
identifyPlayStation->>playStationRootInfo: resolve rootFiles and serial
playStationRootInfo->>playStationRootInfoWalk: use WalkFiles when available
playStationRootInfoWalk->>serialFromSystemCNFFile: read SYSTEM.CNF
serialFromSystemCNFFile-->>playStationRootInfoWalk: serial or empty
playStationRootInfoWalk-->>playStationRootInfo: rootFiles, serial
playStationRootInfo-->>identifyPlayStation: metadata
sequenceDiagram
participant ISO9660.init
participant findPVDOffset
participant readHeaderChunk
participant parsePathTable
ISO9660.init->>findPVDOffset: locate PVD offset
findPVDOffset->>readHeaderChunk: scan header chunks
readHeaderChunk-->>findPVDOffset: chunk data or empty
findPVDOffset-->>ISO9660.init: offset or error
ISO9660.init->>parsePathTable: parse path table
parsePathTable->>parsePathTable: validate offset, size, parents
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
wizzomafizzo
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
iso9660/iso9660_test.go (1)
174-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winForce the fallback scan path in the chunked scan test.
The current ISO places the PVD at the standard sector, so
standardPVDOffset()succeeds with a tiny read and the chunked fallback scan is not exercised. Offset the image so this test fails if the fallback loop regresses.Proposed adjustment
- data := createMinimalISO("TEST", "PLAYSTATION", "") + data := append(make([]byte, 512), createMinimalISO("TEST", "PLAYSTATION", "")...) iso, err := OpenReader(maxReadReaderAt{data: data, maxRead: 2048}, int64(len(data)))identifier/psx_test.go (1)
90-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
WalkFiles-based dispatch path.
TestSerialFromSystemCNFonly exercises the pureserialFromSystemCNFstring parser. NeitherplayStationRootInfo's dispatch norplayStationRootInfoWalk(SYSTEM.CNF discovery viaWalkFiles,rootFilescompleteness) are exercised end-to-end. A mock implementingrootFileWalker/isoFileReader(alongside the existingmockPlayStationISO) would catch regressions like early-exit truncation ofrootFilesor SYSTEM.CNF-vs-IterFiles fallback inconsistencies.🤖 Prompt for 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. In `@identifier/psx_test.go` around lines 90 - 112, Add an end-to-end test that exercises the WalkFiles dispatch path rather than only serialFromSystemCNF, using a mock that implements rootFileWalker and isoFileReader alongside mockPlayStationISO. Verify playStationRootInfo and playStationRootInfoWalk discover SYSTEM.CNF through WalkFiles, correctly populate rootFiles, and still fall back consistently to IterFiles when needed, so regressions like early-exit truncation or lookup path mismatches are caught.
🤖 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 `@identifier/psx.go`:
- Around line 211-222: The closure parameter in serialFromSystemCNF’s
strings.FieldsFunc callback uses the single-character name r, which violates the
naming guideline. Rename that rune parameter to a longer descriptive name (for
example, runeValue) and update the unicode.IsLetter/IsDigit checks accordingly,
keeping the logic in serialFromSystemCNF unchanged.
- Around line 164-189: The early return in playStationRootInfoWalk is truncating
rootFiles because WalkFiles stops as soon as serial is found; update the
callback to always continue walking so every root entry is appended before
returning. Keep the serial detection logic in playStationRootInfoWalk (including
SYSTEM.CNF and serialFromRootFile), but remove the dependency on serial == ""
for stopping the walk so identifyPlayStation gets a complete, order-independent
root_files list.
- Around line 137-162: The fallback path in playStationRootInfo only checks
filenames via serialFromRootFile, so it misses SYSTEM.CNF-based serial
extraction when the ISO does not implement rootFileWalker. Update the IterFiles
branch to also try reading and parsing SYSTEM.CNF through the iso reader path
(as playStationRootInfoWalk does via serialFromSystemCNFFile), then keep the
existing root-file fallback and serial discovery flow. While touching this loop,
rename the single-letter file variable f to a descriptive name that follows the
project’s naming guideline.
In `@iso9660/iso9660_test.go`:
- Around line 160-171: The maxReadReaderAt.ReadAt method uses the one-letter
receiver r, which violates the variable naming guideline. Rename the receiver in
maxReadReaderAt.ReadAt to a two-character-or-longer name that matches the struct
context, and update all references within the method accordingly.
In `@iso9660/iso9660.go`:
- Around line 450-465: In WalkFiles, the directory traversal currently swallows
ReadAt failures by breaking out of the loop, which can hide truncated/corrupt
ISO errors and make callers like ReadFileByPath see ErrFileNotFound instead.
Update the ReadAt handling in the directory record parsing path to return the
underlying error instead of breaking, including the lenBuf read and the recBuf
read, so invalid directory data surfaces back to callers. Use the WalkFiles loop
and the iso.reader.ReadAt calls as the fix points.
---
Nitpick comments:
In `@identifier/psx_test.go`:
- Around line 90-112: Add an end-to-end test that exercises the WalkFiles
dispatch path rather than only serialFromSystemCNF, using a mock that implements
rootFileWalker and isoFileReader alongside mockPlayStationISO. Verify
playStationRootInfo and playStationRootInfoWalk discover SYSTEM.CNF through
WalkFiles, correctly populate rootFiles, and still fall back consistently to
IterFiles when needed, so regressions like early-exit truncation or lookup path
mismatches are caught.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90267b75-1151-47d1-9603-903ca85038bf
📒 Files selected for processing (5)
identifier/psp.goidentifier/psx.goidentifier/psx_test.goiso9660/iso9660.goiso9660/iso9660_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
iso9660/iso9660_test.go (2)
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo bounds check on file data size vs block size.
writeISOTestFileRecordcopiesfile.datadirectly into a singleblockSize-sized slot (data[fileLBA*blockSize:]) with no check thatlen(file.data) <= blockSize. If a future test uses a file payload larger than 2048 bytes, this will silently overwrite the next file's block without any error.
[optional_refactor]🛡️ Proposed defensive check
func createMinimalISOWithFiles(volumeID string, files []isoTestFile) []byte { const blockSize = 2048 base := createMinimalISO(volumeID, "SYS", "PUB") totalBlocks := 20 + len(files) data := make([]byte, totalBlocks*blockSize) copy(data, base) pvdOffset := 16 * blockSize binary.LittleEndian.PutUint32(data[pvdOffset+80:], mustUint32(totalBlocks)) binary.BigEndian.PutUint32(data[pvdOffset+84:], mustUint32(totalBlocks)) recordOffset := 19*blockSize + 68 for idx, file := range files { + if len(file.data) > blockSize { + panic("test ISO file data exceeds block size") + } fileLBA := 20 + idx writeISOTestFileRecord(data[recordOffset:], fileLBA, len(file.data), file.name) copy(data[fileLBA*blockSize:], file.data) recordOffset += isoTestDirectoryRecordLength(file.name) } return data }🤖 Prompt for 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. In `@iso9660/iso9660_test.go` around lines 172 - 177, The file data write in the loop over files can overflow a single block because `copy(data[fileLBA*blockSize:], file.data)` assumes `len(file.data)` always fits within `blockSize`. Add a defensive size check before writing each entry in this test helper, and fail the test explicitly if `file.data` exceeds one block so `writeISOTestFileRecord` and the `files` setup cannot silently corrupt the next block.
155-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate test-helper logic across packages.
mustUint32,mustByte,isoTestDirectoryRecordLength, and the record-writing helpers here are near-identical duplicates ofmustUint32,mustByte,directoryRecordLength, andwriteIdentifierRecordinidentifier/iso_test_helpers_test.go. Since these are test-only helpers in two separate packages, sharing them cleanly would require introducing an internal test-support package, but the current duplication increases the maintenance surface for any future ISO9660 layout tweaks.
[recommended_refactor]🤖 Prompt for 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. In `@iso9660/iso9660_test.go` around lines 155 - 216, The ISO test helpers here duplicate the same record-length, byte/uint32 bounds, and directory-record writing logic already present in the identifier test helpers. Refactor the shared behavior used by createMinimalISOWithFiles, writeISOTestFileRecord, isoTestDirectoryRecordLength, mustUint32, and mustByte into a single test-support helper location that both packages can use, and update these tests to call the shared helpers instead of maintaining near-identical copies.
🤖 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.
Nitpick comments:
In `@iso9660/iso9660_test.go`:
- Around line 172-177: The file data write in the loop over files can overflow a
single block because `copy(data[fileLBA*blockSize:], file.data)` assumes
`len(file.data)` always fits within `blockSize`. Add a defensive size check
before writing each entry in this test helper, and fail the test explicitly if
`file.data` exceeds one block so `writeISOTestFileRecord` and the `files` setup
cannot silently corrupt the next block.
- Around line 155-216: The ISO test helpers here duplicate the same
record-length, byte/uint32 bounds, and directory-record writing logic already
present in the identifier test helpers. Refactor the shared behavior used by
createMinimalISOWithFiles, writeISOTestFileRecord, isoTestDirectoryRecordLength,
mustUint32, and mustByte into a single test-support helper location that both
packages can use, and update these tests to call the shared helpers instead of
maintaining near-identical copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19ba0fb5-ac50-4266-9a5c-2572136ccbf7
📒 Files selected for processing (6)
identifier/iso_test_helpers_test.goidentifier/ps2_test.goidentifier/psp.goidentifier/psp_test.goidentifier/psx_test.goiso9660/iso9660_test.go
💤 Files with no reviewable changes (1)
- identifier/ps2_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- identifier/psp.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
identifier/psx_test.go (1)
216-232: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrefer element-wise slice comparison over joined-string comparison.
Comparing
rootFilesviastrings.Join(..., " / ")can produce false equality if any element itself contains" / "(e.g., after future filename normalization changes). Usingreflect.DeepEqualorslices.Equalavoids this edge case and gives clearer diff output on failure.♻️ Proposed fix
- wantRootFiles := []string{"SYSTEM.CNF", "README.TXT"} - if strings.Join(rootFiles, " / ") != strings.Join(wantRootFiles, " / ") { - t.Errorf("rootFiles = %v, want %v", rootFiles, wantRootFiles) - } + wantRootFiles := []string{"SYSTEM.CNF", "README.TXT"} + if !slices.Equal(rootFiles, wantRootFiles) { + t.Errorf("rootFiles = %v, want %v", rootFiles, wantRootFiles) + }🤖 Prompt for 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. In `@identifier/psx_test.go` around lines 216 - 232, The rootFiles assertion in assertPlayStationRootInfo compares slices by joining them into a string, which can hide element-level mismatches. Update this check to use an element-wise slice comparison such as slices.Equal or reflect.DeepEqual while keeping the existing wantRootFiles expectation, so failures in assertPlayStationRootInfo show the actual differing elements clearly.
🤖 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.
Nitpick comments:
In `@identifier/psx_test.go`:
- Around line 216-232: The rootFiles assertion in assertPlayStationRootInfo
compares slices by joining them into a string, which can hide element-level
mismatches. Update this check to use an element-wise slice comparison such as
slices.Equal or reflect.DeepEqual while keeping the existing wantRootFiles
expectation, so failures in assertPlayStationRootInfo show the actual differing
elements clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd9043a8-b37b-4359-83e1-f0c20dfac4ff
📒 Files selected for processing (4)
identifier/psx.goidentifier/psx_test.goiso9660/iso9660.goiso9660/iso9660_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- iso9660/iso9660_test.go
- identifier/psx.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/testiso/testiso.go`:
- Around line 92-101: Guard the root directory space in testISOSetup before
calling WriteFileRecord and copy: the loop that advances recordOffset in
internal/testiso/testiso.go can overflow the single-block root directory region.
Add a bounds check using rootOffset, BlockSize, and
DirectoryRecordLength(file.Name) so setup fails early if the file records would
spill past the advertised root directory, keeping the fixture from corrupting
payload data.
- Around line 119-131: The writeRecord helper currently writes fixed offsets and
copies the name into record without verifying the buffer is large enough, so
callers of WriteDirectoryRecord and WriteFileRecord can panic. Add a
length/capacity check at the start of writeRecord (using
DirectoryRecordLength(name) or the required minimum size) and fail through
tb.Fatal/tb.Fatalf with a clear message before any indexing or copy occurs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cf4066a-97a5-4f4a-a0d4-46592f632d83
📒 Files selected for processing (4)
identifier/psp_test.goidentifier/psx_test.gointernal/testiso/testiso.goiso9660/iso9660_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- identifier/psp_test.go
- identifier/psx_test.go
- iso9660/iso9660_test.go
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
1 participant
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
Tests
Summary by CodeRabbit
New Features
SYSTEM.CNFwhen available, with improved fallback behavior.Bug Fixes
Tests