linux: Use pread64() instead of pread() in ProcessMemory [crashpad/crashpad : master]
244 views
Skip to first unread message
Mark Mentovai (Gerrit)
Apr 7, 2017, 6:09:49 PM4/7/17
to Joshua Peraza, crashp...@chromium.org
Mark Mentovai posted comments on this change.
Patch set 3:Commit-Queue +1
This was fun. It was pretty easy to work out the fix, but pretty involved to figure out why it wasn’t working.
To view, visit change 472048. To unsubscribe, visit settings.
Joshua Peraza (Gerrit)
Apr 7, 2017, 6:29:07 PM4/7/17
to Mark Mentovai, Commit Bot, crashp...@chromium.org
Joshua Peraza posted comments on this change.
Patch set 3:Code-Review +1
Mark Mentovai (Gerrit)
Apr 7, 2017, 10:41:17 PM4/7/17
to Joshua Peraza, Commit Bot, crashp...@chromium.org
Mark Mentovai merged this change.
Approvals:
Joshua Peraza: Looks good to me
linux: Use pread64() instead of pread() in ProcessMemory
This fixes ProcessMemory for 32-bit processes. All ProcessMemory tests
were failing on 32-bit ARM on Android like this:
[ RUN ] ProcessMemory.ReadSelf
[17345:17345:20170407,172222.579687:ERROR process_memory.cc:55] pread: Invalid argument (22)
../../../../util/linux/process_memory_test.cc:73: Failure
Value of: memory.Read(address, region_size_, result.get())
Actual: false
Expected: true
[ FAILED ] ProcessMemory.ReadSelf (5 ms)
Contemporary Linux doesn’t provide a pread() system call, it provides
pread64(), which operates on off64_t. pread() is a user-space wrapper
that accepts off_t. See Android 7.1.1
bionic/libc/bionic/legacy_32_bit_support.cpp pread().
Note that off_t is a signed type. With a 32-bit off_t, when the
“offset” parameter to pread() has its high bit set, it will be
sign-extended into the 64-bit off64_t, and when interpreted as a memory
address by virtue of being used as an offset into /proc/pid/mem, the
value will take on an incorrect meaning. In fact, the kernel will reject
it outright for its negativity. See linux-4.9.20/fs/read_write.c
[sys_]pread64().
Since ProcessMemory accepts its address parameter as a LinuxVMAddress,
which is wisely a uint64_t, it converts to off64_t properly, retaining
its original value.
Note, however, that the pread64() mechanism evidently cannot read memory
in the high half of a process’ address space even when pread64() is used
throughout. Most importantly, the (pos < 0) check in the kernel will be
tripped. Less importantly, the conversion of our unsigned LinuxVMAddress
to pread64’s signed off64_t, with the high bit set, is not defined. This
is not an immediate practical problem. With the exception of possible
shared pages mapped from kernel space (I only see this for the vsyscall
page on x86_64), Linux restricts 64-bit user process’ address space to
at least the lower half of the addressable range, with the high bit
clear. (The limit of the user address space is
linux-4.9.20/arch/x86/include/asm/processor.h TASK_SIZE_MAX =
0x7ffffffff000 for x86_64 and
linux-4.9.20/arch/arm64/include/asm/memory.h TASK_SIZE_64 =
0x1000000000000 at maximum for arm64.)
The 32-bit off_t may be a surprise, because
third_party/mini_chromium/mini_chromium/build/common.gypi sets
_FILE_OFFSET_BITS=64. Altough this macro is considered in the NDK’s
“unified headers”, in the classic NDK, this macro is never consulted.
Instead, off_t is always “long”, and pread() always gets the
compatibility shim in Bionic.
Bug: crashpad:30
Change-Id: Id00c882a3d521a46ef3fc0060d03ea0ab9493175
Reviewed-on: https://chromium-review.googlesource.com/472048
Reviewed-by: Joshua Peraza <jpe...@chromium.org>
---
M util/linux/process_memory.cc
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/util/linux/process_memory.cc b/util/linux/process_memory.cc
index 72b05bf..8f3290a 100644
--- a/util/linux/process_memory.cc
+++ b/util/linux/process_memory.cc
@@ -50,9 +50,9 @@
char* buffer_c = static_cast<char*>(buffer);
while (size > 0) {
ssize_t bytes_read =
- HANDLE_EINTR(pread(mem_fd_.get(), buffer_c, size, address));
+ HANDLE_EINTR(pread64(mem_fd_.get(), buffer_c, size, address));
if (bytes_read < 0) {
- PLOG(ERROR) << "pread";
+ PLOG(ERROR) << "pread64";
return false;
}
if (bytes_read == 0) {
@@ -95,9 +95,10 @@
read_size = sizeof(buffer);
}
ssize_t bytes_read;
- bytes_read = HANDLE_EINTR(pread(mem_fd_.get(), buffer, read_size, address));
+ bytes_read =
+ HANDLE_EINTR(pread64(mem_fd_.get(), buffer, read_size, address));
if (bytes_read < 0) {
- PLOG(ERROR) << "pread";
+ PLOG(ERROR) << "pread64";
return false;
}
if (bytes_read == 0) {
To view, visit change 472048. To unsubscribe, visit settings.
Reply all
Reply to author
Forward
0 new messages