diff --git a/core/adapters/corelliumadapter.cpp b/core/adapters/corelliumadapter.cpp index 02815426..4a4f29cb 100644 --- a/core/adapters/corelliumadapter.cpp +++ b/core/adapters/corelliumadapter.cpp @@ -348,7 +348,7 @@ std::vector CorelliumAdapter::GetThreadList() reply.AsString().substr(1); const auto tids = RspConnector::Split(shortened_string, ","); for ( const auto& tid : tids ) - threads.emplace_back(std::stoi(tid, nullptr, 16)); + threads.emplace_back(RspConnector::ParseInt(tid, "thread id")); reply = this->m_rspConnector->TransmitAndReceive(RspData("qsThreadInfo")); } @@ -827,7 +827,7 @@ DebugStopReason CorelliumAdapter::ResponseHandler() if (replyString.length() >= 3) { std::string signalString = replyString.substr(1, 2); - uint64_t signal = std::stoull(signalString, nullptr, 16); + uint64_t signal = RspConnector::ParseInt(signalString, "signal"); m_isTargetRunning = false; @@ -993,7 +993,7 @@ static std::string HexToAscii(const std::string& hex) { // Convert the two hex characters to a byte (using a stringstream) std::string byte_string = hex.substr(i, 2); - unsigned char byte = static_cast(std::stoi(byte_string, nullptr, 16)); // Convert to byte + unsigned char byte = static_cast(RspConnector::ParseInt(byte_string, "hex byte")); // Append the byte (ASCII char) to the resulting string ascii.push_back(byte); diff --git a/core/adapters/gdbadapter.cpp b/core/adapters/gdbadapter.cpp index 4e6ab3bb..6a85814c 100644 --- a/core/adapters/gdbadapter.cpp +++ b/core/adapters/gdbadapter.cpp @@ -377,7 +377,7 @@ std::vector GdbAdapter::GetThreadList() reply.AsString().substr(1); const auto tids = RspConnector::Split(shortened_string, ","); for ( const auto& tid : tids ) - threads.emplace_back(std::stoi(tid, nullptr, 16)); + threads.emplace_back(RspConnector::ParseInt(tid, "thread id")); reply = this->m_rspConnector->TransmitAndReceive(RspData("qsThreadInfo")); } @@ -1000,8 +1000,8 @@ DebugStopReason GdbAdapter::ResponseHandler(bool notifyStopped) if (replyString.length() >= 3) { std::string signalString = replyString.substr(1, 2); - uint64_t signal = std::stoull(signalString, nullptr, 16); - + uint64_t signal = RspConnector::ParseInt(signalString, "signal"); + m_isTargetRunning = false; CheckApplyPendingBreakpoints(); @@ -1457,7 +1457,7 @@ static std::string HexToAscii(const std::string& hex) { // Convert the two hex characters to a byte (using a stringstream) std::string byte_string = hex.substr(i, 2); - unsigned char byte = static_cast(std::stoi(byte_string, nullptr, 16)); // Convert to byte + unsigned char byte = static_cast(RspConnector::ParseInt(byte_string, "hex byte")); // Append the byte (ASCII char) to the resulting string ascii.push_back(byte); diff --git a/core/adapters/gdbmiadapter.cpp b/core/adapters/gdbmiadapter.cpp index 0170b083..32b7bed8 100644 --- a/core/adapters/gdbmiadapter.cpp +++ b/core/adapters/gdbmiadapter.cpp @@ -1,5 +1,6 @@ #include "gdbmiadapter.h" #include +#include #include #include "../debuggercontroller.h" #include "../../cli/log.h" @@ -953,7 +954,16 @@ DataBuffer GdbMiAdapter::ReadMemory(std::uintptr_t address, size_t size) { std::string hex_contents = value["memory"][0]["contents"].GetString(); DataBuffer buffer(hex_contents.length() / 2); for(size_t i = 0; i < buffer.GetLength(); i++) { - buffer[i] = std::stoul(hex_contents.substr(i*2, 2), nullptr, 16); + // Parse with the non-throwing std::from_chars, since std::stoul throws on + // malformed data coming from the gdb process + unsigned int byte = 0; + const char* first = hex_contents.data() + i * 2; + if (std::from_chars(first, first + 2, byte, 16).ec != std::errc()) + { + LogDebug("Malformed hex contents in memory read reply"); + return zero; + } + buffer[i] = byte; } return buffer; } diff --git a/core/adapters/rspconnector.cpp b/core/adapters/rspconnector.cpp index 9fdc93b2..728c72fc 100644 --- a/core/adapters/rspconnector.cpp +++ b/core/adapters/rspconnector.cpp @@ -93,9 +93,16 @@ RspData RspConnector::DecodeRLE(const RspData& data) std::unordered_map RspConnector::PacketToUnorderedMap(const RspData& data) { std::unordered_map packet_map{}; - packet_map["signal"] = std::stoull(data.AsString().substr(1, 2), nullptr, 16); - const auto data_string = data.AsString(); + // Every packet this parses is a stop reply, which is at minimum a type byte plus a + // two digit signal. Anything shorter is not a truncated stop reply we can salvage -- + // it is a sign the connection has desynchronized, including the empty RspData that + // ReceiveRspData returns when a read times out or the framing is wrong. + if (data_string.length() < 3) + throw RspProtocolError("stop reply", data_string); + + packet_map["signal"] = ParseInt(data_string.substr(1, 2), "signal"); + const auto after_signal = data_string.substr(3); for ( const auto& entries : RspConnector::Split(after_signal, ";")) { @@ -116,16 +123,25 @@ std::unordered_map RspConnector::PacketToUnorderedMa if (key == "thread") { if (value[0] == 'p' && value.find('.') != std::string::npos) { - auto core_id_and_thread_id = RspConnector::Split(value.substr(1), "."); - packet_map["thread"] = std::stoull(core_id_and_thread_id[1], nullptr, 16); + // Split takes a regex, so the separator has to be escaped -- an + // unescaped "." matches every character and yields only empty tokens, + // which meant multiprocess thread ids ("pPID.TID") never parsed. + auto core_id_and_thread_id = RspConnector::Split(value.substr(1), "\\."); + // "pPID.TID" without the TID half is a malformed thread id, not an + // absent one -- the stub told us which thread stopped and we could + // not read it, so we must not guess. + if (core_id_and_thread_id.size() < 2) + throw RspProtocolError("thread id", value); + packet_map["thread"] = ParseInt(core_id_and_thread_id[1], "thread id"); } else { - packet_map["thread"] = std::stoull(value, nullptr, 16); + packet_map["thread"] = ParseInt(value, "thread id"); } } else if (std::regex_search(key, std::regex("^[0-9a-fA-F]+$"))) { - packet_map[fmt::format("r{}", std::stoi(key, nullptr, 16))] = - static_cast( RspConnector::SwapEndianness(std::stoull(value, nullptr, 16))); + packet_map[fmt::format("r{}", ParseInt(key, "register number"))] = + static_cast( + RspConnector::SwapEndianness(ParseInt(value, "register value"))); } else { - packet_map[key] = std::stoull(value, nullptr, 16); + packet_map[key] = ParseInt(value, key.c_str()); } } else @@ -204,8 +220,11 @@ void RspConnector::NegotiateCapabilities(const std::vector & capabi { if ( reply_token.find("PacketSize=") != std::string::npos ) { - if (auto packet_tokens = RspConnector::Split(reply_token, "="); !packet_tokens.empty()) - this->m_maxPacketLength = std::stoi(packet_tokens[1], nullptr, 16); + // Capability negotiation is best effort: PacketSize is an optional hint, and a + // stub that omits or garbles it is not desynchronized -- we simply keep the + // packet length we already had and carry on. + if (auto packet_tokens = RspConnector::Split(reply_token, "="); packet_tokens.size() >= 2) + this->m_maxPacketLength = ParseIntOr(packet_tokens[1], this->m_maxPacketLength); continue; } @@ -412,12 +431,17 @@ int32_t RspConnector::HostFileIO(const RspData& data, RspData& output, int32_t& // split off errno if (resultErrno.find(',') != std::string::npos) { const auto split = RspConnector::Split(resultErrno, ","); + // The host I/O reply carries a result and an errno. Both are advisory: the caller + // already treats a negative result as "the file operation failed", so an + // unreadable value degrades to exactly that rather than invalidating the session. if ((split.size() >= 2) && (split[1] != "")) - error = std::stol(split[1].c_str(), nullptr, 16); + error = ParseIntOr(split[1], -1); - return std::stol(split[0].c_str(), nullptr, 16); + if (split.empty()) + return -1; + return ParseIntOr(split[0], -1); } - return std::stol(resultErrno.c_str(), nullptr, 16); + return ParseIntOr(resultErrno, -1); } diff --git a/core/adapters/rspconnector.h b/core/adapters/rspconnector.h index 2c9c7bb1..0a407786 100644 --- a/core/adapters/rspconnector.h +++ b/core/adapters/rspconnector.h @@ -19,6 +19,7 @@ limitations under the License. #include #include #include +#include #include #include #include "binaryninjaapi.h" @@ -37,6 +38,7 @@ limitations under the License. #endif #include #include "socket.h" +#include "rspprotocolerror.h" namespace BinaryNinjaDebugger { @@ -153,6 +155,38 @@ namespace BinaryNinjaDebugger static std::unordered_map PacketToUnorderedMap(const RspData& data); static std::vector Split(const std::string& string, const std::string& regex); + // Parse an integer that the protocol requires to be present and well formed. + // + // std::from_chars is used rather than std::stoi/std::stoull because the latter are + // locale sensitive and report failure by throwing types (std::invalid_argument, + // std::out_of_range) that say nothing about which field went wrong. On bad input this + // raises RspProtocolError instead; see that class for why we do not substitute a + // default. The whole string must be consumed -- trailing junk in a field the protocol + // defines as an integer is itself evidence the connection has desynchronized. + template + static Ty ParseInt(const std::string& str, const char* field = "integer", int base = 16) + { + Ty value {}; + const auto result = std::from_chars(str.data(), str.data() + str.size(), value, base); + if ((result.ec != std::errc()) || (result.ptr != str.data() + str.size())) + throw RspProtocolError(field, str); + return value; + } + + // Parse an integer that the protocol genuinely allows to be absent or that we can + // proceed without. Use this only where the fallback is meaningful, and say why at the + // call site -- everywhere else should use ParseInt so that a desynchronized connection + // is reported rather than absorbed. + template + static Ty ParseIntOr(const std::string& str, Ty fallback, int base = 16) + { + Ty value {}; + const auto result = std::from_chars(str.data(), str.data() + str.size(), value, base); + if ((result.ec != std::errc()) || (result.ptr != str.data() + str.size())) + return fallback; + return value; + } + static uint64_t SwapEndianness(uint64_t value, size_t len) { switch (len) diff --git a/core/adapters/rspprotocolerror.h b/core/adapters/rspprotocolerror.h new file mode 100644 index 00000000..383f8c60 --- /dev/null +++ b/core/adapters/rspprotocolerror.h @@ -0,0 +1,52 @@ +/* +Copyright 2020-2026 Vector 35 Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +#pragma once + +// Kept apart from rspconnector.h so that the catch boundary in debuggercontroller.cpp can +// name this type without also pulling in socket.h and the platform networking headers. + +#include +#include +#include + +namespace BinaryNinjaDebugger +{ + // Raised when the remote stub sends data that does not conform to the RSP protocol. + // + // A malformed packet means we and the stub no longer agree on the state of the + // connection, and nothing received afterwards can be trusted. Substituting a default + // value would let the session continue against the wrong thread, the wrong registers, + // or a stop that never happened, which is worse than stopping: the user would be shown + // confident but incorrect state. Callers catch this at the adapter operation boundary + // (DebuggerController::ExecuteAdapterAndWait) and terminate the session there. + class RspProtocolError : public std::runtime_error + { + std::string m_field; + std::string m_data; + + public: + RspProtocolError(const std::string& field, const std::string& data) : + std::runtime_error(fmt::format("malformed {} in remote protocol data: \"{}\"", field, data)), + m_field(field), m_data(data) + {} + + // The protocol field we were trying to read, e.g. "signal" or "thread id". + const std::string& Field() const { return m_field; } + // The offending text, for troubleshooting. + const std::string& Data() const { return m_data; } + }; +}; diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 7d79d7bd..2743f3a8 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -23,6 +23,7 @@ limitations under the License. #include "mediumlevelilinstruction.h" #include "highlevelilinstruction.h" #include "debuggerfileaccessor.h" +#include "adapters/rspprotocolerror.h" using namespace BinaryNinjaDebugger; @@ -3365,7 +3366,52 @@ DebugStopReason DebuggerController::StopReason() const } +// Catch boundary for remote protocol errors. Every adapter operation funnels through here +// on the worker thread (see the invariant in ExecuteAdapterAndWaitInternal), which makes this +// the one place where a malformed packet from the stub can be turned into a clean, deliberate +// end to the debugging session instead of unwinding into a worker thread with no handler. DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOperation operation) +{ + try + { + return ExecuteAdapterAndWaitInternal(operation); + } + catch (const RspProtocolError& e) + { + return HandleProtocolError(e); + } +} + + +// Report a desynchronized connection and end the session. +// +// There is no recovery from a protocol error: we and the stub disagree about what has been +// sent, so every subsequent read is suspect. Rather than continue against state we cannot +// trust, tell the user what went wrong -- including the offending data, which is what makes +// this diagnosable in the field -- and take the target down. +DebugStopReason DebuggerController::HandleProtocolError(const RspProtocolError& error) +{ + LogError("Debugger protocol error: %s. The debugging session has been terminated.", error.what()); + + NotifyError(fmt::format("{}. The debugging session has been terminated.", error.what()), + "Debugger protocol error"); + + // Surfacing this as a target exit is what actually performs the teardown: it resets the + // connection and execution status and lets the UI return to a not-debugging state. Posting + // it synchronously here is safe even though the adapter lock may still be held -- see the + // TargetExitedEventType case in ApplyOwnStateForEvent, which is deliberately lock-free. + DebuggerEvent exitEvent; + exitEvent.type = TargetExitedEventType; + exitEvent.data.exitData.exitCode = 0; + PostDebuggerEvent(exitEvent); + + FinalizeTargetGoneCleanup(); + + return DebugStopReason::InternalError; +} + + +DebugStopReason DebuggerController::ExecuteAdapterAndWaitInternal(const DebugAdapterOperation operation) { // Invariant: ExecuteAdapterAndWait only ever runs on m_workerThread. The worker queue // serializes all adapter operations, so the previous m_adapterMutex / m_adapterMutex2 diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index 272ca8b8..fe5cfd1b 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -34,6 +34,7 @@ DECLARE_DEBUGGER_API_OBJECT(BNDebuggerController, DebuggerController); namespace BinaryNinjaDebugger { class DebuggerController; + class RspProtocolError; // Set to the controller pointer when running on that controller's worker thread, // nullptr otherwise. Used by DebuggerController::Submit to detect re-entrant calls @@ -587,6 +588,9 @@ namespace BinaryNinjaDebugger { bool Pause(); DebugStopReason ExecuteAdapterAndWait(const DebugAdapterOperation operation); + DebugStopReason ExecuteAdapterAndWaitInternal(const DebugAdapterOperation operation); + // Ends the session after the remote stub sent something we cannot parse. + DebugStopReason HandleProtocolError(const RspProtocolError& error); // Synchronous APIs // Synchronous APIs. They submit the operation to the worker thread and block the