Add a bit of error checking to SBProcess::ReadMemory to check if a nullptr destination buffer was provided, return error

[jasonmolenda]

Summary

I helped a developer using SB API yesterday who had a bug and tried to read 0xffffffffffffffe8 with SBProcess::ReadMemory. swig would try to malloc a buffer for that, which would fail but was not checked, and hand the nullptr to ReadMemory. lldb would continue on until it tried to copy bytes into the nullptr and crash.

Fixing the script was obvious, but having lldb crash out from under the script made it a little harder to debug. I think we should check for a null buffer at the SBProcess API point and return an error there.

Added a quick test to the ProcessAPI test. This call will cause a huge malloc which should safely fail on every platform I think? We may need to revise if this ends up crashing lldb on some target I guess.

[JDevlieghere]

LGTM modulo inline comments

lldb/source/API/SBProcess.cpp

807–811

Nit: this isn't using bytes_read so I'd move the new code above it.

lldb/test/API/python_api/process/TestProcessAPI.py

82–84

self.assertFalse(error.Success(), "SBProcessReadMemory claims to have successfully read 0xffffffffffffffe8 bytes")

[mib]

LGTM with @JDevlieghere comments.

lldb/source/API/SBProcess.cpp

807–811

+1

[jasonmolenda]

Update patch for Jonas' suggestions.

lldb/source/API/SBProcess.cpp

const char *SBProcess::GetBroadcasterClass() {
 
   return Process::GetStaticBroadcasterClass().AsCString();
 }
 
 size_t SBProcess::ReadMemory(addr_t addr, void *dst, size_t dst_len,
                              SBError &sb_error) {
   LLDB_INSTRUMENT_VA(this, addr, dst, dst_len, sb_error);
 
-  size_t bytes_read = 0;
+  if (!dst) {
+    sb_error.SetErrorStringWithFormat(
+        "no buffer provided to read %zu bytes into", dst_len);
+    return 0;
+  }
 
+  size_t bytes_read = 0;
   ProcessSP process_sp(GetSP());
 
 
   if (process_sp) {
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       std::lock_guard<std::recursive_mutex> guard(
           process_sp->GetTarget().GetAPIMutex());

lldb/test/API/python_api/process/TestProcessAPI.py

def test_read_memory(self):
             print("memory content:", content)
 
         self.expect(
             content,
             "Result from SBProcess.ReadMemory() matches our expected output: 'x'",
             exe=False,
             startstr=b'x')
 
+        # Try to read an impossibly large amount of memory; swig
+        # will try to malloc it and fail, we should get an error 
+        # result.
+        error = lldb.SBError()
+        content = process.ReadMemory(
+                val.AddressOf().GetValueAsUnsigned(), 
+                0xffffffffffffffe8, error)
+        if error.Success():
+            self.assertFalse(error.Success(), "SBProcessReadMemory claims to have "
+                      "successfully read 0xffffffffffffffe8 bytes")
+        if self.TraceOn():
+            print("Tried to read 0xffffffffffffffe8 bytes, got error message: ",
+                  error.GetCString())
+
         # Read (char *)my_char_ptr.
         val = frame.FindValue("my_char_ptr", lldb.eValueTypeVariableGlobal)
         self.DebugSBValue(val)
         cstring = process.ReadCStringFromMemory(
             val.GetValueAsUnsigned(), 256, error)
         if not error.Success():
             self.fail("SBProcess.ReadCStringFromMemory() failed")
         if self.TraceOn():