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

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

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

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());
         }
       }
 
+      // We got an invalid address range back
+      if (!region_info.GetRange().IsValid()) {
+        error.SetErrorString("Server returned invalid range");
+      }
+
       // 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) {
         region_info.SetReadable(MemoryRegionInfo::eNo);
         region_info.SetWritable(MemoryRegionInfo::eNo);
         region_info.SetExecutable(MemoryRegionInfo::eNo);
         region_info.SetMapped(MemoryRegionInfo::eNo);
       }