From 9cebfc31b39f9f0585f33f5857acd241bcfd95af Mon Sep 17 00:00:00 2001 From: "Unknown W. Brackets" Date: Tue, 11 Apr 2023 20:58:59 -0700 Subject: [PATCH] Debugger: Avoid unaligned reads in expressions. Potentially, a watch or break condition could crash if it was unaligned between mirrors. This might happen if it's not the condition you wanted, especially. Play it safe. --- Core/MIPS/MIPSDebugInterface.cpp | 18 +++++++++++------- Core/MemMap.h | 2 +- GPU/Common/GPUDebugInterface.cpp | 11 +++++++---- Windows/Debugger/CtrlDisAsmView.cpp | 2 ++ Windows/Debugger/CtrlMemView.cpp | 6 ++++-- Windows/Debugger/Debugger_Lists.cpp | 25 ++++++++++++++----------- 6 files changed, 39 insertions(+), 25 deletions(-) diff --git a/Core/MIPS/MIPSDebugInterface.cpp b/Core/MIPS/MIPSDebugInterface.cpp index bffdee83b6..ab415a384a 100644 --- a/Core/MIPS/MIPSDebugInterface.cpp +++ b/Core/MIPS/MIPSDebugInterface.cpp @@ -162,17 +162,20 @@ public: bool getMemoryValue(uint32_t address, int size, uint32_t& dest, char* error) override { // We allow, but ignore, bad access. // If we didn't, log/condition statements that reference registers couldn't be configured. - bool valid = Memory::IsValidRange(address, size); + uint32_t valid = Memory::ValidSize(address, size); + uint8_t buf[4]{}; + if (valid != 0) + memcpy(buf, Memory::GetPointerUnchecked(address), valid); switch (size) { case 1: - dest = valid ? Memory::Read_U8(address) : 0; + dest = buf[0]; return true; case 2: - dest = valid ? Memory::Read_U16(address) : 0; + dest = (buf[1] << 8) | buf[0]; return true; case 4: - dest = valid ? Memory::Read_U32(address) : 0; + dest = (buf[3] << 24) | (buf[2] << 16) | (buf[1] << 8) | buf[0]; return true; } @@ -196,9 +199,10 @@ const char *MIPSDebugInterface::disasm(unsigned int address, unsigned int align) return mojs; } -unsigned int MIPSDebugInterface::readMemory(unsigned int address) -{ - return Memory::Read_Instruction(address).encoding; +unsigned int MIPSDebugInterface::readMemory(unsigned int address) { + if (Memory::IsValidRange(address, 4)) + return Memory::ReadUnchecked_Instruction(address).encoding; + return 0; } bool MIPSDebugInterface::isAlive() diff --git a/Core/MemMap.h b/Core/MemMap.h index b2edbe0290..7a0af46257 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -331,7 +331,7 @@ inline u32 ValidSize(const u32 address, const u32 requested_size) { } inline bool IsValidRange(const u32 address, const u32 size) { - return IsValidAddress(address) && ValidSize(address, size) == size; + return ValidSize(address, size) == size; } } // namespace Memory diff --git a/GPU/Common/GPUDebugInterface.cpp b/GPU/Common/GPUDebugInterface.cpp index 618c958a1f..49b6725743 100644 --- a/GPU/Common/GPUDebugInterface.cpp +++ b/GPU/Common/GPUDebugInterface.cpp @@ -929,17 +929,20 @@ ExpressionType GEExpressionFunctions::getFieldType(GECmdFormat fmt, GECmdField f bool GEExpressionFunctions::getMemoryValue(uint32_t address, int size, uint32_t &dest, char *error) { // We allow, but ignore, bad access. // If we didn't, log/condition statements that reference registers couldn't be configured. - bool valid = Memory::IsValidRange(address, size); + uint32_t valid = Memory::ValidSize(address, size); + uint8_t buf[4]{}; + if (valid != 0) + memcpy(buf, Memory::GetPointerUnchecked(address), valid); switch (size) { case 1: - dest = valid ? Memory::Read_U8(address) : 0; + dest = buf[0]; return true; case 2: - dest = valid ? Memory::Read_U16(address) : 0; + dest = (buf[1] << 8) | buf[0]; return true; case 4: - dest = valid ? Memory::Read_U32(address) : 0; + dest = (buf[3] << 24) | (buf[2] << 16) | (buf[1] << 8) | buf[0]; return true; } diff --git a/Windows/Debugger/CtrlDisAsmView.cpp b/Windows/Debugger/CtrlDisAsmView.cpp index fa4b2630f2..ee637d6890 100644 --- a/Windows/Debugger/CtrlDisAsmView.cpp +++ b/Windows/Debugger/CtrlDisAsmView.cpp @@ -909,6 +909,8 @@ void CtrlDisAsmView::onMouseDown(WPARAM wParam, LPARAM lParam, int button) } void CtrlDisAsmView::CopyInstructions(u32 startAddr, u32 endAddr, CopyInstructionsMode mode) { + _assert_msg_((startAddr & 3) == 0, "readMemory() can't handle unaligned reads"); + if (mode != CopyInstructionsMode::DISASM) { int instructionSize = debugger->getInstructionSize(0); int count = (endAddr - startAddr) / instructionSize; diff --git a/Windows/Debugger/CtrlMemView.cpp b/Windows/Debugger/CtrlMemView.cpp index 16f0a12cc2..587d312f17 100644 --- a/Windows/Debugger/CtrlMemView.cpp +++ b/Windows/Debugger/CtrlMemView.cpp @@ -218,6 +218,8 @@ void CtrlMemView::onPaint(WPARAM wParam, LPARAM lParam) { } }; + _assert_msg_(((windowStart_ | rowSize_) & 3) == 0, "readMemory() can't handle unaligned reads"); + // draw one extra row that may be partially visible for (int i = 0; i < visibleRows_ + 1; i++) { int rowY = rowHeight_ * i; @@ -236,8 +238,8 @@ void CtrlMemView::onPaint(WPARAM wParam, LPARAM lParam) { uint32_t words[4]; uint8_t bytes[16]; } memory; - bool valid = debugger_ != nullptr && debugger_->isAlive() && Memory::IsValidAddress(address); - for (int i = 0; valid && i < 4; ++i) { + int valid = debugger_ != nullptr && debugger_->isAlive() ? Memory::ValidSize(address, 16) / 4 : 0; + for (int i = 0; i < valid; ++i) { memory.words[i] = debugger_->readMemory(address + i * 4); } diff --git a/Windows/Debugger/Debugger_Lists.cpp b/Windows/Debugger/Debugger_Lists.cpp index 7808c5f6c7..8e65ea2770 100644 --- a/Windows/Debugger/Debugger_Lists.cpp +++ b/Windows/Debugger/Debugger_Lists.cpp @@ -887,20 +887,23 @@ bool CtrlWatchList::WindowMessage(UINT msg, WPARAM wParam, LPARAM lParam, LRESUL } void CtrlWatchList::GetColumnText(wchar_t *dest, int row, int col) { - uint32_t value = 0; - float valuef = 0.0f; + const auto &watch = watches_[row]; switch (col) { case WL_NAME: - wcsncpy(dest, ConvertUTF8ToWString(watches_[row].name).c_str(), 255); + wcsncpy(dest, ConvertUTF8ToWString(watch.name).c_str(), 255); dest[255] = 0; break; case WL_EXPRESSION: - wcsncpy(dest, ConvertUTF8ToWString(watches_[row].originalExpression).c_str(), 255); + wcsncpy(dest, ConvertUTF8ToWString(watch.originalExpression).c_str(), 255); dest[255] = 0; break; case WL_VALUE: - if (cpu_->parseExpression(watches_[row].expression, value)) { - switch (watches_[row].format) { + if (watch.evaluateFailed) { + wcscpy(dest, L"(failed to evaluate)"); + } else { + const uint32_t &value = watch.currentValue; + float valuef = 0.0f; + switch (watch.format) { case WatchFormat::HEX: wsprintf(dest, L"0x%08X", value); break; @@ -912,14 +915,14 @@ void CtrlWatchList::GetColumnText(wchar_t *dest, int row, int col) { swprintf_s(dest, 255, L"%f", valuef); break; case WatchFormat::STR: - if (Memory::IsValidAddress(value)) - swprintf_s(dest, 255, L"%.255S", Memory::GetCharPointer(value)); - else + if (Memory::IsValidAddress(value)) { + uint32_t len = Memory::ValidSize(value, 255); + swprintf_s(dest, 255, L"%.*S", len, Memory::GetCharPointer(value)); + } else { wsprintf(dest, L"(0x%08X)", value); + } break; } - } else { - wcscpy(dest, L"(failed to evaluate)"); } break; }