replica
/
hanchenye-llvm-project
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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>

llvm-svn: 265086
This commit is contained in:
Greg Clayton 2016-04-01 00:41:29 +00:00
parent 8eca282dc9
commit 830c81d511
6 changed files with 201 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
View File

@ -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; 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::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)
{

2
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
View File

@ -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();

136
lldb/source/Utility/StringExtractorGDBRemote.cpp
View File

@ -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
}

32
lldb/source/Utility/StringExtractorGDBRemote.h
View File

@ -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_

1
lldb/tools/debugserver/source/MacOSX/MachProcess.mm
View File

@ -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
{

1
lldb/tools/debugserver/source/RNBRemote.cpp
View File

@ -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;