prevent buffer overflow in directDMARead

[ZumiKua]

In the directDMARead function, when the requested transferSize exceeds the available data in m_readFifo, the code attempts to pad the remainder of the destination buffer with m_dataRet.

Due to a missing transferSize-- operation within the while loop, the dest pointer continues to increment indefinitely (or until a segmentation fault occurs), causing a heap buffer overflow.

This fix resolves the crash observed when running the original Japanese version of Planet Laika, which triggers this specific DMA padding scenario.

[coderabbitai]

📝 Walkthrough

Walkthrough

Modified the directDMARead function in the GPU core to decrement transferSize during each loop iteration. Previously, the loop may not have properly terminated by reducing the transfer size, affecting DMA read completion logic.

Changes

Cohort / File(s)	Summary
GPU DMA Read Fix src/core/gpu.cc	Added transferSize decrement in directDMARead loop to properly control iteration count and ensure DMA transfer completes correctly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A loop that spun round and round,
Never quite finishing its ground—
But with one line's gentle care,
The transfer now ends fair and square! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'prevent buffer overflow in directDMARead' clearly and specifically summarizes the main change—adding a missing transferSize decrement to prevent a buffer overflow vulnerability in the directDMARead function.
Description check	✅ Passed	The description is directly related to the changeset, explaining the buffer overflow issue, the specific problem (missing transferSize--), and how the fix resolves a crash in a specific game.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/core/gpu.cc (1)

697-701: Harden transfer accounting against FIFO/request mismatch.

At Line 699, transferSize is reduced by the full pre-read FIFO size, not by the actual requested read length. If FIFO words ever exceed the request, transferSize can become negative before Line 701. Consider clamping consumed words and looping while transferSize > 0.

Suggested hardening patch

 void PCSX::GPU::directDMARead(uint32_t *dest, int transferSize, uint32_t hwAddr) {
     auto size = m_readFifo->size();
-    m_readFifo->read(dest, transferSize * 4);
-    transferSize -= size / 4;
-    dest += size / 4;
-    while (transferSize != 0) {
+    int wordsRead = static_cast<int>(size / 4);
+    if (wordsRead > transferSize) wordsRead = transferSize;
+    m_readFifo->read(dest, wordsRead * 4);
+    transferSize -= wordsRead;
+    dest += wordsRead;
+    while (transferSize > 0) {
         *dest++ = m_dataRet;
         transferSize--;
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/gpu.cc` around lines 697 - 701, The current loop subtracts the
entire pre-read FIFO word count (size/4) from transferSize which can make
transferSize negative if the FIFO contains more words than requested; update the
logic around m_readFifo->read(dest, ...) in function(s) using m_readFifo,
transferSize and dest to clamp the consumed word count to the actual requested
amount: compute consumed = std::min(static_cast<size_t>(transferSize), size /
4), call m_readFifo->read(dest, consumed * 4), decrement transferSize by
consumed, advance dest by consumed, and iterate while (transferSize > 0) so you
never subtract more than requested and never produce a negative transferSize.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/core/gpu.cc`:
- Around line 697-701: The current loop subtracts the entire pre-read FIFO word
count (size/4) from transferSize which can make transferSize negative if the
FIFO contains more words than requested; update the logic around
m_readFifo->read(dest, ...) in function(s) using m_readFifo, transferSize and
dest to clamp the consumed word count to the actual requested amount: compute
consumed = std::min(static_cast<size_t>(transferSize), size / 4), call
m_readFifo->read(dest, consumed * 4), decrement transferSize by consumed,
advance dest by consumed, and iterate while (transferSize > 0) so you never
subtract more than requested and never produce a negative transferSize.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 94b00142-175e-4889-abcc-49af674fa71f

📥 Commits

Reviewing files that changed from the base of the PR and between 3a856d0 and 1584891.

📒 Files selected for processing (1)

• src/core/gpu.cc