Verify memory address range validity in GDBRemoteCommunicationClient

[xiaobai]

Summary

This aims to verify the validity of the response from the debugging
server in GDBRemoteCommunicationClient::GetMemoryRegionInfo. I was
working with ds2 (https://github.com/facebook/ds2) and encountered a bug
that caused the server's response to have a 'size' value of 0, which
caused lldb to behave incorrectly.

[sas]

Doing additional checking on the packets returned over the wire seems decent to me. CC'ing @clayborg to see what he thinks about it also.

[labath]

The additional check sounds fine. There's a test for this class in unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp where you can add a test for this behavior.

[clayborg]

Simple logic change so we don't check the range validity more than once.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

1506–1510 ↗

(On Diff #93427)

Logic is fine, but I would work this into the if statement below. Something like:

if (region_info.GetRange().IsValid()) {
  if (!saw_permissions) {
    region_info.SetReadable(MemoryRegionInfo::eNo);
    region_info.SetWritable(MemoryRegionInfo::eNo);
    region_info.SetExecutable(MemoryRegionInfo::eNo);
    region_info.SetMapped(MemoryRegionInfo::eNo);
  }
} else {
  // We got an invalid address range back
  error.SetErrorString("Server returned invalid range");
}

[xiaobai]

Added unit tests for the method and changed the logic according to clayborg's suggestion.

[xiaobai]

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

1506–1510 ↗

(On Diff #93427)

Thanks for pointing that out!

[clayborg]

Looks good.

[labath]

Thank you for adding the test. Lgtm, assuming the second getmemoryregioninfo call is accidental.

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

373 ↗

(On Diff #93566)

This seems out of place. Should we delete it?

[xiaobai]

I'll resubmit after removing the extra code in the test and get somebody to commit for me.

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

373 ↗

(On Diff #93566)

Whoops! I was going to extend this test further but I decided not to in the end and forgot to remove this. Thanks for pointing it out.

[xiaobai]

Removed extra code in unit test

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

if (SendPacketAndWaitForResponse(packet, response, false) ==
           StringExtractorGDBRemote error_extractor(value);
           std::string error_string;
           // Now convert the HEX bytes into a string value
           error_extractor.GetHexByteString(error_string);
           error.SetErrorString(error_string.c_str());
         }
       }
 
+      if (region_info.GetRange().IsValid()) {
         // We got a valid address range back but no permissions -- which means
         // this is an unmapped page
-      if (region_info.GetRange().IsValid() && saw_permissions == false) {
+        if (!saw_permissions) {
           region_info.SetReadable(MemoryRegionInfo::eNo);
           region_info.SetWritable(MemoryRegionInfo::eNo);
           region_info.SetExecutable(MemoryRegionInfo::eNo);
           region_info.SetMapped(MemoryRegionInfo::eNo);
         }
       } else {
+        // We got an invalid address range back
+        error.SetErrorString("Server returned invalid range");
+      }
+    } else {
       m_supports_memory_region_info = eLazyBoolNo;
     }
   }
 
   if (m_supports_memory_region_info == eLazyBoolNo) {
     error.SetErrorString("qMemoryRegionInfo is not supported");
   }
   if (error.Fail())

lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

 //===-- GDBRemoteCommunicationClientTest.cpp --------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #include <future>
 
 #include "GDBRemoteTestUtils.h"
 
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/StructuredData.h"
+#include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/DataBuffer.h"
 
 #include "llvm/ADT/ArrayRef.h"
 
 using namespace lldb_private::process_gdb_remote;
 using namespace lldb_private;
 using namespace lldb;
 using namespace llvm;
TEST_F(GDBRemoteCommunicationClientTest, SendSignalsToIgnore) {
 
   result = std::async(std::launch::async, [&] {
     return client.SendSignalsToIgnore(std::vector<int32_t>());
   });
 
   HandlePacket(server, "QPassSignals:", "OK");
   EXPECT_TRUE(result.get().Success());
 }
+
+TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfo) {
+  TestClient client;
+  MockServer server;
+  Connect(client, server);
+  if (HasFailure())
+    return;
+
+  const lldb::addr_t addr = 0xa000;
+  MemoryRegionInfo region_info;
+  std::future<Error> result = std::async(std::launch::async, [&] {
+    return client.GetMemoryRegionInfo(addr, region_info);
+  });
+
+  // name is: /foo/bar.so
+  HandlePacket(server,
+      "qMemoryRegionInfo:a000",
+      "start:a000;size:2000;permissions:rx;name:2f666f6f2f6261722e736f;");
+  EXPECT_TRUE(result.get().Success());
+
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfoInvalidResponse) {
+  TestClient client;
+  MockServer server;
+  Connect(client, server);
+  if (HasFailure())
+    return;
+
+  const lldb::addr_t addr = 0x4000;
+  MemoryRegionInfo region_info;
+  std::future<Error> result = std::async(std::launch::async, [&] {
+    return client.GetMemoryRegionInfo(addr, region_info);
+  });
+
+  HandlePacket(server, "qMemoryRegionInfo:4000", "start:4000;size:0000;");
+  EXPECT_FALSE(result.get().Success());
+}