mirror of
https://github.com/hrydgard/ppsspp.git
synced 2026-10-01 14:58:14 +00:00
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.
This commit is contained in:
1 parent
3bd9e23f91
commit
98e8ffe7cd
4 files changed
+56
-8
No files matched your search
+12
-4
@@ -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;
|
||||
|
||||
@@ -140,9 +140,17 @@ void CPUInfo::Detect()
|
||||
#else // __linux__
|
||||
LoongArchCPUInfoParser parser;
|
||||
num_cores = parser.ProcessorCount();
|
||||
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);
|
||||
|
||||
@@ -190,9 +190,17 @@ void CPUInfo::Detect()
|
||||
#else // __linux__
|
||||
RiscVCPUInfoParser parser;
|
||||
num_cores = parser.ProcessorCount();
|
||||
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());
|
||||
|
||||
|
||||
@@ -1958,6 +1958,22 @@ static const CopyCandidate *GetBestCopyCandidate(const TinySet<CopyCandidate, 4>
|
||||
// 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,6 +2214,8 @@ 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;
|
||||
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");
|
||||
@@ -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<int>(dstX * dstXFactor), dstY, srcBase, dstRect.vfb->fb_format, static_cast<int>(srcStride * dstXFactor), static_cast<int>(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<int>(dstX * dstXFactor), dstY, srcBase, dstRect.vfb->fb_format, static_cast<int>(srcStride * dstXFactor), static_cast<int>(dstRect.w_bytes / bpp * dstXFactor), safeH, RASTER_COLOR, "BlockTransferCopy_DrawPixels");
|
||||
SetColorUpdated(dstRect.vfb, skipDrawReason);
|
||||
RebindFramebuffer("RebindFramebuffer - NotifyBlockTransferAfter");
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user