Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debugger: Avoid unaligned reads in expressions #17269

Merged
merged 2 commits into from
Apr 12, 2023
Merged

Debugger: Avoid unaligned reads in expressions #17269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cebfc3
Debugger: Avoid unaligned reads in expressions.
unknownbrackets Apr 12, 2023
6998b18
Debugger: Fix disasm at start of kernel RAM.
unknownbrackets Apr 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Core/Debugger/DisassemblyManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ u32 DisassemblyManager::getNthPreviousAddress(u32 address, int n)
analyze(address-127,128);
}

return address-n*4;
return (address - n * 4) & ~3;
}

u32 DisassemblyManager::getNthNextAddress(u32 address, int n)
Expand Down Expand Up @@ -396,7 +396,7 @@ u32 DisassemblyManager::getNthNextAddress(u32 address, int n)
analyze(address);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we avoid analyzing if the address has gotten unaligned? It feels like there are some early checks possible, rather than just aligning the return value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that actually aligns the address, and I didn't want to change that logic.

-[Unknown]

}

return address+n*4;
return (address + n * 4) & ~3;
}

DisassemblyManager::~DisassemblyManager() {
Expand Down
18 changes: 11 additions & 7 deletions Core/MIPS/MIPSDebugInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,17 +162,20 @@ class MipsExpressionFunctions: public IExpressionFunctions
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;
}

Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion Core/MemMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 7 additions & 4 deletions GPU/Common/GPUDebugInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 2 additions & 0 deletions Windows/Debugger/CtrlDisAsmView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 4 additions & 2 deletions Windows/Debugger/CtrlMemView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}

Expand Down
25 changes: 14 additions & 11 deletions Windows/Debugger/Debugger_Lists.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down