[Lldb-commits] [lldb] r320985 - Fix regression in jModulesInfo packet handling

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Dec 18 06:31:44 2017
New Revision: 320985

URL: http://llvm.org/viewvc/llvm-project?rev=320985&view=rev
Log:
Fix regression in jModulesInfo packet handling

The recent UUID cleanups exposed a bug in the parsing code for the
jModulesInfo response, which was passing wrong value for the second
argument to UUID::SetFromStringRef (it passed the length of the string,
whereas the correct value should be the number of decoded bytes we
expect to receive).

This was not picked up by tests, because they test with 16-byte uuids,
for which the function happens to do the right thing even if the length
does not match (if the length does not match, the function does not
update m_num_uuid_bytes member, but that member is already 16 to begin
with).

I fix that and add a test with 20-byte uuid to catch if this regresses.
I have also added more safeguards into the parsing code to fail if we
cannot parse the entire uuid field we recieve. While testing the latter
part, I noticed that the "negative" jModulesInfo tests were succeeding
because we were sending malformed json (and not because the json
contents was invalid), so I make those tests a bit more robuts as well.

Modified:
    lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
    lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.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=320985&r1=320984&r2=320985&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp Mon Dec 18 06:31:44 2017
@@ -3442,7 +3442,9 @@ ParseModuleSpec(StructuredData::Dictiona
 
   if (!dict->GetValueForKeyAsString("uuid", string))
     return llvm::None;
-  result.GetUUID().SetFromStringRef(string, string.size());
+  if (result.GetUUID().SetFromStringRef(string, string.size() / 2) !=
+      string.size())
+    return llvm::None;
 
   if (!dict->GetValueForKeyAsInteger("file_offset", integer))
     return llvm::None;

Modified: lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp?rev=320985&r1=320984&r2=320985&view=diff
==============================================================================
--- lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp (original)
+++ lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp Mon Dec 18 06:31:44 2017
@@ -197,27 +197,53 @@ TEST_F(GDBRemoteCommunicationClientTest,
   EXPECT_EQ(1234u, result.getValue()[0].GetObjectSize());
 }
 
+TEST_F(GDBRemoteCommunicationClientTest, GetModulesInfo_UUID20) {
+  llvm::Triple triple("i386-pc-linux");
+
+  FileSpec file_spec("/foo/bar.so", false, FileSpec::ePathSyntaxPosix);
+  std::future<llvm::Optional<std::vector<ModuleSpec>>> async_result =
+      std::async(std::launch::async,
+                 [&] { return client.GetModulesInfo(file_spec, triple); });
+  HandlePacket(
+      server,
+      "jModulesInfo:["
+      R"({"file":"/foo/bar.so","triple":"i386-pc-linux"}])",
+      R"([{"uuid":"404142434445464748494a4b4c4d4e4f50515253","triple":"i386-pc-linux",)"
+      R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}]])");
+
+  auto result = async_result.get();
+  ASSERT_TRUE(result.hasValue());
+  ASSERT_EQ(1u, result->size());
+  EXPECT_EQ("/foo/bar.so", result.getValue()[0].GetFileSpec().GetPath());
+  EXPECT_EQ(triple, result.getValue()[0].GetArchitecture().GetTriple());
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20), result.getValue()[0].GetUUID());
+  EXPECT_EQ(0u, result.getValue()[0].GetObjectOffset());
+  EXPECT_EQ(1234u, result.getValue()[0].GetObjectSize());
+}
+
 TEST_F(GDBRemoteCommunicationClientTest, GetModulesInfoInvalidResponse) {
   llvm::Triple triple("i386-pc-linux");
   FileSpec file_spec("/foo/bar.so", false, FileSpec::ePathSyntaxPosix);
 
   const char *invalid_responses[] = {
-      "OK", "E47", "[]",
       // no UUID
       R"([{"triple":"i386-pc-linux",)"
-      R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}])",
+      R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}]])",
+      // invalid UUID
+      R"([{"uuid":"XXXXXX","triple":"i386-pc-linux",)"
+      R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}]])",
       // no triple
       R"([{"uuid":"404142434445464748494a4b4c4d4e4f",)"
-      R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}])",
+      R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}]])",
       // no file_path
       R"([{"uuid":"404142434445464748494a4b4c4d4e4f","triple":"i386-pc-linux",)"
-      R"("file_offset":0,"file_size":1234}])",
+      R"("file_offset":0,"file_size":1234}]])",
       // no file_offset
       R"([{"uuid":"404142434445464748494a4b4c4d4e4f","triple":"i386-pc-linux",)"
-      R"("file_path":"/foo/bar.so","file_size":1234}])",
+      R"("file_path":"/foo/bar.so","file_size":1234}]])",
       // no file_size
       R"([{"uuid":"404142434445464748494a4b4c4d4e4f","triple":"i386-pc-linux",)"
-      R"("file_path":"/foo/bar.so","file_offset":0}])",
+      R"("file_path":"/foo/bar.so","file_offset":0}]])",
   };
 
   for (const char *response : invalid_responses) {
@@ -229,7 +255,9 @@ TEST_F(GDBRemoteCommunicationClientTest,
         R"(jModulesInfo:[{"file":"/foo/bar.so","triple":"i386-pc-linux"}])",
         response);
 
-    ASSERT_FALSE(async_result.get().hasValue()) << "response was: " << response;
+    auto result = async_result.get();
+    ASSERT_TRUE(result);
+    ASSERT_EQ(0u, result->size()) << "response was: " << response;
   }
 }