[NativeProcessLinux] Reuse memory read by process_vm_readv before calling ptrace

[aadsm]

Summary

I'm putting this up as discussed per D62503.
Today when process_vm_readv fails to read the entire range we fallback to ptrace and try to read the same range.
However, process_vm_readv does partial reads so there's no need to read again what has already been read by process_vm_readv.

[labath]

Thinking about tests, do you know of any way to create memory which is readable by ptrace, but it is not accessible via process_vm_readv ? I know that latest android phones have memory like that, but I believe this depends on selinux or some other mechanism which cannot be reproduced in a test.

If we can't do anything like that, we should at least have an lldb-server test that performs a read ($m packet) crossing page boundaries, where one of the pages is unmapped. Probably the easiest way to guarantee that a particular page will be unmapped is to first mmap 2 pages worth of memory, and then munmap one of them.

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

1457

llvm's policy is to *not* "almost always use auto". And it's nice to know whether this returns size_t, ssize_t, or something else without looking up the documentation.

1467

addr += vm_bytes_read ?
Since this is talking about addresses in the other process, I can't imagine that casting to a char * is going to make this more "correct" in any way.

[aadsm]

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

1457

Makes sense. I'll make sure to take a more careful read at https://llvm.org/docs/CodingStandards.html.

1467

For some reason I thought that would increase addr by vm_bytes_read*4 or 8 but it's really just a uint64_t not a pointer. I'll also update ReadCStringFromMemory function on my other diff then, I believe I make the same logic there.

[aadsm]

That's a good idea for the tests, will look into that.

[labath]

In D62715#1525085, @aadsm wrote:

That's a good idea for the tests, will look into that.

Actually, it looks like getting process_vm_readv to fail is as simple as running mprotect(..., PROT_NONE) on the piece of memory. Here's my test program, in case you find it useful for anything:

#include <cassert>
#include <cerrno>
#include <cstdio>
#include <cstring>
#include <sys/mman.h>
#include <sys/prctl.h>
#include <sys/ptrace.h>
#include <sys/uio.h>
#include <sys/wait.h>
#include <unistd.h>

long *mem;

void child() {
  assert(ptrace(PTRACE_TRACEME, 0, 0, 0) == 0);
  raise(SIGSTOP);
  _exit(0);
}

void parent(pid_t child) {
  int status;
  assert(waitpid(child, &status, __WALL) == child);
  assert(WIFSTOPPED(status));
  assert(WSTOPSIG(status) == SIGSTOP);
  unsigned long myx;
  struct iovec local;
  local.iov_base = &myx;
  local.iov_len = sizeof myx;

  struct iovec remote;
  remote.iov_base = mem;
  remote.iov_len = sizeof myx;

  ssize_t s = process_vm_readv(child, &local, 1, &remote, 1, 0);
  fprintf(stderr, "readv: %zd (%d - %s)\n", s, errno, strerror(errno));
  myx = ptrace(PTRACE_PEEKDATA, child, mem, 0);
  fprintf(stderr, "peek: %lx (%d - %s)\n", myx, errno, strerror(errno));
  _exit(0);
}

int main() {
  mem = reinterpret_cast<long *>(mmap(nullptr, 0x1000, PROT_READ | PROT_WRITE,
                                      MAP_PRIVATE | MAP_ANONYMOUS, 0, 0));
  assert(mem != MAP_FAILED);
  *mem = 0x424344454647;
  assert(mprotect(mem, 0x1000, PROT_NONE) == 0);

  pid_t pid = fork();
  assert(pid != -1);
  if (pid)
    parent(pid);
  else
    child();
}


$ g++ a.cc
$ ./a.out 
readv: -1 (14 - Bad address)
peek: 424344454647 (0 - Success)

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

case llvm::Triple::arm:
     }
   default:
     return NativeProcessProtocol::GetSoftwareBreakpointTrapOpcode(size_hint);
   }
 }
 
 Status NativeProcessLinux::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
                                       size_t &bytes_read) {
+  bytes_read = 0;
   if (ProcessVmReadvSupported()) {
     // The process_vm_readv path is about 50 times faster than ptrace api. We
     // want to use this syscall if it is supported.
 
     const ::pid_t pid = GetID();
 
     struct iovec local_iov, remote_iov;
     local_iov.iov_base = buf;
     local_iov.iov_len = size;
     remote_iov.iov_base = reinterpret_cast<void *>(addr);
     remote_iov.iov_len = size;
 
-    bytes_read = process_vm_readv(pid, &local_iov, 1, &remote_iov, 1, 0);
-    const bool success = bytes_read == size;
+    auto vm_bytes_read =
+        process_vm_readv(pid, &local_iov, 1, &remote_iov, 1, 0);
+    const bool success = vm_bytes_read == size;
+    if (vm_bytes_read > 0) {
+      bytes_read = vm_bytes_read;
+      // When process_vm_readv is not able to read all the data we fallback to
+      // ptrace. However, if process_vm_readv didn't fail (vm_bytes_read != -1)
+      // and was able to some bytes avoid re-reading them with ptrace by
+      // updating the addr and size to point to the rest of the data.
+      if (!success) {
+        addr = reinterpret_cast<addr_t>(reinterpret_cast<char *>(addr) +
+                                        vm_bytes_read);
+        size -= vm_bytes_read;
+      }
+    }
 
     Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
     LLDB_LOG(log,
              "using process_vm_readv to read {0} bytes from inferior "
              "address {1:x}: {2}",
              size, addr, success ? "Success" : llvm::sys::StrError(errno));
 
     if (success)
       return Status();
     // else the call failed for some reason, let's retry the read using ptrace
     // api.
   }
 
-  unsigned char *dst = static_cast<unsigned char *>(buf);
+  unsigned char *dst = static_cast<unsigned char *>(buf) + bytes_read;
   size_t remainder;
   long data;
 
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_MEMORY));
   LLDB_LOG(log, "addr = {0}, buf = {1}, size = {2}", addr, buf, size);
 
-  for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
+  for (; bytes_read < size; bytes_read += remainder) {
     Status error = NativeProcessLinux::PtraceWrapper(
         PTRACE_PEEKDATA, GetID(), (void *)addr, nullptr, 0, &data);
     if (error.Fail())
       return error;
 
     remainder = size - bytes_read;
     remainder = remainder > k_ptrace_word_size ? k_ptrace_word_size : remainder;