Adding memory card support to psyqo#2042
Open
nicolasnoble wants to merge 7 commits into
Open
Conversation
Inverts the existing sjis-table.h decode table into a Unicode -> Shift-JIS mapping, so code that needs to write Shift-JIS (memory card save titles and the like) can encode from UTF-8 or ASCII instead of hand-rolling a partial table. Ships the generator, the generated table (sorted for binary search), and a small header exposing unicodeToSjis, a UTF-8 string encoder, and a fullwidth-title helper matching the BIOS save-title convention. Verified by round-tripping encoded output back through the existing Shift-JIS decoder. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Adds a PSYQo memory card driver in two layers: a low level MemoryCard transport (the raw 128-byte sector protocol over SIO0, interrupt driven, exposed as callback, blocking, awaiter and TaskQueue forms) and a Sony-compatible MemoryCardFileSystem on top of it (format, list, read, write and delete files that the retail BIOS manager shows with their title and icon, and can copy or delete itself). The controllers and the card share the single SIO0 bus and cannot drive it at the same time, and a card transaction can span several frames, so an SIO0Bus ownership lock is held for the whole transaction rather than per sector; AdvancedPad consults it and stands down its frame polling while a card operation is in flight. Because of this the driver only works alongside AdvancedPad, which drives SIO0 directly; using SimplePad (the BIOS-driven pad) at the same time touches the bus outside the lock and is undefined. Save titles are supplied as UTF-8 and encoded to the Shift-JIS field the BIOS displays, with printable ASCII promoted to its fullwidth form. Includes a worked example exercising format, write, read-back and delete. Co-authored-by: Matt Hadden <forceh2010@gmail.com> Co-authored-by: Bandwidth <jracek@honzuvkod.dev> Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The SIO0 card protocol is timing sensitive within a single sector transfer: the bytes must flow without interruption, and the card has no dependable acknowledge interrupt to lean on. So a sector is now a busy-polled exchange run with all interrupts disabled (fastEnterCriticalSection), polling the latched acknowledge bit, and the controller-IRQ state machine is removed entirely. The per-sector transfer is intentionally synchronous: it is the atomic unit that the timer-driven filesystem layer chains across frames. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
…hine A memory card is itself a small state machine whose timing varies by card and by phase, and reading the data register before the chip has advanced is a no-op rather than a stale byte. So a sector transfer now clocks each byte and then polls the card's output until it reaches the expected reply: this self-synchronizes to the card's per-state timing without tracking a fixed full-duplex offset. The identifier and command-acknowledge bytes are the slow, variable states that are polled; the data phase is regular and acknowledge-paced. Every protocol byte is named rather than a hanging hex literal, and the inner transfer runs with interrupts disabled. The read path follows an inner loop verified on hardware; the write path mirrors its structure over the standard protocol and still needs validation on a real card. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The memory card filesystem now follows the same asynchronous contract as CDRomDevice. A whole operation is a transaction spanning many sector transfers across several frames, so each is driven by a main-loop timer (never an interrupt): one sector is transferred per tick and the game keeps running in between, and only one operation runs at a time. Every operation has a callback form (the basis: it takes a GPU to arm the timer and reports its result to an eastl::function during pumping) and a *Blocking(GPU&) form that pumps until it completes. The SIO0 bus is owned for the whole transaction, so AdvancedPad stands down for its full duration. Internally each operation is a small state machine over shared phases (read the directory, write the directory, read or write data) advanced one sector per tick. The example exercises format, write, read-back, list and delete. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Testing the memory card driver on real hardware surfaced a set of fixes plus assorted cleanups across the transport and filesystem layers. The main correctness fix is an inter-transaction recovery delay before each port (re)select: the retail BIOS gets that recovery for free by driving one sector per vblank, but back-to-back transactions can reselect a slow third-party card too soon and read stale data that still carries a valid checksum. The port-select sequence is also corrected to enable the transmitter and the acknowledge IRQ before the bus-settle delay, so a present card is not misread as absent on the first byte. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
📝 WalkthroughWalkthroughAdds a complete memory card subsystem to the psyqo MIPS library: a Changespsyqo Memory Card Stack
Sequence Diagram(s)sequenceDiagram
participant App
participant MemoryCardFileSystem
participant MemoryCard
participant SIO0Bus
participant SIO0HW as SIO0 Hardware
App->>MemoryCardFileSystem: writeFile(port, name, title, icon, data, len, cb)
MemoryCardFileSystem->>MemoryCardFileSystem: begin() — preflight block count
MemoryCardFileSystem->>SIO0Bus: acquire() via Lock
MemoryCardFileSystem->>MemoryCard: readSector(header sector)
MemoryCard->>SIO0HW: startTransfer / IRQ state machine
SIO0HW-->>MemoryCard: IRQ per byte → finishTransfer
MemoryCard-->>MemoryCardFileSystem: onSectorDone(OK)
MemoryCardFileSystem->>MemoryCard: readSector(dir frames ×15)
MemoryCard-->>MemoryCardFileSystem: onSectorDone(OK) ×15
MemoryCardFileSystem->>MemoryCardFileSystem: afterReadDir() — allocate blocks
MemoryCardFileSystem->>MemoryCard: writeSector(title/icon/data frames)
MemoryCard-->>MemoryCardFileSystem: onSectorDone(OK) per frame
MemoryCardFileSystem->>MemoryCard: writeSector(dirty directory entries)
MemoryCard-->>MemoryCardFileSystem: onSectorDone(OK)
MemoryCardFileSystem->>SIO0Bus: release()
MemoryCardFileSystem-->>App: callback(Error::OK)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/mips/psyqo/src/memory-card.cpp (1)
718-758: 🩺 Stability & Availability | 🔵 TrivialAsync sector transfers should acquire SIO0Bus for their in-flight duration to prevent AdvancedPad from corrupting the transfer.
The callback variants of
readSectorandwriteSectorreturn immediately and let the interrupt state machine run across frames without acquiringSIO0Bus, relying entirely on the caller to hold the bus. While the header comment documents this caller responsibility and the current codebase respects it (the filesystem transaction holds the outer lock), this is a footgun waiting for a direct caller to bypass the safeguard. AcquireSIO0BusinstartAsyncAttemptand release inonAsyncAttemptDone; the re-entrant counter tolerates nested acquisition from the filesystem's outer lock.🤖 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 `@src/mips/psyqo/src/memory-card.cpp` around lines 718 - 758, The async sector transfer methods `readSector` and `writeSector` initiate async operations via `startAsyncAttempt` without acquiring the SIO0Bus lock, relying entirely on the caller to hold it and creating a footgun for direct callers. To fix this, acquire SIO0Bus at the beginning of `startAsyncAttempt` method and release it in the `onAsyncAttemptDone` method that completes the async operation. Since SIO0Bus uses a re-entrant counter, nested acquisition from the filesystem's outer lock will be safely tolerated.
🤖 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/mips/common/util/gen-sjis-encode-table.py`:
- Around line 17-19: The if, elif, and else statements on lines 17-19 contain
multiple statements on a single line (condition and assignment together),
violating Ruff E701/E702 rules. Split each statement by moving the assignment of
base and lead_hi to a new line below the if/elif/else condition, so each line
contains only one statement. Apply the same fix to line 46 which has a similar
multi-statement violation.
In `@src/mips/psyqo/sio0-bus.hh`:
- Around line 46-47: The documentation comment at lines 46-47 is misleading
about how the re-entrant count works. The current wording implies that the
low-level driver function irqTransferBlocking() also acquires and releases the
lock, but it never actually calls acquire() or release(). Only the filesystem
layer (MemoryCardFileSystem) holds the lock across multiple sector operations.
Update the documentation to clarify that the re-entrant count exists because
filesystem operations hold the lock across all their sector transfers, not
because the blocking transfer function itself participates in arbitration. Make
it clear that only the filesystem operation caller (not the driver) acquires and
holds the bus.
In `@src/mips/psyqo/src/memory-card-filesystem.cpp`:
- Around line 229-238: The Phase::WriteDir case commits dirty directory entries
by index order, but this creates a safety issue with the multi-entry chain logic
built elsewhere (around lines 347-377) which requires the first entry of a chain
to be written last. Currently, if power loss occurs after writing the first
entry in a chain but before all linked entries are written, an incomplete file
chain can be exposed on the card. Modify the WriteDir phase to commit directory
entries in a specific order where the first entry of any chain is written LAST,
after all its linked entries are already committed. This ensures the "first
entry last" rule is applied consistently throughout the write and overwrite
paths, preventing partially committed files from being visible on interruption.
- Around line 444-455: The block chain traversal loop exits when the block value
goes out of range but still proceeds to the ReadTitle phase as if it completed
successfully. Add validation after the while loop to check if the chain
terminated cleanly by verifying that either the 0xffff terminator was
encountered or the chain length reached its maximum. If the loop exited due to
block going out of bounds without a clean termination, set m_result to
Error::BadData and return StepResult::Done instead of proceeding to the
ReadTitle phase.
- Around line 128-133: The totalBytes calculation at line 128 uses uint32_t
which can overflow when m_dataLen is large, allowing the FileTooLarge check to
be bypassed silently. Before computing totalBytes, check if adding
m_headerFrames multiplied by MemoryCard::c_sectorSize and m_dataLen would
overflow a uint32_t, or alternatively compute totalBytes using a larger type
like uint64_t and verify the result does not exceed the uint32_t maximum before
proceeding with the block-count calculation and the existing bounds check.
---
Nitpick comments:
In `@src/mips/psyqo/src/memory-card.cpp`:
- Around line 718-758: The async sector transfer methods `readSector` and
`writeSector` initiate async operations via `startAsyncAttempt` without
acquiring the SIO0Bus lock, relying entirely on the caller to hold it and
creating a footgun for direct callers. To fix this, acquire SIO0Bus at the
beginning of `startAsyncAttempt` method and release it in the
`onAsyncAttemptDone` method that completes the async operation. Since SIO0Bus
uses a re-entrant counter, nested acquisition from the filesystem's outer lock
will be safely tolerated.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f213614b-0fe8-4a0f-b059-576161ffe694
📒 Files selected for processing (11)
src/mips/common/util/gen-sjis-encode-table.pysrc/mips/common/util/sjis-encode-table.hsrc/mips/common/util/sjis-encode.hsrc/mips/psyqo/examples/memorycard/Makefilesrc/mips/psyqo/examples/memorycard/memorycard.cppsrc/mips/psyqo/memory-card-filesystem.hhsrc/mips/psyqo/memory-card.hhsrc/mips/psyqo/sio0-bus.hhsrc/mips/psyqo/src/advancedpad.cppsrc/mips/psyqo/src/memory-card-filesystem.cppsrc/mips/psyqo/src/memory-card.cpp
Comment on lines
+17
to
+19
| if index < 0x1100: base, lead_hi = 0x100, 0x80 | ||
| elif index < 0x2100: base, lead_hi = 0x1100, 0x90 | ||
| else: base, lead_hi = 0x2100, 0xe0 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve Ruff E701/E702 inline-statement violations.
Line 17, Line 18, Line 19, and Line 46 use multi-statements on one line and currently fail the configured lint rules.
💡 Suggested cleanup
def index_to_sjis(index):
if index < 0x100:
return index
- if index < 0x1100: base, lead_hi = 0x100, 0x80
- elif index < 0x2100: base, lead_hi = 0x1100, 0x90
- else: base, lead_hi = 0x2100, 0xe0
+ if index < 0x1100:
+ base, lead_hi = 0x100, 0x80
+ elif index < 0x2100:
+ base, lead_hi = 0x1100, 0x90
+ else:
+ base, lead_hi = 0x2100, 0xe0
@@
tok = "{0x%04x, 0x%04x}, " % (uni, sjis)
if len(line) + len(tok) > 116:
- f.write(line.rstrip() + "\n"); line = " "
+ f.write(line.rstrip() + "\n")
+ line = " "Also applies to: 46-46
🧰 Tools
🪛 Ruff (0.15.18)
[error] 17-17: Multiple statements on one line (colon)
(E701)
[error] 18-18: Multiple statements on one line (colon)
(E701)
[error] 19-19: Multiple statements on one line (colon)
(E701)
🤖 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 `@src/mips/common/util/gen-sjis-encode-table.py` around lines 17 - 19, The if,
elif, and else statements on lines 17-19 contain multiple statements on a single
line (condition and assignment together), violating Ruff E701/E702 rules. Split
each statement by moving the assignment of base and lead_hi to a new line below
the if/elif/else condition, so each line contains only one statement. Apply the
same fix to line 46 which has a similar multi-statement violation.
Source: Linters/SAST tools
Comment on lines
+46
to
+47
| * The count is re-entrant: a filesystem operation holds it across all of its | ||
| * sector transfers even though each blocking sector transfer also takes it. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the low-level MemoryCard driver ever acquires SIO0Bus.
echo "=== SIO0Bus references across psyqo ==="
rg -nH -C2 'SIO0Bus' src/mips/psyqo
echo "=== sio0-bus.hh include in memory-card.cpp? ==="
rg -nH 'sio0-bus' src/mips/psyqo/src/memory-card.cpp || echo "no include"Repository: grumpycoders/pcsx-redux
Length of output: 2140
🏁 Script executed:
sed -n '40,60p' src/mips/psyqo/sio0-bus.hhRepository: grumpycoders/pcsx-redux
Length of output: 1114
🏁 Script executed:
ast-grep outline src/mips/psyqo/src/memory-card.cpp | head -50Repository: grumpycoders/pcsx-redux
Length of output: 210
🏁 Script executed:
wc -l src/mips/psyqo/src/memory-card.cpp && head -100 src/mips/psyqo/src/memory-card.cppRepository: grumpycoders/pcsx-redux
Length of output: 3633
🏁 Script executed:
rg -nH 'blocking|transfer|readBlock|writeBlock' src/mips/psyqo/src/memory-card.cpp | head -30Repository: grumpycoders/pcsx-redux
Length of output: 1536
🏁 Script executed:
sed -n '400,430p' src/mips/psyqo/src/memory-card.cppRepository: grumpycoders/pcsx-redux
Length of output: 1282
🏁 Script executed:
sed -n '750,800p' src/mips/psyqo/src/memory-card.cpp && echo "---" && rg 'acquire|release' src/mips/psyqo/src/memory-card.cppRepository: grumpycoders/pcsx-redux
Length of output: 2615
Correct documentation: the low-level driver does not acquire the SIO0Bus.
The blocking sector transfer function (irqTransferBlocking()) in src/mips/psyqo/src/memory-card.cpp never calls acquire() or release(). The re-entrant count exists because the filesystem layer (MemoryCardFileSystem) holds the lock across multiple sector operations, not because blocking transfers self-arbitrate.
The current wording at lines 46–47 is misleading: "a filesystem operation holds it across all of its sector transfers even though each blocking sector transfer also takes it" falsely implies the low-level driver participates in arbitration. Update the documentation to clarify that only the filesystem layer (the operation caller) acquires and holds the bus.
The current code is functionally safe (blocking transfers run synchronously on the main thread, and AdvancedPad respects the owned() check), but the contract must be explicit to prevent a future maintainer from incorrectly assuming the driver self-protects.
🤖 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 `@src/mips/psyqo/sio0-bus.hh` around lines 46 - 47, The documentation comment
at lines 46-47 is misleading about how the re-entrant count works. The current
wording implies that the low-level driver function irqTransferBlocking() also
acquires and releases the lock, but it never actually calls acquire() or
release(). Only the filesystem layer (MemoryCardFileSystem) holds the lock
across multiple sector operations. Update the documentation to clarify that the
re-entrant count exists because filesystem operations hold the lock across all
their sector transfers, not because the blocking transfer function itself
participates in arbitration. Make it clear that only the filesystem operation
caller (not the driver) acquires and holds the bus.
Comment on lines
+128
to
+133
| uint32_t totalBytes = m_headerFrames * MemoryCard::c_sectorSize + m_dataLen; | ||
| m_blocksNeeded = (totalBytes + (MemoryCard::c_blockSize - 1)) >> 13; // ceil over 8192 | ||
| if (m_blocksNeeded < 1) m_blocksNeeded = 1; | ||
| if (m_blocksNeeded > 15) { | ||
| m_result = Error::FileTooLarge; | ||
| m_phase = Phase::Fail; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Guard totalBytes against overflow before block-count calculation.
Line 128 computes totalBytes in uint32_t; large dataLen can wrap and bypass the FileTooLarge check, causing silent truncation instead of a deterministic error.
💡 Suggested fix
- uint32_t totalBytes = m_headerFrames * MemoryCard::c_sectorSize + m_dataLen;
- m_blocksNeeded = (totalBytes + (MemoryCard::c_blockSize - 1)) >> 13; // ceil over 8192
+ const uint32_t headerBytes = m_headerFrames * MemoryCard::c_sectorSize;
+ const uint32_t maxCardBytes = 15u * MemoryCard::c_blockSize;
+ if (headerBytes > maxCardBytes || m_dataLen > (maxCardBytes - headerBytes)) {
+ m_result = Error::FileTooLarge;
+ m_phase = Phase::Fail;
+ return;
+ }
+ const uint32_t totalBytes = headerBytes + m_dataLen;
+ m_blocksNeeded = (totalBytes + (MemoryCard::c_blockSize - 1)) >> 13; // ceil over 8192📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uint32_t totalBytes = m_headerFrames * MemoryCard::c_sectorSize + m_dataLen; | |
| m_blocksNeeded = (totalBytes + (MemoryCard::c_blockSize - 1)) >> 13; // ceil over 8192 | |
| if (m_blocksNeeded < 1) m_blocksNeeded = 1; | |
| if (m_blocksNeeded > 15) { | |
| m_result = Error::FileTooLarge; | |
| m_phase = Phase::Fail; | |
| const uint32_t headerBytes = m_headerFrames * MemoryCard::c_sectorSize; | |
| const uint32_t maxCardBytes = 15u * MemoryCard::c_blockSize; | |
| if (headerBytes > maxCardBytes || m_dataLen > (maxCardBytes - headerBytes)) { | |
| m_result = Error::FileTooLarge; | |
| m_phase = Phase::Fail; | |
| return; | |
| } | |
| const uint32_t totalBytes = headerBytes + m_dataLen; | |
| m_blocksNeeded = (totalBytes + (MemoryCard::c_blockSize - 1)) >> 13; // ceil over 8192 | |
| if (m_blocksNeeded < 1) m_blocksNeeded = 1; | |
| if (m_blocksNeeded > 15) { | |
| m_result = Error::FileTooLarge; | |
| m_phase = Phase::Fail; |
🤖 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 `@src/mips/psyqo/src/memory-card-filesystem.cpp` around lines 128 - 133, The
totalBytes calculation at line 128 uses uint32_t which can overflow when
m_dataLen is large, allowing the FileTooLarge check to be bypassed silently.
Before computing totalBytes, check if adding m_headerFrames multiplied by
MemoryCard::c_sectorSize and m_dataLen would overflow a uint32_t, or
alternatively compute totalBytes using a larger type like uint64_t and verify
the result does not exceed the uint32_t maximum before proceeding with the
block-count calculation and the existing bounds check.
Comment on lines
+444
to
+455
| while (block >= 1 && block <= 15 && m_chainLen < 16) { | ||
| if (visited & (1 << block)) { | ||
| m_result = Error::BadData; | ||
| return StepResult::Done; | ||
| } | ||
| visited |= (1 << block); | ||
| m_chain[m_chainLen++] = static_cast<uint8_t>(block); | ||
| uint16_t next = get16(m_dir[block - 1].bytes + c_offNext); | ||
| if (next == 0xffff) break; | ||
| block = next + 1; | ||
| } | ||
| m_phase = Phase::ReadTitle; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject malformed block chains that do not terminate cleanly.
Line 444 exits the traversal when the next block goes out-of-range, but still proceeds as success. That can return partial/truncated data as OK instead of Error::BadData.
💡 Suggested fix
m_chainLen = 0;
uint16_t visited = 0;
int block = first;
+ bool terminated = false;
while (block >= 1 && block <= 15 && m_chainLen < 16) {
if (visited & (1 << block)) {
m_result = Error::BadData;
return StepResult::Done;
}
visited |= (1 << block);
m_chain[m_chainLen++] = static_cast<uint8_t>(block);
uint16_t next = get16(m_dir[block - 1].bytes + c_offNext);
- if (next == 0xffff) break;
+ if (next == 0xffff) {
+ terminated = true;
+ break;
+ }
block = next + 1;
}
+ if (!terminated) {
+ m_result = Error::BadData;
+ return StepResult::Done;
+ }
m_phase = Phase::ReadTitle;
return StepResult::Continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (block >= 1 && block <= 15 && m_chainLen < 16) { | |
| if (visited & (1 << block)) { | |
| m_result = Error::BadData; | |
| return StepResult::Done; | |
| } | |
| visited |= (1 << block); | |
| m_chain[m_chainLen++] = static_cast<uint8_t>(block); | |
| uint16_t next = get16(m_dir[block - 1].bytes + c_offNext); | |
| if (next == 0xffff) break; | |
| block = next + 1; | |
| } | |
| m_phase = Phase::ReadTitle; | |
| m_chainLen = 0; | |
| uint16_t visited = 0; | |
| int block = first; | |
| bool terminated = false; | |
| while (block >= 1 && block <= 15 && m_chainLen < 16) { | |
| if (visited & (1 << block)) { | |
| m_result = Error::BadData; | |
| return StepResult::Done; | |
| } | |
| visited |= (1 << block); | |
| m_chain[m_chainLen++] = static_cast<uint8_t>(block); | |
| uint16_t next = get16(m_dir[block - 1].bytes + c_offNext); | |
| if (next == 0xffff) { | |
| terminated = true; | |
| break; | |
| } | |
| block = next + 1; | |
| } | |
| if (!terminated) { | |
| m_result = Error::BadData; | |
| return StepResult::Done; | |
| } | |
| m_phase = Phase::ReadTitle; | |
| return StepResult::Continue; |
🧰 Tools
🪛 GitHub Check: CodeScene Code Health Review (main)
[warning] 444-444: ❌ New issue: Complex Conditional
psyqo::MemoryCardFileSystem::afterReadDir has 1 complex conditionals with 2 branches, threshold = 2
A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple,
logical operators such as AND/OR. The more logical operators in an expression, the more severe the code
smell.
🤖 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 `@src/mips/psyqo/src/memory-card-filesystem.cpp` around lines 444 - 455, The
block chain traversal loop exits when the block value goes out of range but
still proceeds to the ReadTitle phase as if it completed successfully. Add
validation after the while loop to check if the chain terminated cleanly by
verifying that either the 0xffff terminator was encountered or the chain length
reached its maximum. If the loop exited due to block going out of bounds without
a clean termination, set m_result to Error::BadData and return StepResult::Done
instead of proceeding to the ReadTitle phase.
The directory parser only knew about in-use (0x51/0x52/0x53) and never-allocated (0xa0) blocks, which is incomplete: deleting a file marks its blocks 0xa1/0xa2/0xa3 (a deleted first/middle/last), keeping the chain so it stays recoverable. - Deletion (and the replace path of writeFile) now flips block states to their deleted forms in place instead of wiping them, and allocation prefers never-allocated (0xa0) blocks over deleted ones so deletions survive as long as possible. - A block counts as in use only when it is reachable from a valid first-block head, so an orphaned middle/last block (such as one left by an interrupted write) is reclaimed as free rather than leaked. - The directory is committed in an order that survives power loss: a create writes the file's 0x51 head last and a delete cuts the head first, so a loss mid-commit can only orphan tail blocks (which read as free) and never leaves a referenced file pointing at a half-written chain. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mips/psyqo/src/memory-card-filesystem.cpp (1)
373-399: 🗄️ Data Integrity & Integration | 🟠 MajorUnbounded name length in directory entry construction can corrupt metadata fields.
The code at lines 373-399 computes
nameLenby scanningm_nameto its NUL terminator without any bounds check. Line 391 then copiesnameLenbytes into the name field atentry + c_offName. SinceListFiles(line 452) reads exactly 20 bytes from this position and expects the field width to be 20 bytes (matchingFileEntry.name[21]), anynameLen > 20will overflow into adjacent metadata fields. Additionally, if the caller passes an unterminated string viawriteFile()ordeleteFileBlocking(), the scan at line 374 will read out of bounds.The
writeFile()anddeleteFile()API entry points do not validate thenameparameter before storing it inm_name. Other code paths in this same file (line 137-139) demonstrate proper validation usingc_maxNameLength.Clamp
nameLentoc_maxNameLengthbefore the memcpy.🤖 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 `@src/mips/psyqo/src/memory-card-filesystem.cpp` around lines 373 - 399, The nameLen variable is computed by scanning m_name without bounds checking, and when copied to entry + c_offName using __builtin_memcpy, any nameLen exceeding the field width of 20 bytes will overflow into adjacent metadata fields. Additionally, if m_name is unterminated, the while loop will read out of bounds. After computing nameLen (and before the memcpy at line 391), clamp nameLen to c_maxNameLength to ensure the copied name does not overflow the field width, matching the validation pattern used elsewhere in this file.
🤖 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.
Outside diff comments:
In `@src/mips/psyqo/src/memory-card-filesystem.cpp`:
- Around line 373-399: The nameLen variable is computed by scanning m_name
without bounds checking, and when copied to entry + c_offName using
__builtin_memcpy, any nameLen exceeding the field width of 20 bytes will
overflow into adjacent metadata fields. Additionally, if m_name is unterminated,
the while loop will read out of bounds. After computing nameLen (and before the
memcpy at line 391), clamp nameLen to c_maxNameLength to ensure the copied name
does not overflow the field width, matching the validation pattern used
elsewhere in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77d2af96-3aad-4062-937a-f42214e4ccfc
📒 Files selected for processing (2)
src/mips/psyqo/memory-card-filesystem.hhsrc/mips/psyqo/src/memory-card-filesystem.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mips/psyqo/memory-card-filesystem.hh
eliasdaler
reviewed
Jun 26, 2026
|
|
||
| #include "common/util/sjis-encode-table.h" | ||
|
|
||
| // Unicode -> Shift-JIS encoding, the inverse of support/sjis_conv.cc's decoder. |
Contributor
There was a problem hiding this comment.
Maybe it would be good to add an EASTL version of sjis_conv.h/.cc to psyqo to make it possible to display/printf obtained filenames?
Right now you can't use it as-is, because sjis_conv.h/.cc use which is not available in freestanding mode.
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
2 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.
No description provided.