From 98e8ffe7cd30b9abbfead6a0e83c436973093cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 31 Aug 2026 12:15:50 +0200 Subject: [PATCH] Check framebuffer copy sources, and fix two easy crashes The three framebuffer upload paths took Memory::GetPointerUnchecked() on a GE-supplied source address and then read height rows of it, without ever checking that span was mapped. Only the destination was validated (and DoBlockTransfer's own memcpy is carefully guarded, so the intent was clearly there). A copy whose source starts near the end of RAM walks straight off the end of the view. Clamp the row count to what's actually mapped, and warn when we do. GhidraClient dereferenced getArray()->value for both "symbols" and "types" without a null check, and the getTag() test underneath could never catch it - getArray() has already filtered by tag, so it returns either a JSON_ARRAY node or nullptr. Any HTTP 200 that parses as JSON but isn't the shape we expect - {}, a bare array, an incompatible ghidra-rest-api, or the host/port pointed at some other JSON service - crashed the worker thread. FetchTypes() runs first, so that's the one you'd hit. RiscV and LoongArch CPU detection divided TotalLogicalCount() by ProcessorCount() before checking it. ProcessorCount() returns 0 whenever /proc/cpuinfo can't be read or doesn't parse, which is SIGFPE during static init of the cpu_info global - before anything could handle it. The existing <= 0 guard sat after the division. 314 pspautotests pass; frametests show the same 3 pre-existing failures as master. --- Common/GhidraClient.cpp | 16 +++++++++--- Common/LoongArchCPUDetect.cpp | 12 +++++++-- Common/RiscVCPUDetect.cpp | 12 +++++++-- GPU/Common/FramebufferManagerCommon.cpp | 34 +++++++++++++++++++++---- 4 files changed, 61 insertions(+), 13 deletions(-) diff --git a/Common/GhidraClient.cpp b/Common/GhidraClient.cpp index 26a95ff796..9bc40f500a 100644 --- a/Common/GhidraClient.cpp +++ b/Common/GhidraClient.cpp @@ -79,11 +79,15 @@ bool GhidraClient::FetchSymbols() { pendingResult_.error = "symbols parsing error"; return false; } - const JsonValue entries = reader.root().getArray("symbols")->value; - if (entries.getTag() != JSON_ARRAY) { + const JsonNode *entriesNode = reader.root().getArray("symbols"); + if (!entriesNode) { + // Null for a missing key, a non-array value, or a root that isn't an object at all - + // so any JSON that parses but isn't what we expect. The getTag() check below it could + // never catch that, since getArray() already filtered by tag. pendingResult_.error = "symbols is not an array"; return false; } + const JsonValue entries = entriesNode->value; for (const auto pEntry : entries) { JsonGet entry = pEntry->value; @@ -109,11 +113,15 @@ bool GhidraClient::FetchTypes() { pendingResult_.error = "types parsing error"; return false; } - const JsonValue entries = reader.root().getArray("types")->value; - if (entries.getTag() != JSON_ARRAY) { + const JsonNode *entriesNode = reader.root().getArray("types"); + if (!entriesNode) { + // Null for a missing key, a non-array value, or a root that isn't an object at all - + // so any JSON that parses but isn't what we expect. The getTag() check below it could + // never catch that, since getArray() already filtered by tag. pendingResult_.error = "types is not an array"; return false; } + const JsonValue entries = entriesNode->value; for (const auto pEntry : entries) { const JsonGet entry = pEntry->value; diff --git a/Common/LoongArchCPUDetect.cpp b/Common/LoongArchCPUDetect.cpp index 0e4d7e6909..ac418419e8 100644 --- a/Common/LoongArchCPUDetect.cpp +++ b/Common/LoongArchCPUDetect.cpp @@ -140,9 +140,17 @@ void CPUInfo::Detect() #else // __linux__ LoongArchCPUInfoParser parser; num_cores = parser.ProcessorCount(); - logical_cpu_count = parser.TotalLogicalCount() / num_cores; - if (logical_cpu_count <= 0) + if (num_cores <= 0) { + // ProcessorCount() is 0 when /proc/cpuinfo couldn't be read or didn't parse - dividing + // by it here raised SIGFPE during static init of cpu_info, before any handler exists. + // The guard below this was too late to help. + num_cores = 1; logical_cpu_count = 1; + } else { + logical_cpu_count = parser.TotalLogicalCount() / num_cores; + if (logical_cpu_count <= 0) + logical_cpu_count = 1; + } #endif unsigned long hwcap = getauxval(AT_HWCAP); diff --git a/Common/RiscVCPUDetect.cpp b/Common/RiscVCPUDetect.cpp index 53670c1f1e..27c2b31815 100644 --- a/Common/RiscVCPUDetect.cpp +++ b/Common/RiscVCPUDetect.cpp @@ -190,9 +190,17 @@ void CPUInfo::Detect() #else // __linux__ RiscVCPUInfoParser parser; num_cores = parser.ProcessorCount(); - logical_cpu_count = parser.TotalLogicalCount() / num_cores; - if (logical_cpu_count <= 0) + if (num_cores <= 0) { + // ProcessorCount() is 0 when /proc/cpuinfo couldn't be read or didn't parse - dividing + // by it here raised SIGFPE during static init of cpu_info, before any handler exists. + // The guard below this was too late to help. + num_cores = 1; logical_cpu_count = 1; + } else { + logical_cpu_count = parser.TotalLogicalCount() / num_cores; + if (logical_cpu_count <= 0) + logical_cpu_count = 1; + } truncate_cpy(cpu_string, parser.ISAString()); diff --git a/GPU/Common/FramebufferManagerCommon.cpp b/GPU/Common/FramebufferManagerCommon.cpp index d68c428a07..cc921feb6d 100644 --- a/GPU/Common/FramebufferManagerCommon.cpp +++ b/GPU/Common/FramebufferManagerCommon.cpp @@ -1958,6 +1958,22 @@ static const CopyCandidate *GetBestCopyCandidate(const TinySet // NOTE: This is very tricky because there's no information about color depth here, so we'll have to make guesses // about what underlying framebuffer is the most likely to be the relevant ones. For src, we can probably prioritize recent // ones. For dst, less clear. +// Source address, stride and height reach us as independent GE registers, so the span a copy is +// about to read has to be checked against what's actually mapped. The upload paths below took +// an unchecked pointer and trusted the height. Returns how many rows are safe to read. +static int ClampCopyRows(u32 srcAddr, int strideInBytes, int height, const char *tag) { + if (height <= 0 || strideInBytes <= 0) { + return 0; + } + const u32 needed = (u32)height * (u32)strideInBytes; + if (Memory::IsValidRange(srcAddr, needed)) { + return height; + } + const int rows = (int)(Memory::MaxSizeAtAddress(srcAddr) / (u32)strideInBytes); + WARN_LOG_N_TIMES(fbcopyrange, 5, Log::FrameBuf, "%s: source %08x only has %d of %d rows mapped", tag, srcAddr, rows, height); + return std::min(rows, height); +} + bool FramebufferManagerCommon::NotifyFramebufferCopy(u32 src, u32 dst, int size, GPUCopyFlag flags, u32 skipDrawReason) { if (size == 0) { return false; @@ -2198,7 +2214,9 @@ bool FramebufferManagerCommon::NotifyFramebufferCopy(u32 src, u32 dst, int size, GEBufferFormat srcFormat = channel == RASTER_DEPTH ? GE_FORMAT_DEPTH16 : dstBuffer->fb_format; // TODO: srcStride here looks suspicious! Actually the whole calculation does... int srcStride = channel == RASTER_DEPTH ? dstBuffer->z_stride : dstBuffer->fb_stride; - DrawPixels(dstBuffer, 0, dstY, srcBase, srcFormat, srcStride, dstBuffer->width, dstH, channel, "MemcpyFboUpload_DrawPixels"); + dstH = ClampCopyRows(src, srcStride * BufferFormatBytesPerPixel(srcFormat), dstH, "MemcpyFboUpload"); + if (dstH > 0) + DrawPixels(dstBuffer, 0, dstY, srcBase, srcFormat, srcStride, dstBuffer->width, dstH, channel, "MemcpyFboUpload_DrawPixels"); SetColorUpdated(dstBuffer, skipDrawReason); RebindFramebuffer("RebindFramebuffer - Memcpy fbo upload"); // This is a memcpy, let's still copy just in case. @@ -2781,8 +2799,11 @@ bool FramebufferManagerCommon::NotifyBlockTransferBefore(u32 dstBasePtr, int dst if (dstRect.channel == RASTER_DEPTH) { WARN_LOG_ONCE(btud, Log::G3D, "Block transfer upload %08x -> %08x (%dx%d %d,%d bpp=%d %s)", srcBasePtr, dstBasePtr, width, height, dstX, dstY, bpp, RasterChannelToString(dstRect.channel)); FlushBeforeCopy(); - const u8 *srcBase = Memory::GetPointerUnchecked(srcBasePtr) + (srcX + srcY * srcStride) * bpp; - DrawPixels(dstRect.vfb, dstX, dstY, srcBase, dstRect.vfb->Format(dstRect.channel), srcStride * bpp / 2, (int)(dstRect.w_bytes / 2), dstRect.h, dstRect.channel, "BlockTransferCopy_DrawPixelsDepth"); + const u32 srcStart = srcBasePtr + (srcX + srcY * srcStride) * bpp; + const u8 *srcBase = Memory::GetPointerUnchecked(srcStart); + const int safeH = ClampCopyRows(srcStart, srcStride * bpp, dstRect.h, "BlockTransferUploadDepth"); + if (safeH > 0) + DrawPixels(dstRect.vfb, dstX, dstY, srcBase, dstRect.vfb->Format(dstRect.channel), srcStride * bpp / 2, (int)(dstRect.w_bytes / 2), safeH, dstRect.channel, "BlockTransferCopy_DrawPixelsDepth"); RebindFramebuffer("RebindFramebuffer - UploadDepth"); return true; } @@ -2864,7 +2885,8 @@ void FramebufferManagerCommon::NotifyBlockTransferAfter(u32 dstBasePtr, int dstS if (dstBuffer && !srcBuffer) { WARN_LOG_ONCE(btu, Log::G3D, "Block transfer upload %08x -> %08x (%dx%d %d,%d bpp=%d)", srcBasePtr, dstBasePtr, width, height, dstX, dstY, bpp); FlushBeforeCopy(); - const u8 *srcBase = Memory::GetPointerUnchecked(srcBasePtr) + (srcX + srcY * srcStride) * bpp; + const u32 srcStart = srcBasePtr + (srcX + srcY * srcStride) * bpp; + const u8 *srcBase = Memory::GetPointerUnchecked(srcStart); int dstBpp = BufferFormatBytesPerPixel(dstRect.vfb->fb_format); float dstXFactor = (float)bpp / dstBpp; @@ -2880,7 +2902,9 @@ void FramebufferManagerCommon::NotifyBlockTransferAfter(u32 dstBasePtr, int dstS // Resizing may change the viewport/etc. gstate_c.Dirty(DIRTY_VIEWPORTSCISSOR_STATE); } - DrawPixels(dstRect.vfb, static_cast(dstX * dstXFactor), dstY, srcBase, dstRect.vfb->fb_format, static_cast(srcStride * dstXFactor), static_cast(dstRect.w_bytes / bpp * dstXFactor), dstRect.h, RASTER_COLOR, "BlockTransferCopy_DrawPixels"); + const int safeH = ClampCopyRows(srcStart, srcStride * bpp, dstRect.h, "BlockTransferUpload"); + if (safeH > 0) + DrawPixels(dstRect.vfb, static_cast(dstX * dstXFactor), dstY, srcBase, dstRect.vfb->fb_format, static_cast(srcStride * dstXFactor), static_cast(dstRect.w_bytes / bpp * dstXFactor), safeH, RASTER_COLOR, "BlockTransferCopy_DrawPixels"); SetColorUpdated(dstRect.vfb, skipDrawReason); RebindFramebuffer("RebindFramebuffer - NotifyBlockTransferAfter"); }