From 76afb2a8d5eaed855a92b6f13c0d343ab951474c Mon Sep 17 00:00:00 2001 From: "Unknown W. Brackets" Date: Sat, 25 Jan 2014 21:40:23 -0800 Subject: [PATCH] Avoid returning points from the symbol map. Now that it uses a lookup, this is even more dangerous. But, the maps could be reordered while it's trying to print the pointer and cause that data to become invalid. This should be safe from race conditions. --- Core/Debugger/Breakpoints.cpp | 2 +- Core/Debugger/DebugInterface.h | 3 ++- Core/Debugger/DisassemblyManager.cpp | 28 +++++++++++++------------- Core/Debugger/SymbolMap.cpp | 13 +++++++++--- Core/Debugger/SymbolMap.h | 7 ++++--- Core/MIPS/MIPSAnalyst.cpp | 17 ++++++++++++---- Core/MIPS/MIPSDebugInterface.cpp | 2 +- Core/MIPS/MIPSDebugInterface.h | 3 ++- Core/MIPS/x86/Jit.cpp | 6 +++--- Windows/Debugger/CtrlDisAsmView.cpp | 30 ++++++++++++++-------------- Windows/Debugger/Debugger_Lists.cpp | 8 ++++---- 11 files changed, 69 insertions(+), 50 deletions(-) diff --git a/Core/Debugger/Breakpoints.cpp b/Core/Debugger/Breakpoints.cpp index fa416b8c3e..7ea1cf1f28 100644 --- a/Core/Debugger/Breakpoints.cpp +++ b/Core/Debugger/Breakpoints.cpp @@ -41,7 +41,7 @@ void MemCheck::Action(u32 addr, bool write, int size, u32 pc) ++numHits; if (result & MEMCHECK_LOG) - NOTICE_LOG(MEMMAP, "CHK %s%i at %08x (%s), PC=%08x (%s)", write ? "Write" : "Read", size * 8, addr, symbolMap.GetDescription(addr), pc, symbolMap.GetDescription(pc)); + NOTICE_LOG(MEMMAP, "CHK %s%i at %08x (%s), PC=%08x (%s)", write ? "Write" : "Read", size * 8, addr, symbolMap.GetDescription(addr).c_str(), pc, symbolMap.GetDescription(pc)); if (result & MEMCHECK_BREAK) { Core_EnableStepping(true); diff --git a/Core/Debugger/DebugInterface.h b/Core/Debugger/DebugInterface.h index 56a89529df..e356194269 100644 --- a/Core/Debugger/DebugInterface.h +++ b/Core/Debugger/DebugInterface.h @@ -16,6 +16,7 @@ // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. #pragma once +#include #include #include "Common/CommonTypes.h" #include "native/math/expression_parser.h" @@ -45,7 +46,7 @@ public: virtual void step() {} virtual void runToBreakpoint() {} virtual int getColor(unsigned int address){return 0xFFFFFFFF;} - virtual const char *getDescription(unsigned int address) {return "";} + virtual std::string getDescription(unsigned int address) {return "";} virtual bool initExpression(const char* exp, PostfixExpression& dest) { return false; }; virtual bool parseExpression(PostfixExpression& exp, u32& dest) { return false; }; diff --git a/Core/Debugger/DisassemblyManager.cpp b/Core/Debugger/DisassemblyManager.cpp index 57a0f55ac2..96e0d46756 100644 --- a/Core/Debugger/DisassemblyManager.cpp +++ b/Core/Debugger/DisassemblyManager.cpp @@ -15,7 +15,7 @@ // Official git repository and contact information can be found at // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. - +#include #include #include @@ -70,10 +70,10 @@ void parseDisasm(const char* disasm, char* opcode, char* arguments, bool insertS u32 branchTarget; sscanf(disasm+3,"%08x",&branchTarget); - const char* addressSymbol = symbolMap.GetLabelName(branchTarget); - if (addressSymbol != NULL && insertSymbols) + const std::string addressSymbol = symbolMap.GetLabelString(branchTarget); + if (!addressSymbol.empty() && insertSymbols) { - arguments += sprintf(arguments,"%s",addressSymbol); + arguments += sprintf(arguments,"%s",addressSymbol.c_str()); } else { arguments += sprintf(arguments,"0x%08X",branchTarget); } @@ -740,16 +740,16 @@ bool DisassemblyMacro::disassemble(u32 address, DisassemblyLineInfo& dest, bool dest.type = DISTYPE_MACRO; dest.info = MIPSAnalyst::GetOpcodeInfo(DisassemblyManager::getCpu(),address); - const char* addressSymbol; + std::string addressSymbol; switch (type) { case MACRO_LI: dest.name = name; - addressSymbol = symbolMap.GetLabelName(immediate); - if (addressSymbol != NULL && insertSymbols) + addressSymbol = symbolMap.GetLabelString(immediate); + if (!addressSymbol.empty() && insertSymbols) { - sprintf(buffer,"%s,%s",DisassemblyManager::getCpu()->GetRegName(0,rt),addressSymbol); + sprintf(buffer,"%s,%s",DisassemblyManager::getCpu()->GetRegName(0,rt),addressSymbol.c_str()); } else { sprintf(buffer,"%s,0x%08X",DisassemblyManager::getCpu()->GetRegName(0,rt),immediate); } @@ -762,10 +762,10 @@ bool DisassemblyMacro::disassemble(u32 address, DisassemblyLineInfo& dest, bool case MACRO_MEMORYIMM: dest.name = name; - addressSymbol = symbolMap.GetLabelName(immediate); - if (addressSymbol != NULL && insertSymbols) + addressSymbol = symbolMap.GetLabelString(immediate); + if (!addressSymbol.empty() && insertSymbols) { - sprintf(buffer,"%s,%s",DisassemblyManager::getCpu()->GetRegName(0,rt),addressSymbol); + sprintf(buffer,"%s,%s",DisassemblyManager::getCpu()->GetRegName(0,rt),addressSymbol.c_str()); } else { sprintf(buffer,"%s,0x%08X",DisassemblyManager::getCpu()->GetRegName(0,rt),immediate); } @@ -956,9 +956,9 @@ void DisassemblyData::createLines() case DATATYPE_WORD: { value = Memory::Read_U32(pos); - const char* label = symbolMap.GetLabelName(value); - if (label != NULL) - sprintf(buffer,"%s",label); + const std::string label = symbolMap.GetLabelString(value); + if (!label.empty()) + sprintf(buffer,"%s",label.c_str()); else sprintf(buffer,"0x%08X",value); pos += 4; diff --git a/Core/Debugger/SymbolMap.cpp b/Core/Debugger/SymbolMap.cpp index d4c900ea56..4bb51498d8 100644 --- a/Core/Debugger/SymbolMap.cpp +++ b/Core/Debugger/SymbolMap.cpp @@ -333,9 +333,7 @@ u32 SymbolMap::GetNextSymbolAddress(u32 address, SymbolType symmask) { return dataAddress; } -static char descriptionTemp[256]; - -const char *SymbolMap::GetDescription(unsigned int address) const { +std::string SymbolMap::GetDescription(unsigned int address) const { lock_guard guard(lock_); const char* labelName = NULL; @@ -351,6 +349,7 @@ const char *SymbolMap::GetDescription(unsigned int address) const { if (labelName != NULL) return labelName; + char descriptionTemp[256]; sprintf(descriptionTemp, "(%08x)", address); return descriptionTemp; } @@ -742,6 +741,14 @@ const char *SymbolMap::GetLabelNameRel(u32 relAddress, int moduleIndex) const { return it->second.name; } +std::string SymbolMap::GetLabelString(u32 address) const { + lock_guard guard(lock_); + const char *label = GetLabelName(address); + if (label == NULL) + return ""; + return label; +} + bool SymbolMap::GetLabelValue(const char* name, u32& dest) { lock_guard guard(lock_); for (auto it = activeLabels.begin(); it != activeLabels.end(); it++) { diff --git a/Core/Debugger/SymbolMap.h b/Core/Debugger/SymbolMap.h index 23b254fc21..0b7825b356 100644 --- a/Core/Debugger/SymbolMap.h +++ b/Core/Debugger/SymbolMap.h @@ -67,7 +67,7 @@ public: SymbolType GetSymbolType(u32 address) const; bool GetSymbolInfo(SymbolInfo *info, u32 address, SymbolType symmask = ST_FUNCTION) const; u32 GetNextSymbolAddress(u32 address, SymbolType symmask); - const char *GetDescription(unsigned int address) const; + std::string GetDescription(unsigned int address) const; std::vector GetAllSymbols(SymbolType symmask); #ifdef _WIN32 @@ -89,9 +89,8 @@ public: bool RemoveFunction(u32 startAddress, bool removeName); void AddLabel(const char* name, u32 address, int moduleIndex = -1); + std::string GetLabelString(u32 address) const; void SetLabelName(const char* name, u32 address); - const char *GetLabelName(u32 address) const; - const char *GetLabelNameRel(u32 relAddress, int moduleIndex) const; bool GetLabelValue(const char* name, u32& dest); void AddData(u32 address, u32 size, DataType type, int moduleIndex = -1); @@ -104,6 +103,8 @@ public: private: void AssignFunctionIndices(); void UpdateActiveSymbols(); + const char *GetLabelName(u32 address) const; + const char *GetLabelNameRel(u32 relAddress, int moduleIndex) const; struct FunctionEntry { u32 start; diff --git a/Core/MIPS/MIPSAnalyst.cpp b/Core/MIPS/MIPSAnalyst.cpp index d8437d06b9..66b1aca7b0 100644 --- a/Core/MIPS/MIPSAnalyst.cpp +++ b/Core/MIPS/MIPSAnalyst.cpp @@ -264,6 +264,15 @@ skip: return !strncmp(name, "z_un_", strlen("z_un_")) || !strncmp(name, "u_un_", strlen("u_un_")); } + static bool IsDefaultFunction(const std::string &name) { + if (name.empty()) { + // Must be I guess? + return true; + } + + return IsDefaultFunction(name.c_str()); + } + static u32 ScanAheadForJumpback(u32 fromAddr, u32 knownStart, u32 knownEnd) { static const u32 MAX_AHEAD_SCAN = 0x1000; // Maybe a bit high... just to make sure we don't get confused by recursive tail recursion. @@ -516,13 +525,13 @@ skip: continue; } // Functions with default names aren't very interesting either. - const char *name = symbolMap.GetLabelName(f.start); + const std::string name = symbolMap.GetLabelString(f.start); if (IsDefaultFunction(name)) { continue; } HashMapFunc mf = { "", f.hash, f.size }; - strncpy(mf.name, name, sizeof(mf.name) - 1); + strncpy(mf.name, name.c_str(), sizeof(mf.name) - 1); hashMap.insert(mf); } } @@ -584,10 +593,10 @@ skip: if (f.hash == mf->hash && f.size == mf->size) { strncpy(f.name, mf->name, sizeof(mf->name) - 1); - const char *existingLabel = symbolMap.GetLabelName(f.start); + std::string existingLabel = symbolMap.GetLabelString(f.start); char defaultLabel[256]; // If it was renamed, keep it. Only change the name if it's still the default. - if (existingLabel == NULL || !strcmp(existingLabel, DefaultFunctionName(defaultLabel, f.start))) { + if (existingLabel.empty() || !strcmp(existingLabel.c_str(), DefaultFunctionName(defaultLabel, f.start))) { symbolMap.SetLabelName(mf->name, f.start); } } diff --git a/Core/MIPS/MIPSDebugInterface.cpp b/Core/MIPS/MIPSDebugInterface.cpp index c29f2dc8f3..ad7ccd42a8 100644 --- a/Core/MIPS/MIPSDebugInterface.cpp +++ b/Core/MIPS/MIPSDebugInterface.cpp @@ -223,7 +223,7 @@ int MIPSDebugInterface::getColor(unsigned int address) if (n==-1) return 0xFFFFFF; return colors[n%6]; } -const char *MIPSDebugInterface::getDescription(unsigned int address) +std::string MIPSDebugInterface::getDescription(unsigned int address) { return symbolMap.GetDescription(address); } diff --git a/Core/MIPS/MIPSDebugInterface.h b/Core/MIPS/MIPSDebugInterface.h index c83ffa99d0..28665b697f 100644 --- a/Core/MIPS/MIPSDebugInterface.h +++ b/Core/MIPS/MIPSDebugInterface.h @@ -17,6 +17,7 @@ #pragma once +#include #include "MIPS.h" #include "Core/Debugger/DebugInterface.h" @@ -39,7 +40,7 @@ public: virtual void step() {} virtual void runToBreakpoint(); virtual int getColor(unsigned int address); - virtual const char *getDescription(unsigned int address); + virtual std::string getDescription(unsigned int address); virtual bool initExpression(const char* exp, PostfixExpression& dest); virtual bool parseExpression(PostfixExpression& exp, u32& dest); diff --git a/Core/MIPS/x86/Jit.cpp b/Core/MIPS/x86/Jit.cpp index ee9f96f8c4..07a984de53 100644 --- a/Core/MIPS/x86/Jit.cpp +++ b/Core/MIPS/x86/Jit.cpp @@ -374,9 +374,9 @@ bool Jit::DescribeCodePtr(const u8 *ptr, std::string &name) else if (jitAddr != (u32)-1) { char temp[1024]; - const char *label = symbolMap.GetDescription(jitAddr); - if (label != NULL) - snprintf(temp, sizeof(temp), "%08x_%s", jitAddr, label); + const std::string label = symbolMap.GetDescription(jitAddr); + if (!label.empty()) + snprintf(temp, sizeof(temp), "%08x_%s", jitAddr, label.c_str()); else snprintf(temp, sizeof(temp), "%08x", jitAddr); name = temp; diff --git a/Windows/Debugger/CtrlDisAsmView.cpp b/Windows/Debugger/CtrlDisAsmView.cpp index a50716f009..ec54e330b4 100644 --- a/Windows/Debugger/CtrlDisAsmView.cpp +++ b/Windows/Debugger/CtrlDisAsmView.cpp @@ -205,8 +205,8 @@ bool CtrlDisAsmView::getDisasmAddressText(u32 address, char* dest, bool abbrevia { if (displaySymbols) { - const char* addressSymbol = symbolMap.GetLabelName(address); - if (addressSymbol != NULL) + const std::string addressSymbol = symbolMap.GetLabelString(address); + if (!addressSymbol.empty()) { for (int k = 0; addressSymbol[k] != 0; k++) { @@ -915,7 +915,7 @@ void CtrlDisAsmView::onMouseUp(WPARAM wParam, LPARAM lParam, int button) { char name[256]; std::string newname; - strncpy_s(name, symbolMap.GetLabelName(funcBegin),_TRUNCATE); + strncpy_s(name, symbolMap.GetLabelString(funcBegin).c_str(),_TRUNCATE); if (InputBox_GetString(MainWindow::GetHInstance(), MainWindow::GetHWND(), L"New function name", name, newname)) { symbolMap.SetLabelName(newname.c_str(),funcBegin); u32 funcSize = symbolMap.GetFunctionSize(curAddress); @@ -1048,10 +1048,10 @@ void CtrlDisAsmView::updateStatusBarText() // TODO: Could also be a float... { u32 data = Memory::Read_U32(line.info.dataAddress); - const char* addressSymbol = symbolMap.GetLabelName(data); - if (addressSymbol) + const std::string addressSymbol = symbolMap.GetLabelString(data); + if (!addressSymbol.empty()) { - sprintf(text,"[%08X] = %s (%08X)",line.info.dataAddress,addressSymbol,data); + sprintf(text,"[%08X] = %s (%08X)",line.info.dataAddress,addressSymbol.c_str(),data); } else { sprintf(text,"[%08X] = %08X",line.info.dataAddress,data); } @@ -1066,12 +1066,12 @@ void CtrlDisAsmView::updateStatusBarText() if (line.info.isBranch) { - const char* addressSymbol = symbolMap.GetLabelName(line.info.branchTarget); - if (addressSymbol == NULL) + const std::string addressSymbol = symbolMap.GetLabelString(line.info.branchTarget); + if (addressSymbol.empty()) { sprintf(text,"%08X",line.info.branchTarget); } else { - sprintf(text,"%08X = %s",line.info.branchTarget,addressSymbol); + sprintf(text,"%08X = %s",line.info.branchTarget,addressSymbol.c_str()); } } } else if (line.type == DISTYPE_DATA) @@ -1081,14 +1081,14 @@ void CtrlDisAsmView::updateStatusBarText() start = curAddress; u32 diff = curAddress-start; - const char* label = symbolMap.GetLabelName(start); + const std::string label = symbolMap.GetLabelString(start); - if (label != NULL) + if (!label.empty()) { if (diff != 0) - sprintf(text,"%08X (%s) + %08X",start,label,diff); + sprintf(text,"%08X (%s) + %08X",start,label.c_str(),diff); else - sprintf(text,"%08X (%s)",start,label); + sprintf(text,"%08X (%s)",start,label.c_str()); } else { if (diff != 0) sprintf(text,"%08X + %08X",start,diff); @@ -1204,7 +1204,7 @@ std::string CtrlDisAsmView::disassembleRange(u32 start, u32 size) { MIPSAnalyst::MipsOpcodeInfo info = MIPSAnalyst::GetOpcodeInfo(debugger,start+i); - if (info.isBranch && symbolMap.GetLabelName(info.branchTarget) == NULL) + if (info.isBranch && symbolMap.GetLabelString(info.branchTarget).empty()) { if (branchAddresses.find(info.branchTarget) == branchAddresses.end()) { @@ -1236,7 +1236,7 @@ std::string CtrlDisAsmView::disassembleRange(u32 start, u32 size) } if (line.info.isBranch && !line.info.isBranchToRegister - && symbolMap.GetLabelName(line.info.branchTarget) == NULL + && symbolMap.GetLabelString(line.info.branchTarget).empty() && branchAddresses.find(line.info.branchTarget) != branchAddresses.end()) { sprintf(buffer,"pos_%08X",line.info.branchTarget); diff --git a/Windows/Debugger/Debugger_Lists.cpp b/Windows/Debugger/Debugger_Lists.cpp index b73cd1b302..b5f1de0115 100644 --- a/Windows/Debugger/Debugger_Lists.cpp +++ b/Windows/Debugger/Debugger_Lists.cpp @@ -463,8 +463,8 @@ void CtrlBreakpointList::GetColumnText(wchar_t* dest, int row, int col) else wsprintf(dest,L"0x%08X",mc.end-mc.start); } else { - const char* sym = symbolMap.GetLabelName(displayedBreakPoints_[index].addr); - if (sym != NULL) + const std::string sym = symbolMap.GetLabelString(displayedBreakPoints_[index].addr); + if (!sym.empty()) { std::wstring s = ConvertUTF8ToWString(sym); wcscpy(dest,s.c_str()); @@ -635,8 +635,8 @@ void CtrlStackTraceView::GetColumnText(wchar_t* dest, int row, int col) break; case SF_ENTRYNAME: { - const char* sym = symbolMap.GetLabelName(frames[row].entry); - if (sym != NULL) { + const std::string sym = symbolMap.GetLabelString(frames[row].entry); + if (!sym.empty()) { wcscpy(dest, ConvertUTF8ToWString(sym).c_str()); } else { wcscpy(dest,L"-");