From fcdb1af655a80b4f1b4f17929235613216ebaaf5 Mon Sep 17 00:00:00 2001 From: Todd Fiala Date: Sat, 10 Sep 2016 00:06:29 +0000 Subject: [PATCH] async structured data packet handling improvements This change does the following: * Changes the signature for the continuation delegate method that handles async structured data from accepting an already-parsed structured data element to taking just the packet contents. * Moves the conversion of the JSON-async: packet contents from GDBRemoteClientBase to the continuation delegate method. * Adds a new unit test for verifying that the $JSON-asyc: packets get decoded and that the decoded packets get forwarded on to the delegate for further processing. Thanks to Pavel for making that whole section of code easily unit testable! * Tightens up the packet verification on reception of a $JSON-async: packet contents. The code prior to this change is susceptible to a segfault if a packet is carefully crafted that starts with $J but has a total length shorter than the length of "$JSON-async:". Reviewers: labath, clayborg, zturner Differential Revision: https://reviews.llvm.org/D23884 llvm-svn: 281121 --- .../Process/gdb-remote/GDBRemoteClientBase.cpp | 33 +--------------- .../Process/gdb-remote/GDBRemoteClientBase.h | 13 +++--- .../Process/gdb-remote/ProcessGDBRemote.cpp | 46 ++++++++++++++++++++-- .../Plugins/Process/gdb-remote/ProcessGDBRemote.h | 3 +- .../Process/gdb-remote/GDBRemoteClientBaseTest.cpp | 38 ++++++++++++++++-- 5 files changed, 87 insertions(+), 46 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp index 3b27c10..853e201 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp @@ -103,38 +103,9 @@ StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse( delegate.HandleAsyncMisc( llvm::StringRef(response.GetStringRef()).substr(1)); break; - case 'J': - // Asynchronous JSON packet, destined for a - // StructuredDataPlugin. - { - // Parse the content into a StructuredData instance. - auto payload_index = strlen("JSON-async:"); - StructuredData::ObjectSP json_sp = StructuredData::ParseJSON( - response.GetStringRef().substr(payload_index)); - if (log) { - if (json_sp) - log->Printf("GDBRemoteCommmunicationClientBase::%s() " - "received Async StructuredData packet: %s", - __FUNCTION__, - response.GetStringRef().substr(payload_index).c_str()); - else - log->Printf("GDBRemoteCommmunicationClientBase::%s" - "() received StructuredData packet:" - " parse failure", - __FUNCTION__); - } - - // Pass the data to the process to route to the - // appropriate plugin. The plugin controls what happens - // to it from there. - bool routed = delegate.HandleAsyncStructuredData(json_sp); - if (log) - log->Printf("GDBRemoteCommmunicationClientBase::%s()" - " packet %s", - __FUNCTION__, routed ? "handled" : "not handled"); - break; - } + delegate.HandleAsyncStructuredDataPacket(response.GetStringRef()); + break; case 'T': case 'S': // Do this with the continue lock held. diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h index f392895..15d7ec2 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h @@ -25,14 +25,13 @@ public: virtual void HandleAsyncMisc(llvm::StringRef data) = 0; virtual void HandleStopReply() = 0; - // - /// Processes async structured data. + // ========================================================================= + /// Process asynchronously-received structured data. /// - /// @return - /// true if the data was handled; otherwise, false. - // - virtual bool - HandleAsyncStructuredData(const StructuredData::ObjectSP &object_sp) = 0; + /// @param[in] data + /// The complete data packet, expected to start with JSON-async. + // ========================================================================= + virtual void HandleAsyncStructuredDataPacket(llvm::StringRef data) = 0; }; GDBRemoteClientBase(const char *comm_name, const char *listener_name); diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index e64ce50..504aa4f 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4808,9 +4808,49 @@ void ProcessGDBRemote::HandleStopReply() { BuildDynamicRegisterInfo(true); } -bool ProcessGDBRemote::HandleAsyncStructuredData( - const StructuredData::ObjectSP &object_sp) { - return RouteAsyncStructuredData(object_sp); +static const char *const s_async_json_packet_prefix = "JSON-async:"; + +static StructuredData::ObjectSP +ParseStructuredDataPacket(llvm::StringRef packet) { + Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS)); + + if (!packet.consume_front(s_async_json_packet_prefix)) { + if (log) { + log->Printf( + "GDBRemoteCommmunicationClientBase::%s() received $J packet " + "but was not a StructuredData packet: packet starts with " + "%s", + __FUNCTION__, + packet.slice(0, strlen(s_async_json_packet_prefix)).str().c_str()); + } + return StructuredData::ObjectSP(); + } + + // This is an asynchronous JSON packet, destined for a + // StructuredDataPlugin. + StructuredData::ObjectSP json_sp = StructuredData::ParseJSON(packet); + if (log) { + if (json_sp) { + StreamString json_str; + json_sp->Dump(json_str); + json_str.Flush(); + log->Printf("ProcessGDBRemote::%s() " + "received Async StructuredData packet: %s", + __FUNCTION__, json_str.GetString().c_str()); + } else { + log->Printf("ProcessGDBRemote::%s" + "() received StructuredData packet:" + " parse failure", + __FUNCTION__); + } + } + return json_sp; +} + +void ProcessGDBRemote::HandleAsyncStructuredDataPacket(llvm::StringRef data) { + auto structured_data_sp = ParseStructuredDataPacket(data); + if (structured_data_sp) + RouteAsyncStructuredData(structured_data_sp); } class CommandObjectProcessGDBRemoteSpeedTest : public CommandObjectParsed { diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h index 493176d..d607d35 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -415,8 +415,7 @@ private: void HandleAsyncStdout(llvm::StringRef out) override; void HandleAsyncMisc(llvm::StringRef data) override; void HandleStopReply() override; - bool - HandleAsyncStructuredData(const StructuredData::ObjectSP &object_sp) override; + void HandleAsyncStructuredDataPacket(llvm::StringRef data) override; using ModuleCacheKey = std::pair; // KeyInfo for the cached module spec DenseMap. diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp index c9d4544..0e74d88 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp @@ -20,6 +20,7 @@ #include "Plugins/Process/Utility/LinuxSignals.h" #include "Plugins/Process/gdb-remote/GDBRemoteClientBase.h" #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h" +#include "lldb/Core/StreamGDBRemote.h" #include "llvm/ADT/STLExtras.h" @@ -34,14 +35,14 @@ struct MockDelegate : public GDBRemoteClientBase::ContinueDelegate { std::string output; std::string misc_data; unsigned stop_reply_called = 0; + std::vector structured_data_packets; void HandleAsyncStdout(llvm::StringRef out) { output += out; } void HandleAsyncMisc(llvm::StringRef data) { misc_data += data; } void HandleStopReply() { ++stop_reply_called; } - bool HandleAsyncStructuredData(const StructuredData::ObjectSP &object_sp) { - // TODO work in a test here after I fix the gtest breakage. - return true; + void HandleAsyncStructuredDataPacket(llvm::StringRef data) { + structured_data_packets.push_back(data); } }; @@ -321,6 +322,37 @@ TEST_F(GDBRemoteClientBaseTest, SendContinueDelegateInterface) { EXPECT_EQ(1u, fix.delegate.stop_reply_called); } +TEST_F(GDBRemoteClientBaseTest, SendContinueDelegateStructuredDataReceipt) { + // Build the plain-text version of the JSON data we will have the + // server send. + const std::string json_payload = + "{ \"type\": \"MyFeatureType\", " + " \"elements\": [ \"entry1\", \"entry2\" ] }"; + const std::string json_packet = "JSON-async:" + json_payload; + + // Escape it properly for transit. + StreamGDBRemote stream; + stream.PutEscapedBytes(json_packet.c_str(), json_packet.length()); + stream.Flush(); + + // Set up the + StringExtractorGDBRemote response; + ContinueFixture fix; + if (HasFailure()) + return; + + // Send async structured data packet, then stop. + ASSERT_EQ(PacketResult::Success, fix.server.SendPacket(stream.GetData())); + ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("T01")); + ASSERT_EQ(eStateStopped, fix.SendCPacket(response)); + ASSERT_EQ("T01", response.GetStringRef()); + ASSERT_EQ(1, fix.delegate.structured_data_packets.size()); + + // Verify the packet contents. It should have been unescaped upon packet + // reception. + ASSERT_EQ(json_packet, fix.delegate.structured_data_packets[0]); +} + TEST_F(GDBRemoteClientBaseTest, InterruptNoResponse) { StringExtractorGDBRemote continue_response, response; ContinueFixture fix; -- 2.7.4