[Lldb-commits] [lldb] r265086 - 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:
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Thu Mar 31 17:41:29 PDT 2016
Author: gclayton
Date: Thu Mar 31 19:41:29 2016
New Revision: 265086
URL: http://llvm.org/viewvc/llvm-project?rev=265086&view=rev
Log:
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<stop reply 1>
--> z0,AAAAA,BB (set breakpoint)
<-- $T<stop reply 1> (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<stop reply 1>
--> z0,AAAAA,BB (set breakpoint)
<-- $T<stop reply 1> (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.
<rdar://problem/22859505>
Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp
lldb/trunk/source/Utility/StringExtractorGDBRemote.h
lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm
lldb/trunk/tools/debugserver/source/RNBRemote.cpp
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp Thu Mar 31 19:41:29 2016
@@ -623,6 +623,7 @@ GDBRemoteCommunicationClient::GetThreads
if (m_supports_jThreadsInfo)
{
StringExtractorGDBRemote response;
+ response.SetResponseValidatorToJSON();
if (SendPacketAndWaitForResponse("jThreadsInfo", response, false) == PacketResult::Success)
{
if (response.IsUnsupportedResponse())
@@ -765,9 +766,29 @@ GDBRemoteCommunicationClient::SendPacket
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; i<max_response_retries; ++i)
+ {
+ packet_result = ReadPacket(response, GetPacketTimeoutInMicroSeconds (), true);
+ // Make sure we received a response
+ if (packet_result != PacketResult::Success)
+ return packet_result;
+ // Make sure our response is valid for the payload that was sent
+ if (response.ValidateResponse())
+ return packet_result;
+ // Response says it wasn't valid
+ Log *log = ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PACKETS);
+ if (log)
+ log->Printf("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::SendPacket
{
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::SendPacket
if (log)
log->Printf ("async: failed to interrupt");
}
+
+ m_async_response.SetResponseValidator(nullptr, nullptr);
+
}
else
{
@@ -1136,8 +1161,11 @@ GDBRemoteCommunicationClient::SendContin
// 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::SendGDBSto
// 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)
{
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Thu Mar 31 19:41:29 2016
@@ -4170,6 +4170,7 @@ ProcessGDBRemote::GetExtendedInfoForThre
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::GetLoadedDynamicLibrar
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();
Modified: lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp (original)
+++ lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp Thu Mar 31 19:41:29 2016
@@ -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::GetEscapedBina
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
+}
+
+
Modified: lldb/trunk/source/Utility/StringExtractorGDBRemote.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/StringExtractorGDBRemote.h?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/source/Utility/StringExtractorGDBRemote.h (original)
+++ lldb/trunk/source/Utility/StringExtractorGDBRemote.h Thu Mar 31 19:41:29 2016
@@ -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_
Modified: lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm (original)
+++ lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm Thu Mar 31 19:41:29 2016
@@ -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
{
Modified: lldb/trunk/tools/debugserver/source/RNBRemote.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/source/RNBRemote.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/tools/debugserver/source/RNBRemote.cpp (original)
+++ lldb/trunk/tools/debugserver/source/RNBRemote.cpp Thu Mar 31 19:41:29 2016
@@ -4433,6 +4433,7 @@ RNBRemote::HandlePacket_stop_process (co
{
// 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;
More information about the lldb-commits
mailing list