From 830c81d511881eae8be197d268e497d7fdf62e49 Mon Sep 17 00:00:00 2001 From: Greg Clayton Date: Fri, 1 Apr 2016 00:41:29 +0000 Subject: [PATCH] Fixed an issue that could cause debugserver to return two stop reply packets ($T packets) for one \x03 interrupt. The problem was that when a \x03 byte is sent to debugserver while the process is running, and up calling: rnb_err_t RNBRemote::HandlePacket_stop_process (const char *p) { if (!DNBProcessInterrupt(m_ctx.ProcessID())) HandlePacket_last_signal (NULL); return rnb_success; } In the call to DNBProcessInterrupt we did: nub_bool_t DNBProcessInterrupt(nub_process_t pid) { MachProcessSP procSP; if (GetProcessSP (pid, procSP)) return procSP->Interrupt(); return false; } This would always return false. It would cause HandlePacket_stop_process to always call "HandlePacket_last_signal (NULL);" which would send an extra stop reply packet _if_ the process is stopped. On a machine with enough cores, it would call DNBProcessInterrupt(...) and then HandlePacket_last_signal(NULL) so quickly that it will never send out an extra stop reply packet. But if the machine is slow enough or doesn't have enough cores, it could cause the call to HandlePacket_last_signal() to actually succeed and send an extra stop reply packet. This would cause problems up in GDBRemoteCommunicationClient::SendContinuePacketAndWaitForResponse() where it would get the first stop reply packet and then possibly return or execute an async packet. If it returned, then the next packet that was sent will get the second stop reply as its response. If it executes an async packet, the async packet will get the wrong response. To fix this I did the following: 1 - in debugserver, I fixed "bool MachProcess::Interrupt()" to return true if it sends the signal so we avoid sending the stop reply twice on slower machines 2 - Added a log line to RNBRemote::HandlePacket_stop_process() to say if we ever send an extra stop reply so we will see this in the darwin console output if this does happen 3 - Added response validators to StringExtractorGDBRemote so that we can verify some responses to some packets. 4 - Added validators to packets that often follow stop reply packets like the "m" packet for memory reads, JSON packets since "jThreadsInfo" is often sent immediately following a stop reply. 5 - Modified GDBRemoteCommunicationClient::SendPacketAndWaitForResponseNoLock() to validate responses. Any "StringExtractorGDBRemote &response" that contains a valid response verifier will verify the response and keep looking for correct responses up to 3 times. This will help us get back on track if we do get extra stop replies. If a StringExtractorGDBRemote does not have a response validator, it will accept any packet in response. 6 - In GDBRemoteCommunicationClient::SendPacketAndWaitForResponse we copy the response validator from the "response" argument over into m_async_response so that if we send the packet by interrupting the running process, we can validate the response we actually get in GDBRemoteCommunicationClient::SendContinuePacketAndWaitForResponse() 7 - Modified GDBRemoteCommunicationClient::SendContinuePacketAndWaitForResponse() to always check for an extra stop reply packet for 100ms when the process is interrupted. We were already doing this because we might interrupt a process with a \x03 packet, yet the process was in the process of stopping due to another reason. This race condition could cause an extra stop reply packet because the GDB remote protocol says if a \x03 packet is sent while the process is stopped, we should send a stop reply packet back. Now we always check for an extra stop reply packet when we manually interrupt a process. The issue was showing up when our IDE would attempt to set a breakpoint while the process is running and this would happen: --> \x03 <-- $T --> z0,AAAAA,BB (set breakpoint) <-- $T (incorrect extra stop reply packet) --> c <-- OK (response from z0 packet) Now all packet traffic was off by one response. Since we now have a validator on the response for "z" packets, we do this: --> \x03 <-- $T --> z0,AAAAA,BB (set breakpoint) <-- $T (Ignore this because this can't be the response to z0 packets) <-- OK -- (we are back on track as this is a valid response to z0) ... As time goes on we should add more packet validators. llvm-svn: 265086 --- .../GDBRemoteCommunicationClient.cpp | 38 ++++- .../Process/gdb-remote/ProcessGDBRemote.cpp | 2 + .../Utility/StringExtractorGDBRemote.cpp | 136 +++++++++++++++++- .../source/Utility/StringExtractorGDBRemote.h | 32 ++++- .../debugserver/source/MacOSX/MachProcess.mm | 1 + lldb/tools/debugserver/source/RNBRemote.cpp | 1 + 6 files changed, 201 insertions(+), 9 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp index e61999dbf2a3..472c6aae915d 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -623,6 +623,7 @@ GDBRemoteCommunicationClient::GetThreadsInfo() if (m_supports_jThreadsInfo) { StringExtractorGDBRemote response; + response.SetResponseValidatorToJSON(); if (SendPacketAndWaitForResponse("jThreadsInfo", response, false) == PacketResult::Success) { if (response.IsUnsupportedResponse()) @@ -765,9 +766,29 @@ GDBRemoteCommunicationClient::SendPacketAndWaitForResponseNoLock (const char *pa size_t payload_length, StringExtractorGDBRemote &response) { - PacketResult packet_result = SendPacketNoLock (payload, payload_length); + PacketResult packet_result = SendPacketNoLock(payload, payload_length); if (packet_result == PacketResult::Success) - packet_result = ReadPacket (response, GetPacketTimeoutInMicroSeconds (), true); + { + const size_t max_response_retries = 3; + for (size_t i=0; iPrintf("error: packet with payload \"%*s\" got invalid response \"%s\": %s", + (int)payload_length, + payload, + response.GetStringRef().c_str(), + (i == (max_response_retries - 1)) ? "using invalid response and giving up" : "ignoring response and waiting for another"); + } + } return packet_result; } @@ -801,6 +822,7 @@ GDBRemoteCommunicationClient::SendPacketAndWaitForResponse { Mutex::Locker async_locker (m_async_mutex); m_async_packet.assign(payload, payload_length); + m_async_response.CopyResponseValidator(response); m_async_packet_predicate.SetValue (true, eBroadcastNever); if (log) @@ -867,6 +889,9 @@ GDBRemoteCommunicationClient::SendPacketAndWaitForResponse if (log) log->Printf ("async: failed to interrupt"); } + + m_async_response.SetResponseValidator(nullptr, nullptr); + } else { @@ -1136,8 +1161,11 @@ GDBRemoteCommunicationClient::SendContinuePacketAndWaitForResponse // which will just re-send a copy of the last stop reply // packet. If we don't do this, then the reply for our // async packet will be the repeat stop reply packet and cause - // a lot of trouble for us! - if (signo != sigint_signo && signo != sigstop_signo) + // a lot of trouble for us! We also have some debugserver + // binaries that would send two stop replies anytime the process + // was interrupted, so we need to also check for an extra + // stop reply packet if we interrupted the process + if (m_interrupt_sent || (signo != sigint_signo && signo != sigstop_signo)) { continue_after_async = false; @@ -3572,6 +3600,8 @@ GDBRemoteCommunicationClient::SendGDBStoppointTypePacket (GDBStoppointType type, // Check we haven't overwritten the end of the packet buffer assert (packet_len + 1 < (int)sizeof(packet)); StringExtractorGDBRemote response; + // Make sure the response is either "OK", "EXX" where XX are two hex digits, or "" (unsupported) + response.SetResponseValidatorToOKErrorNotSupported(); // Try to send the breakpoint packet, and check that it was correctly sent if (SendPacketAndWaitForResponse(packet, packet_len, response, true) == PacketResult::Success) { diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index e666b852ba30..c0db8ebc9def 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4170,6 +4170,7 @@ ProcessGDBRemote::GetExtendedInfoForThread (lldb::tid_t tid) packet << (char) (0x7d ^ 0x20); StringExtractorGDBRemote response; + response.SetResponseValidatorToJSON(); if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == GDBRemoteCommunication::PacketResult::Success) { StringExtractorGDBRemote::ResponseType response_type = response.GetResponseType(); @@ -4211,6 +4212,7 @@ ProcessGDBRemote::GetLoadedDynamicLibrariesInfos (lldb::addr_t image_list_addres packet << (char) (0x7d ^ 0x20); StringExtractorGDBRemote response; + response.SetResponseValidatorToJSON(); if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == GDBRemoteCommunication::PacketResult::Success) { StringExtractorGDBRemote::ResponseType response_type = response.GetResponseType(); diff --git a/lldb/source/Utility/StringExtractorGDBRemote.cpp b/lldb/source/Utility/StringExtractorGDBRemote.cpp index 44bb11793639..311ce57b83bc 100644 --- a/lldb/source/Utility/StringExtractorGDBRemote.cpp +++ b/lldb/source/Utility/StringExtractorGDBRemote.cpp @@ -14,8 +14,7 @@ // Other libraries and framework includes // Project includes #include "Utility/StringExtractorGDBRemote.h" - - +#include "lldb/Utility/LLDBAssert.h" StringExtractorGDBRemote::ResponseType StringExtractorGDBRemote::GetResponseType () const @@ -391,3 +390,136 @@ StringExtractorGDBRemote::GetEscapedBinaryData (std::string &str) return str.size(); } +static bool +OKErrorNotSupportedResponseValidator(void *, const StringExtractorGDBRemote &response) +{ + switch (response.GetResponseType()) + { + case StringExtractorGDBRemote::eOK: + case StringExtractorGDBRemote::eError: + case StringExtractorGDBRemote::eUnsupported: + return true; + + case StringExtractorGDBRemote::eAck: + case StringExtractorGDBRemote::eNack: + case StringExtractorGDBRemote::eResponse: + break; + } + lldbassert(!"Packet validatation failed, check why this is happening"); + return false; +} + +static bool +JSONResponseValidator(void *, const StringExtractorGDBRemote &response) +{ + switch (response.GetResponseType()) + { + case StringExtractorGDBRemote::eUnsupported: + case StringExtractorGDBRemote::eError: + return true; // Accept unsupported or EXX as valid responses + + case StringExtractorGDBRemote::eOK: + case StringExtractorGDBRemote::eAck: + case StringExtractorGDBRemote::eNack: + break; + + case StringExtractorGDBRemote::eResponse: + // JSON that is returned in from JSON query packets is currently always + // either a dictionary which starts with a '{', or an array which + // starts with a '['. This is a quick validator to just make sure the + // response could be valid JSON without having to validate all of the + // JSON content. + switch (response.GetStringRef()[0]) + { + case '{': return true; + case '[': return true; + default: + break; + } + break; + } + lldbassert(!"Packet validatation failed, check why this is happening"); + return false; +} + +static bool +ASCIIHexBytesResponseValidator(void *, const StringExtractorGDBRemote &response) +{ + switch (response.GetResponseType()) + { + case StringExtractorGDBRemote::eUnsupported: + case StringExtractorGDBRemote::eError: + return true; // Accept unsupported or EXX as valid responses + + case StringExtractorGDBRemote::eOK: + case StringExtractorGDBRemote::eAck: + case StringExtractorGDBRemote::eNack: + break; + + case StringExtractorGDBRemote::eResponse: + { + uint32_t valid_count = 0; + for (const char ch : response.GetStringRef()) + { + if (!isxdigit(ch)) + { + lldbassert(!"Packet validatation failed, check why this is happening"); + return false; + } + if (++valid_count >= 16) + break; // Don't validate all the characters in case the packet is very large + } + return true; + } + break; + } + lldbassert(!"Packet validatation failed, check why this is happening"); + return false; +} + +void +StringExtractorGDBRemote::CopyResponseValidator(const StringExtractorGDBRemote& rhs) +{ + m_validator = rhs.m_validator; + m_validator_baton = rhs.m_validator_baton; +} + +void +StringExtractorGDBRemote::SetResponseValidator(ResponseValidatorCallback callback, void *baton) +{ + m_validator = callback; + m_validator_baton = baton; +} + +void +StringExtractorGDBRemote::SetResponseValidatorToOKErrorNotSupported() +{ + m_validator = OKErrorNotSupportedResponseValidator; + m_validator_baton = nullptr; +} + +void +StringExtractorGDBRemote::SetResponseValidatorToASCIIHexBytes() +{ + m_validator = ASCIIHexBytesResponseValidator; + m_validator_baton = nullptr; +} + +void +StringExtractorGDBRemote::SetResponseValidatorToJSON() +{ + m_validator = JSONResponseValidator; + m_validator_baton = nullptr; +} + +bool +StringExtractorGDBRemote::ValidateResponse() const +{ + // If we have a validator callback, try to validate the callback + if (m_validator) + return m_validator(m_validator_baton, *this); + else + return true; // No validator, so response is valid +} + + diff --git a/lldb/source/Utility/StringExtractorGDBRemote.h b/lldb/source/Utility/StringExtractorGDBRemote.h index 7e2f1e7c6962..ade0edbbb7ae 100644 --- a/lldb/source/Utility/StringExtractorGDBRemote.h +++ b/lldb/source/Utility/StringExtractorGDBRemote.h @@ -20,18 +20,23 @@ class StringExtractorGDBRemote : public StringExtractor { public: + typedef bool (*ResponseValidatorCallback)(void * baton, const StringExtractorGDBRemote &response); StringExtractorGDBRemote() : - StringExtractor () + StringExtractor(), + m_validator(nullptr) { } StringExtractorGDBRemote(const char *cstr) : - StringExtractor (cstr) + StringExtractor(cstr), + m_validator(nullptr) { } + StringExtractorGDBRemote(const StringExtractorGDBRemote& rhs) : - StringExtractor (rhs) + StringExtractor(rhs), + m_validator(rhs.m_validator) { } @@ -39,6 +44,24 @@ public: { } + bool + ValidateResponse() const; + + void + CopyResponseValidator(const StringExtractorGDBRemote& rhs); + + void + SetResponseValidator(ResponseValidatorCallback callback, void *baton); + + void + SetResponseValidatorToOKErrorNotSupported(); + + void + SetResponseValidatorToASCIIHexBytes(); + + void + SetResponseValidatorToJSON(); + enum ServerPacketType { eServerPacketType_nack = 0, @@ -192,6 +215,9 @@ public: size_t GetEscapedBinaryData (std::string &str); +protected: + ResponseValidatorCallback m_validator; + void *m_validator_baton; }; #endif // utility_StringExtractorGDBRemote_h_ diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm index b9e06307a4aa..7caf79c56ce6 100644 --- a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm +++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm @@ -1048,6 +1048,7 @@ MachProcess::Interrupt() if (Signal (m_sent_interrupt_signo)) { DNBLogThreadedIf(LOG_PROCESS, "MachProcess::Interrupt() - sent %i signal to interrupt process", m_sent_interrupt_signo); + return true; } else { diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp b/lldb/tools/debugserver/source/RNBRemote.cpp index 72c2008c21d8..b3256dc6a9d5 100644 --- a/lldb/tools/debugserver/source/RNBRemote.cpp +++ b/lldb/tools/debugserver/source/RNBRemote.cpp @@ -4433,6 +4433,7 @@ RNBRemote::HandlePacket_stop_process (const char *p) { // If we failed to interrupt the process, then send a stop // reply packet as the process was probably already stopped + DNBLogThreaded ("RNBRemote::HandlePacket_stop_process() sending extra stop reply because DNBProcessInterrupt returned false"); HandlePacket_last_signal (NULL); } return rnb_success;