Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

createdump can read memory with process_vm_readv on Linux #420

Merged
merged 2 commits into from
Jan 28, 2020
+19 −1
Merged

createdump can read memory with process_vm_readv on Linux #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3abc94d
Use process_vm_readv when function exists
nxtn Nov 30, 2019
917cc6f
Cast result to ULONG32
nxtn Dec 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/coreclr/src/debug/createdump/createdump.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ typedef int T_CONTEXT;
#include <sys/user.h>
#include <sys/wait.h>
#include <sys/procfs.h>
#ifdef HAVE_PROCESS_VM_READV
#include <sys/uio.h>
#endif
#include <dirent.h>
#include <fcntl.h>
#include <elf.h>
Expand Down
14 changes: 13 additions & 1 deletion src/coreclr/src/debug/createdump/datatarget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,28 @@
DumpDataTarget::DumpDataTarget(pid_t pid) :
m_ref(1),
m_pid(pid),
#ifndef HAVE_PROCESS_VM_READV
m_fd(-1),
#endif
m_crashInfo(nullptr)
{
}

DumpDataTarget::~DumpDataTarget()
{
#ifndef HAVE_PROCESS_VM_READV
if (m_fd != -1)
{
close(m_fd);
m_fd = -1;
}
#endif
}

bool
DumpDataTarget::Initialize(CrashInfo * crashInfo)
{
#ifndef HAVE_PROCESS_VM_READV
char memPath[128];
_snprintf_s(memPath, sizeof(memPath), sizeof(memPath), "/proc/%lu/mem", m_pid);

Expand All @@ -35,6 +40,7 @@ DumpDataTarget::Initialize(CrashInfo * crashInfo)
fprintf(stderr, "open(%s) FAILED %d (%s)\n", memPath, errno, strerror(errno));
return false;
}
#endif
m_crashInfo = crashInfo;
return true;
}
Expand Down Expand Up @@ -149,14 +155,20 @@ DumpDataTarget::ReadVirtual(
/* [in] */ ULONG32 size,
/* [optional][out] */ ULONG32 *done)
{
#ifdef HAVE_PROCESS_VM_READV
iovec local{ buffer, size };
iovec remote{ (void*)(ULONG_PTR)address, size };
ssize_t read = process_vm_readv(m_pid, &local, 1, &remote, 1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssize_t is a signed, 64-bit integer; ULONG32 is an unsigned, 32-bit integer. It's unlikely that read will be larger than size as that's the quantity passed as a parameter, however assigning read to *done will at least generate a warning.

(Using 32-bit integers to denote sizes is something we should fix throughout the runtime. It should use size_t throughout.)

Copy link
Contributor

@lpereira lpereira Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, are there any places where it would be beneficial to pass an iovec array rather than just 1 element at a time to process_vm_readv()? (There could be a fallback method to go through each one and read the memory on OSes that do not offer this particular syscall or a similar feature.)

It might be a good idea to #ifdef this for Linux only, or check if the syscall exist during build-time. For instance, the FreeBSD procfs supports /proc/<PID>/mem but does not have this syscall, so this breaks createdump on FreeBSD. On systems without process_vm_readv() but with a compatible procfs, you might benefit from using readv() instead of pread() and have similar speedups if the suggestion above is possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may even be Linux distros that don't support process_vm_readv so a build-time check (like all the other HAVE_xxxx defines) should be added falling back to the proc//mem code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Distro Version Kernel
RHEL 7.0 3.10
CentOS 7.0 3.10
Oracle Linux 7.0 3.10
Fedora 29 4.18
Debian 9 4.9
Ubuntu 16.04 4.4
Linux Mint 18 4.4
openSUSE 15 4.12
SLES 12 SP2 4.4
Alpline Linux 3.8 4.14

There may even be Linux distros that don't support process_vm_readv

All supported Linux distros meet the minimum version requirement, so I assume it only needs to check for Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, the FreeBSD procfs supports /proc/<PID>/mem

The createdump doc says /proc doesn't work on FreeBSD. Can you confirm it? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, are there any places where it would be beneficial to pass an iovec array rather than just 1 element at a time to process_vm_readv()?

There are some parallel read operations, but they are invoked on ICLRDataTarget instead of DumpDataTarget. It's not easy to extend that interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote that createdump doc 2 or 3 years ago and at the time, it didn't look like FreeBSD supported /proc//mem but now I'm not sure.

#else
assert(m_fd != -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikem8361 Any reason these are not runtime-like asserts?

ssize_t read = pread64(m_fd, buffer, size, (off64_t)(ULONG_PTR)address);
#endif
if (read == -1)
{
*done = 0;
return E_FAIL;
}
*done = read;
*done = (ULONG32)read;
return S_OK;
}

Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/debug/createdump/datatarget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ class DumpDataTarget : public ICLRDataTarget
private:
LONG m_ref; // reference count
pid_t m_pid; // process id
#ifndef HAVE_PROCESS_VM_READV
int m_fd; // /proc/<pid>/mem handle
#endif
CrashInfo* m_crashInfo;

public:
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/pal/src/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ check_function_exists(semget HAS_SYSV_SEMAPHORES)
check_function_exists(pthread_mutex_init HAS_PTHREAD_MUTEXES)
check_function_exists(ttrace HAVE_TTRACE)
check_function_exists(pipe2 HAVE_PIPE2)
check_function_exists(process_vm_readv HAVE_PROCESS_VM_READV)

check_cxx_source_compiles("
#include <pthread_np.h>
Expand Down