Parse remote protocol integers without throwing - #1166
Conversation
std::stoi/stol/stoull raise std::invalid_argument or std::out_of_range on malformed input. The RSP/GDB adapters called them directly on data received from the remote debug stub, so a malformed packet could crash the process with an uncaught exception. Add RspConnector::ParseInt, a std::from_chars-based helper that returns a fallback value instead, and use it at all unguarded call sites. Also guard the packet substr/index accesses in PacketToUnorderedMap against short packets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Why is it ok for all these sites to just default to |
Usually when this happens, the communication already deviated from the expected way in an irrecoverable way (e.g., the GDB stub and our adapter has a different assumption of the RSP protocol), so the actual value does not matter as long we we do not crash ourselves. I think we can add a warning when this happens so as to assist troubleshooting, do you want that to be done? |
|
This kind feels like the opposite of what you should be doing. It seems like instead of just silently accepting corrupted rsp messages you need to throw and exception and you just need to have a more principled approach to what you do when this occurs. So instead of preventing exceptions you just need to catch them in the correct locations so you can do proper teardown. |
Addressing review feedback: defaulting to 0 on a parse failure means a corrupt packet silently becomes a valid-looking answer. A bad thread id turns into thread 0, and the debugger then shows the user the wrong thread's registers with no indication anything went wrong. For a debugger that is worse than stopping. Split parsing in two, so the choice is made per call site rather than by one blanket default: - ParseInt throws RspProtocolError, naming the field, for values the protocol requires. It also now requires the whole field to be consumed, since trailing junk in an integer field is itself evidence of desynchronization. - ParseIntOr takes an explicit fallback, for the two places where a default is genuinely correct: the optional PacketSize capability hint, and the advisory errno in a host I/O reply. Both say why at the call site. The exception is caught in exactly one place, DebuggerController:: ExecuteAdapterAndWait, which every adapter operation already funnels through on the worker thread. It reports the offending data and ends the session, rather than unwinding into a worker thread with no handler and aborting the process. RspProtocolError lives in its own header so that boundary can name it without pulling socket.h and the platform networking headers into the controller. Also fixes multiprocess thread ids, which never parsed: Split takes a regex, so the "." separator matched every character and produced only empty tokens. A "pPID.TID" thread id therefore always reached the parser as an empty string, which crashed on dev. Escaped to "\\.". Verified against a live Corellium stub: its real stop reply parses, malformed packets raise a typed error naming the field, and well-formed packets including "pPID.TID" still parse. Refs #1164
I made the code to throw RspProtocolError with an explanation of the error. When the exception is caught and handled, the debugging is gracefully stopped and an LogError is used to communicate the error to the user Would you please have a look at it again? @plafosse |
2 participants
Add
RspConnector::ParseInt(based onstd::from_chars) and use it instead ofstd::stoi/stoullwhen parsing data from the remote debug stub, so malformed packets can no longer crash the process. Also guard short-packetsubstr/index accesses inPacketToUnorderedMap.Fixes #1164