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

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
index 3b27c10..853e201 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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<std::string, std::string>;
   // 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 (file)
--- 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<std::string> 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;