From ff37cbe18497886e185a79d91de4c17929bd2af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 14 May 2025 00:41:39 +0200 Subject: [PATCH] Minor memory safety fixes --- Core/Debugger/DisassemblyManager.cpp | 24 ++++++++++++++---------- Core/Debugger/DisassemblyManager.h | 2 +- Core/MIPS/MIPSInt.cpp | 4 +--- Core/MemMap.cpp | 2 +- Core/System.cpp | 1 + UI/DeveloperToolsScreen.cpp | 19 +++++++++++++++++++ UI/DeveloperToolsScreen.h | 2 ++ UI/EmuScreen.cpp | 1 - UI/ImDebugger/ImDisasmView.cpp | 10 +++++----- UI/ImDebugger/ImDisasmView.h | 2 +- Windows/Debugger/CtrlDisAsmView.cpp | 6 +++--- 11 files changed, 48 insertions(+), 25 deletions(-) diff --git a/Core/Debugger/DisassemblyManager.cpp b/Core/Debugger/DisassemblyManager.cpp index b14d3984e3..5e2a8f7891 100644 --- a/Core/Debugger/DisassemblyManager.cpp +++ b/Core/Debugger/DisassemblyManager.cpp @@ -1093,7 +1093,11 @@ bool DisassemblyComment::disassemble(u32 address, DisassemblyLineInfo &dest, boo return true; } -bool GetDisasmAddressText(u32 address, char* dest, bool abbreviateLabels, bool showData, bool displaySymbols) { +bool GetDisasmAddressText(u32 address, char *dest, size_t bufSize, bool abbreviateLabels, bool showData, bool displaySymbols) { + if (!Memory::IsValid4AlignedAddress(address)) { + snprintf(dest, bufSize, "(bad address!)"); + return true; + } if (displaySymbols) { const std::string addressSymbol = g_symbolMap->GetLabelString(address); if (!addressSymbol.empty()) { @@ -1109,15 +1113,15 @@ bool GetDisasmAddressText(u32 address, char* dest, bool abbreviateLabels, bool s *dest = 0; return true; } else { - sprintf(dest," %08X",address); + snprintf(dest, bufSize, " %08X",address); return false; } } else { if (showData) { - u32 encoding = Memory::IsValidAddress(address) ? Memory::Read_Instruction(address, true).encoding : 0; - sprintf(dest, "%08X %08X", address, encoding); + const u32 encoding = Memory::Read_Instruction(address, true).encoding; + snprintf(dest, bufSize, "%08X %08X", address, encoding); } else { - sprintf(dest, "%08X", address); + snprintf(dest, bufSize, "%08X", address); } return false; } @@ -1147,28 +1151,28 @@ std::string DisassembleRange(u32 start, u32 size, bool displaySymbols, MIPSDebug char addressText[64], buffer[512]; g_disassemblyManager.getLine(disAddress, displaySymbols, line, debugger); - bool isLabel = GetDisasmAddressText(disAddress, addressText, false, line.type == DISTYPE_OPCODE, displaySymbols); + bool isLabel = GetDisasmAddressText(disAddress, addressText, sizeof(addressText), false, line.type == DISTYPE_OPCODE, displaySymbols); if (isLabel) { if (!previousLabel) result += "\r\n"; - sprintf(buffer, "%s\r\n\r\n", addressText); + snprintf(buffer, sizeof(buffer), "%s\r\n\r\n", addressText); result += buffer; } else if (branchAddresses.find(disAddress) != branchAddresses.end()) { if (!previousLabel) result += "\r\n"; - sprintf(buffer, "pos_%08X:\r\n\r\n", disAddress); + snprintf(buffer, sizeof(buffer), "pos_%08X:\r\n\r\n", disAddress); result += buffer; } if (line.info.isBranch && !line.info.isBranchToRegister && g_symbolMap->GetLabelString(line.info.branchTarget).empty() && branchAddresses.find(line.info.branchTarget) != branchAddresses.end()) { - sprintf(buffer, "pos_%08X", line.info.branchTarget); + snprintf(buffer, sizeof(buffer), "pos_%08X", line.info.branchTarget); line.params = line.params.substr(0, line.params.find("0x")) + buffer; } - sprintf(buffer, "\t%s\t%s\r\n", line.name.c_str(), line.params.c_str()); + snprintf(buffer, sizeof(buffer), "\t%s\t%s\r\n", line.name.c_str(), line.params.c_str()); result += buffer; previousLabel = isLabel; disAddress += line.totalSize; diff --git a/Core/Debugger/DisassemblyManager.h b/Core/Debugger/DisassemblyManager.h index b2e028e070..ecfde48f81 100644 --- a/Core/Debugger/DisassemblyManager.h +++ b/Core/Debugger/DisassemblyManager.h @@ -228,4 +228,4 @@ bool isInInterval(u32 start, u32 size, u32 value); bool IsLikelyStringAt(uint32_t addr); std::string DisassembleRange(u32 start, u32 size, bool displaySymbols, MIPSDebugInterface *debugger); -bool GetDisasmAddressText(u32 address, char* dest, bool abbreviateLabels, bool showData, bool displaySymbols); +bool GetDisasmAddressText(u32 address, char *dest, size_t bufSize, bool abbreviateLabels, bool showData, bool displaySymbols); diff --git a/Core/MIPS/MIPSInt.cpp b/Core/MIPS/MIPSInt.cpp index 0896a169ab..e4a14d80f1 100644 --- a/Core/MIPS/MIPSInt.cpp +++ b/Core/MIPS/MIPSInt.cpp @@ -1045,7 +1045,7 @@ namespace MIPSInt void Int_Emuhack(MIPSOpcode op) { if (((op >> 24) & 3) != EMUOP_CALL_REPLACEMENT) { - _dbg_assert_msg_(false,"Trying to interpret emuhack instruction that can't be interpreted"); + _dbg_assert_msg_(false, "Trying to interpret emuhack instruction that can't be interpreted"); } // It's a replacement func! int index = op.encoding & 0xFFFFFF; @@ -1071,6 +1071,4 @@ namespace MIPSInt MIPSInterpret(Memory::Read_Instruction(PC, true)); } } - - } diff --git a/Core/MemMap.cpp b/Core/MemMap.cpp index 6cd14d01d2..0d0cfbe990 100644 --- a/Core/MemMap.cpp +++ b/Core/MemMap.cpp @@ -496,7 +496,7 @@ void Memset(const u32 _Address, const u8 _iValue, const u32 _iLength, const char // TODO: This mainly seems to be produced by GPUCommon::PerformMemorySet, called from // Replace_memset_jak(). Strangely, this managed to crash in Write_U8(). for (size_t i = 0; i < _iLength; i++) { - if (Memory::IsValidAddress(_Address + i)) { + if (Memory::IsValidAddress(_Address + (u32)i)) { WriteUnchecked_U8(_iValue, (u32)(_Address + i)); } } diff --git a/Core/System.cpp b/Core/System.cpp index 3c35f02848..4ca9d52a01 100644 --- a/Core/System.cpp +++ b/Core/System.cpp @@ -342,6 +342,7 @@ static bool CPU_Init(FileLoader *fileLoader, IdentifiedFileType type, std::strin auto sc = GetI18NCategory(I18NCat::SCREEN); + _dbg_assert_(!g_symbolMap); g_symbolMap = new SymbolMap(); MIPSAnalyst::Reset(); diff --git a/UI/DeveloperToolsScreen.cpp b/UI/DeveloperToolsScreen.cpp index 5509ee29d4..56d62d1ede 100644 --- a/UI/DeveloperToolsScreen.cpp +++ b/UI/DeveloperToolsScreen.cpp @@ -222,6 +222,13 @@ void DeveloperToolsScreen::CreateTestsTab(UI::LinearLayout *list) { if (g_Config.iGPUBackend == (int)GPUBackend::VULKAN || g_Config.iGPUBackend == (int)GPUBackend::OPENGL) { list->Add(new Choice(dev->T("GPU Driver Test")))->OnClick.Handle(this, &DeveloperToolsScreen::OnGPUDriverTest); } + + auto memmapTest = list->Add(new Choice(dev->T("Memory map test"))); + memmapTest->OnClick.Add([this](UI::EventParams &e) { + MemoryMapTest(); + return UI::EVENT_DONE; + }); + memmapTest->SetEnabled(PSP_IsInited()); } void DeveloperToolsScreen::CreateDumpFileTab(UI::LinearLayout *list) { @@ -613,6 +620,18 @@ void DeveloperToolsScreen::update() { canAllowDebugger_ = !WebServerStopping(WebServerFlags::DEBUGGER); } +void DeveloperToolsScreen::MemoryMapTest() { + int sum = 0; + for (uint64_t addr = 0; addr < 0x100000000ULL; addr += 0x1000) { + const u32 addr32 = (u32)addr; + if (Memory::IsValidAddress(addr32)) { + sum += Memory::ReadUnchecked_U32(addr32); + } + } + // Just to force the compiler to do things properly. + INFO_LOG(Log::JIT, "Total sum: %08x", sum); +} + static bool RunMemstickTest(std::string *error) { Path testRoot = GetSysDirectory(PSPDirectories::DIRECTORY_CACHE) / "test"; diff --git a/UI/DeveloperToolsScreen.h b/UI/DeveloperToolsScreen.h index 4aaaa6b47d..bb1fd67101 100644 --- a/UI/DeveloperToolsScreen.h +++ b/UI/DeveloperToolsScreen.h @@ -42,6 +42,8 @@ private: UI::EventReturn OnTouchscreenTest(UI::EventParams &e); UI::EventReturn OnCopyStatesToRoot(UI::EventParams &e); + void MemoryMapTest(); + bool allowDebugger_ = false; bool canAllowDebugger_ = true; enum class HasIni { diff --git a/UI/EmuScreen.cpp b/UI/EmuScreen.cpp index daf1dc58d7..6bab13fc36 100644 --- a/UI/EmuScreen.cpp +++ b/UI/EmuScreen.cpp @@ -476,7 +476,6 @@ void EmuScreen::dialogFinished(const Screen *dialog, DialogResult result) { // DR_CANCEL/DR_BACK means clicked on "continue", DR_OK means clicked on "back to menu", // DR_YES means a message sent to PauseMenu by System_PostUIMessage. if ((result == DR_OK || quit_) && !bootPending_) { - _dbg_assert_(!bootPending_); screenManager()->switchScreen(new MainScreen()); quit_ = false; } else { diff --git a/UI/ImDebugger/ImDisasmView.cpp b/UI/ImDebugger/ImDisasmView.cpp index e0153e92d2..e7d7438856 100644 --- a/UI/ImDebugger/ImDisasmView.cpp +++ b/UI/ImDebugger/ImDisasmView.cpp @@ -51,11 +51,11 @@ static ImColor scaleColor(ImColor color, float factor) { return color; } -bool ImDisasmView::getDisasmAddressText(u32 address, char* dest, bool abbreviateLabels, bool showData) { +bool ImDisasmView::getDisasmAddressText(u32 address, char *dest, size_t bufSize, bool abbreviateLabels, bool showData) { if (PSP_GetBootState() != BootState::Complete) return false; - return GetDisasmAddressText(address, dest, abbreviateLabels, showData, displaySymbols_); + return GetDisasmAddressText(address, dest, bufSize, abbreviateLabels, showData, displaySymbols_); } void ImDisasmView::assembleOpcode(u32 address, const std::string &defaultText) { @@ -358,7 +358,7 @@ void ImDisasmView::Draw(ImDrawList *drawList, ImControl &control) { } char addressText[64]; - getDisasmAddressText(address, addressText, true, line.type == DISTYPE_OPCODE); + getDisasmAddressText(address, addressText, sizeof(addressText), true, line.type == DISTYPE_OPCODE); drawList->AddText(ImVec2(bounds.x + pixelPositions_.addressStart, bounds.y + rowY1 + 2), textColor, addressText); if (isInInterval(address, line.totalSize, pc)) { @@ -1013,7 +1013,7 @@ void ImDisasmView::SearchNext(bool forward) { g_disassemblyManager.getLine(searchAddress, displaySymbols_, lineInfo, debugger_); char addressText[64]; - getDisasmAddressText(searchAddress, addressText, true, lineInfo.type == DISTYPE_OPCODE); + getDisasmAddressText(searchAddress, addressText, sizeof(addressText), true, lineInfo.type == DISTYPE_OPCODE); const char* opcode = lineInfo.name.c_str(); const char* arguments = lineInfo.params.c_str(); @@ -1075,7 +1075,7 @@ std::string ImDisasmView::disassembleRange(u32 start, u32 size) { char addressText[64], buffer[512]; g_disassemblyManager.getLine(disAddress, displaySymbols_, line, debugger_); - bool isLabel = getDisasmAddressText(disAddress, addressText, false, line.type == DISTYPE_OPCODE); + bool isLabel = getDisasmAddressText(disAddress, addressText, sizeof(addressText), false, line.type == DISTYPE_OPCODE); if (isLabel) { if (!previousLabel) result += "\r\n"; diff --git a/UI/ImDebugger/ImDisasmView.h b/UI/ImDebugger/ImDisasmView.h index b092c1df69..f70f7291f4 100644 --- a/UI/ImDebugger/ImDisasmView.h +++ b/UI/ImDebugger/ImDisasmView.h @@ -130,7 +130,7 @@ private: void disassembleToFile(); void FollowBranch(); void calculatePixelPositions(); - bool getDisasmAddressText(u32 address, char* dest, bool abbreviateLabels, bool showData); + bool getDisasmAddressText(u32 address, char *dest, size_t bufSize, bool abbreviateLabels, bool showData); void updateStatusBarText(); void drawBranchLine(ImDrawList *list, Bounds rc, std::map &addressPositions, const BranchLine &line); void CopyInstructions(u32 startAddr, u32 endAddr, CopyInstructionsMode mode); diff --git a/Windows/Debugger/CtrlDisAsmView.cpp b/Windows/Debugger/CtrlDisAsmView.cpp index 0edb83857d..2865dee776 100644 --- a/Windows/Debugger/CtrlDisAsmView.cpp +++ b/Windows/Debugger/CtrlDisAsmView.cpp @@ -529,8 +529,8 @@ void CtrlDisAsmView::onPaint(WPARAM wParam, LPARAM lParam) SetTextColor(hdc,textColor); char addressText[64]; - GetDisasmAddressText(address,addressText,true,line.type == DISTYPE_OPCODE, displaySymbols); - TextOutA(hdc,pixelPositions.addressStart,rowY1+2,addressText,(int)strlen(addressText)); + GetDisasmAddressText(address, addressText, sizeof(addressText), true, line.type == DISTYPE_OPCODE, displaySymbols); + TextOutA(hdc, pixelPositions.addressStart, rowY1+2, addressText, (int)strlen(addressText)); if (isInInterval(address,line.totalSize,debugger->GetPC())) { @@ -1252,7 +1252,7 @@ void CtrlDisAsmView::search(bool continueSearch) g_disassemblyManager.getLine(searchAddress,displaySymbols,lineInfo, debugger); char addressText[64]; - GetDisasmAddressText(searchAddress,addressText,true,lineInfo.type == DISTYPE_OPCODE, displaySymbols); + GetDisasmAddressText(searchAddress, addressText, sizeof(addressText), true, lineInfo.type == DISTYPE_OPCODE, displaySymbols); const char* opcode = lineInfo.name.c_str(); const char* arguments = lineInfo.params.c_str();