Skip to content

Fix crash from unhandled exceptions in RspConnector#1046

Merged
xusheng6 merged 1 commit intodevfrom
test_1043-unhandled-exceptions-in-connect
Apr 8, 2026
Merged

Fix crash from unhandled exceptions in RspConnector#1046
xusheng6 merged 1 commit intodevfrom
test_1043-unhandled-exceptions-in-connect

Conversation

@xusheng6
Copy link
Copy Markdown
Member

@xusheng6 xusheng6 commented Apr 6, 2026

Summary

  • Replace all throw std::runtime_error(...) in RspConnector::ExpectAck, TransmitAndReceive, and GetXml with LogError(...) + return empty/default values
  • These exceptions were unhandled in the call stacks of CorelliumAdapter::Connect, GdbAdapter::Connect, and EsrevenAdapter::Connect, causing crashes (aborts)
  • Callers already handle empty responses gracefully (e.g., LoadRegisterInfo returns false on parse failure, which is already checked)

Test plan

  • Connect to a GDB remote that returns malformed RSP data — verify error is logged and connect fails gracefully instead of crashing
  • Connect to a Corellium target — verify normal operation is unaffected
  • Disconnect during RSP handshake — verify no crash

Fixes #1043

🤖 Generated with Claude Code

Copy link
Copy Markdown
Member

@plafosse plafosse left a comment

Choose a reason for hiding this comment

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

You changed from throw to return empty but it doesn't look like you're handling that return empty anywhere? Changing RspConnector::ExpectAck() from “throw on protocol failure” to “log and return empty” is not safe for the resume paths that ignore its return value. GdbAdapter::GenericGo, CorelliumAdapter::GenericGo, and EsrevenAdapter::GenericGo all set their local running flag, send the packet, call ExpectAck(), and then continue into ResponseHandler(...) without checking whether the ack failed (public/debugger/core/adapters/gdbadapter.cpp:1052, public/debugger/core/adapters/corelliumadapter.cpp:918, public/debugger/core/adapters/esrevenadapter.cpp:1903). With this PR, a disconnect or malformed ack no longer aborts that flow; it just logs and keeps going. That can leave the adapter/controller believing the target resumed when the command was never successfully acknowledged, which is a behavioral regression from the previous fail-fast behavior.

ui/ui.cpp Outdated
frame->navigate(m_controller->GetData(), address, true, true);
}

openDebuggerSideBar(frame);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that same commit again

RspConnector::ExpectAck, TransmitAndReceive, and GetXml threw
std::runtime_error on protocol errors, but nothing in the call
stack caught them, causing crashes (e.g., in CorelliumAdapter::Connect
via LoadRegisterInfo -> GetXml, and GdbAdapter::Connect via
TransmitAndReceive -> ExpectAck).

Replace all throws with LogError + return empty/default values.
Callers already handle empty responses gracefully (e.g.,
LoadRegisterInfo returns false on parse failure).

Fixes #1043

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@xusheng6 xusheng6 force-pushed the test_1043-unhandled-exceptions-in-connect branch from 75f75cc to 4a4022f Compare April 7, 2026 03:22
@xusheng6
Copy link
Copy Markdown
Member Author

xusheng6 commented Apr 7, 2026

You changed from throw to return empty but it doesn't look like you're handling that return empty anywhere? Changing RspConnector::ExpectAck() from “throw on protocol failure” to “log and return empty” is not safe for the resume paths that ignore its return value. GdbAdapter::GenericGo, CorelliumAdapter::GenericGo, and EsrevenAdapter::GenericGo all set their local running flag, send the packet, call ExpectAck(), and then continue into ResponseHandler(...) without checking whether the ack failed (public/debugger/core/adapters/gdbadapter.cpp:1052, public/debugger/core/adapters/corelliumadapter.cpp:918, public/debugger/core/adapters/esrevenadapter.cpp:1903). With this PR, a disconnect or malformed ack no longer aborts that flow; it just logs and keeps going. That can leave the adapter/controller believing the target resumed when the command was never successfully acknowledged, which is a behavioral regression from the previous fail-fast behavior.

I changed it so that ExpectAck() now returns a bool, and the callers all check its return value.

Also, When ExpectAck fails, we are using LogError to display an error dialog so that the user knows what is going on

@plafosse
Copy link
Copy Markdown
Member

plafosse commented Apr 7, 2026

I'm not seeing these changes but that sounds like the appropriate thing to do.

@xusheng6 xusheng6 merged commit b17c0b9 into dev Apr 8, 2026
1 check passed
@xusheng6 xusheng6 deleted the test_1043-unhandled-exceptions-in-connect branch April 8, 2026 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash due to unhandled exceptions below FooAdapter::Connect

2 participants