[PATCH 0/3] MR8834: server: Use process_vm_writev / process_vm_readv instead of ptrace when available.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/virtual.c | 4 ++- server/ptrace.c | 64 ++++++++++++++++++++++++++++++----- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/dlls/kernel32/tests/virtual.c b/dlls/kernel32/tests/virtual.c index d0553b19966..ecce5683064 100644 --- a/dlls/kernel32/tests/virtual.c +++ b/dlls/kernel32/tests/virtual.c @@ -4546,8 +4546,10 @@ static void test_ReadProcessMemory(void) ok(ret, "ReadProcessMemory failed %lu\n", GetLastError()); ok(copied == si.dwPageSize, "copied = %Id\n", copied); + copied = 1; ret = ReadProcessMemory(hproc, ptr, buf, 2 * si.dwPageSize, &copied); - todo_wine ok(!ret, "ReadProcessMemory succeeded\n"); + todo_wine_if(ret) ok(!ret, "ReadProcessMemory succeeded\n"); + todo_wine_if(ret) ok(!copied, "copied = %Id\n", copied); ret = ReadProcessMemory(hproc, ptr, buf, si.dwPageSize, &copied); ok(ret, "ReadProcessMemory failed %lu\n", GetLastError()); diff --git a/server/ptrace.c b/server/ptrace.c index 955567a704f..404c0a8d12f 100644 --- a/server/ptrace.c +++ b/server/ptrace.c @@ -43,6 +43,10 @@ #ifdef HAVE_SYS_THR_H # include <sys/thr.h> #endif +#if defined(HAVE_SYS_UIO_H) && defined(__NR_process_vm_readv) +# include <sys/uio.h> +#define USE_PROCESS_VM +#endif #include <unistd.h> #include "ntstatus.h" @@ -350,21 +354,45 @@ static struct thread *get_ptrace_thread( struct process *process ) return NULL; } -/* read data from a process memory space */ -int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, char *dest ) +#ifdef USE_PROCESS_VM +static int read_process_memory_vm( struct thread *thread, client_ptr_t ptr, data_size_t size, char *dest ) { - struct thread *thread = get_ptrace_thread( process ); - unsigned int first_offset, last_offset, len; - unsigned long data, *addr; + static int not_supported; + struct iovec local, remote; + ssize_t len; - if (!thread) return 0; - - if ((unsigned long)ptr != ptr) + if (not_supported) return -1; + if (thread->unix_pid == -1 || !is_process_init_done( thread->process )) { set_error( STATUS_ACCESS_DENIED ); return 0; } + local.iov_len = remote.iov_len = size; + local.iov_base = dest; + remote.iov_base = (void *)(unsigned long)ptr; + len = syscall( __NR_process_vm_readv, thread->unix_pid, &local, 1, &remote, 1, 0 ); + if (len < 0 && errno == ENOSYS) + { + not_supported = 1; + return -1; + } + if (len == size) return 1; + set_error( len >= 0 ? STATUS_PARTIAL_COPY : STATUS_ACCESS_DENIED ); + return 0; +} +#else +static int read_process_memory_vm( struct thread *thread, client_ptr_t ptr, data_size_t size, char *dest ) +{ + return -1; +} +#endif + +static int read_process_memory_ptrace( struct thread *thread, client_ptr_t ptr, data_size_t size, char *dest ) +{ + unsigned int first_offset, last_offset, len; + unsigned long data, *addr; + first_offset = ptr % sizeof(long); last_offset = (size + first_offset) % sizeof(long); if (!last_offset) last_offset = sizeof(long); @@ -379,7 +407,7 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t char procmem[24]; int fd; - snprintf( procmem, sizeof(procmem), "/proc/%u/mem", process->unix_pid ); + snprintf( procmem, sizeof(procmem), "/proc/%u/mem", thread->process->unix_pid ); if ((fd = open( procmem, O_RDONLY )) != -1) { ssize_t ret = pread( fd, dest, size, ptr ); @@ -417,6 +445,24 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t return !len; } +/* read data from a process memory space */ +int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, char *dest ) +{ + struct thread *thread = get_ptrace_thread( process ); + int ret; + + if (!thread) return 0; + + if ((unsigned long)ptr != ptr) + { + set_error( STATUS_ACCESS_DENIED ); + return 0; + } + + if ((ret = read_process_memory_vm( thread, ptr, size, dest )) != -1) return ret; + return read_process_memory_ptrace( thread, ptr, size, dest ); +} + /* make sure we can write to the whole address range */ /* len is the total size (in longs) */ static int check_process_write_access( struct thread *thread, long *addr, data_size_t len ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8834
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/virtual.c | 3 ++- server/mach.c | 4 +++- server/process.c | 7 ++++--- server/process.h | 3 ++- server/procfs.c | 4 +++- server/protocol.def | 2 ++ server/ptrace.c | 4 +++- 7 files changed, 19 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 4b4d9d6cd7f..a6dd1f6eded 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -6604,7 +6604,8 @@ NTSTATUS WINAPI NtWriteVirtualMemory( HANDLE process, void *addr, const void *bu req->handle = wine_server_obj_handle( process ); req->addr = wine_server_client_ptr( addr ); wine_server_add_data( req, buffer, size ); - if ((status = wine_server_call( req ))) size = 0; + status = wine_server_call( req ); + size = reply->written; } SERVER_END_REQ; } diff --git a/server/mach.c b/server/mach.c index 19737fceef8..eceb53d1f98 100644 --- a/server/mach.c +++ b/server/mach.c @@ -415,7 +415,8 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t } /* write data to a process memory space */ -int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src ) +int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src, + data_size_t *written ) { kern_return_t ret; mach_port_t process_port = get_process_port( process ); @@ -538,6 +539,7 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t out: free( (void *)data ); mach_set_error( ret ); + if (ret == KERN_SUCCESS && written) *written = size; return (ret == KERN_SUCCESS); } diff --git a/server/process.c b/server/process.c index 8e0f9189a61..436694e592d 100644 --- a/server/process.c +++ b/server/process.c @@ -1143,8 +1143,8 @@ int set_process_debug_flag( struct process *process, int flag ) if (is_wow64_process( process )) peb32 = process->peb + 0x1000; /* BeingDebugged flag is the byte at offset 2 in the PEB */ - if (peb32 && !write_process_memory( process, peb32 + 2, 1, &data )) return 0; - return write_process_memory( process, process->peb + 2, 1, &data ); + if (peb32 && !write_process_memory( process, peb32 + 2, 1, &data, NULL )) return 0; + return write_process_memory( process, process->peb + 2, 1, &data, NULL ); } /* create a new process */ @@ -1798,7 +1798,8 @@ DECL_HANDLER(write_process_memory) if ((process = get_process_from_handle( req->handle, PROCESS_VM_WRITE ))) { data_size_t len = get_req_data_size(); - if (len) write_process_memory( process, req->addr, len, get_req_data() ); + reply->written = 0; + if (len) write_process_memory( process, req->addr, len, get_req_data(), &reply->written ); release_object( process ); } } diff --git a/server/process.h b/server/process.h index 251af45be79..2bf96abb55b 100644 --- a/server/process.h +++ b/server/process.h @@ -137,7 +137,8 @@ extern void init_tracing_mechanism(void); extern void init_process_tracing( struct process *process ); extern void finish_process_tracing( struct process *process ); extern int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, char *dest ); -extern int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src ); +extern int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src, + data_size_t *written ); static inline process_id_t get_process_id( struct process *process ) { return process->id; } diff --git a/server/procfs.c b/server/procfs.c index 6ae0a9006ed..17ff6b166a5 100644 --- a/server/procfs.c +++ b/server/procfs.c @@ -150,7 +150,8 @@ int read_process_memory( struct process *process, client_ptr_t ptr, size_t size, } /* write data to a process memory space */ -int write_process_memory( struct process *process, client_ptr_t ptr, size_t size, const char *src ) +int write_process_memory( struct process *process, client_ptr_t ptr, size_t size, const char *src, + data_size_t *written ) { ssize_t ret; int fd; @@ -165,6 +166,7 @@ int write_process_memory( struct process *process, client_ptr_t ptr, size_t size ret = pwrite( fd, src, size, (off_t)ptr ); close( fd ); + if (ret == size && written) *written = size; if (ret == size) return 1; if (ret == -1) file_set_error(); diff --git a/server/protocol.def b/server/protocol.def index cf92cdbabaa..475a84652ff 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1994,6 +1994,8 @@ struct process_info obj_handle_t handle; /* process handle */ client_ptr_t addr; /* addr to write to */ VARARG(data,bytes); /* data to write */ +(a)REPLY + data_size_t written; /* number of bytes written */ @END diff --git a/server/ptrace.c b/server/ptrace.c index 404c0a8d12f..e065da1f183 100644 --- a/server/ptrace.c +++ b/server/ptrace.c @@ -480,7 +480,8 @@ static int check_process_write_access( struct thread *thread, long *addr, data_s } /* write data to a process memory space */ -int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src ) +int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src, + data_size_t *written ) { struct thread *thread = get_ptrace_thread( process ); int ret = 0; @@ -564,6 +565,7 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t done: resume_after_ptrace( thread ); } + if (ret && written) *written = size; return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8834
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/virtual.c | 19 +++++---- dlls/ntdll/tests/wow64.c | 4 +- server/ptrace.c | 72 ++++++++++++++++++++++++++++------- 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/dlls/kernel32/tests/virtual.c b/dlls/kernel32/tests/virtual.c index ecce5683064..5aa147eb16a 100644 --- a/dlls/kernel32/tests/virtual.c +++ b/dlls/kernel32/tests/virtual.c @@ -151,8 +151,7 @@ static void test_VirtualAllocEx(void) bytes_written = 0xdeadbeef; b = WriteProcessMemory(hProcess, addr1, src, alloc_size, &bytes_written); ok( !b, "WriteProcessMemory succeeded\n" ); - ok( GetLastError() == ERROR_NOACCESS || - GetLastError() == ERROR_PARTIAL_COPY, /* vista */ + ok( GetLastError() == ERROR_PARTIAL_COPY, /* vista */ "wrong error %lu\n", GetLastError() ); ok( bytes_written == 0, "%Iu bytes written\n", bytes_written ); bytes_read = 0xdeadbeef; @@ -211,10 +210,10 @@ static void test_VirtualAllocEx(void) if (!b) ok( GetLastError() == ERROR_NOACCESS, "wrong error %lu\n", GetLastError() ); ok( bytes_written == 0xdeadbeef, "%Iu bytes written\n", bytes_written ); status = pNtWriteVirtualMemory( hProcess, addr1, src, alloc_size, &bytes_written ); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( status == STATUS_PARTIAL_COPY || broken(status == STATUS_ACCESS_VIOLATION), "wrong status %lx\n", status ); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( bytes_written == 0, "%Iu bytes written\n", bytes_written ); b = VirtualProtectEx( hProcess, addr1, alloc_size, PAGE_EXECUTE_READ, &old_prot ); @@ -225,21 +224,21 @@ static void test_VirtualAllocEx(void) ok( bytes_written == alloc_size, "%Iu bytes written\n", bytes_written ); bytes_written = 0xdeadbeef; status = pNtWriteVirtualMemory( hProcess, addr1, src, alloc_size, &bytes_written ); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( status == STATUS_PARTIAL_COPY || broken(status == STATUS_ACCESS_VIOLATION), "wrong status %lx\n", status ); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( bytes_written == 0, "%Iu bytes written\n", bytes_written ); b = VirtualProtectEx( hProcess, addr1, 0x2000, PAGE_EXECUTE_READWRITE, &old_prot ); ok( b, "VirtualProtectEx, error %lu\n", GetLastError() ); bytes_written = 0xdeadbeef; b = WriteProcessMemory(hProcess, addr1, src, alloc_size, &bytes_written); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( !b || broken(b), /* <= win10 1507 */ "WriteProcessMemory succeeded\n" ); bytes_written = 0xdeadbeef; status = pNtWriteVirtualMemory( hProcess, addr1, src, alloc_size, &bytes_written ); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( status == STATUS_PARTIAL_COPY || broken(status == STATUS_SUCCESS), /* <= win10 1507 */ "wrong status %lx\n", status ); ok( bytes_written == (status ? 0x2000 : alloc_size), "%Iu bytes written\n", bytes_written ); @@ -248,10 +247,10 @@ static void test_VirtualAllocEx(void) ok( b, "VirtualProtectEx, error %lu\n", GetLastError() ); bytes_written = 0xdeadbeef; b = WriteProcessMemory(hProcess, addr1, src, alloc_size, &bytes_written); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( !b || broken(b), /* <= win10 1507 */ "WriteProcessMemory succeeded\n" ); status = pNtWriteVirtualMemory( hProcess, addr1, src, alloc_size, &bytes_written ); - todo_wine + todo_wine_if(b) ok( status == STATUS_PARTIAL_COPY || broken(status == STATUS_SUCCESS), /* <= win10 1507 */ "wrong status %lx\n", status ); ok( bytes_written == (status ? 0x2000 : alloc_size), "%Iu bytes written\n", bytes_written ); diff --git a/dlls/ntdll/tests/wow64.c b/dlls/ntdll/tests/wow64.c index 544290c3273..824fc45fe9f 100644 --- a/dlls/ntdll/tests/wow64.c +++ b/dlls/ntdll/tests/wow64.c @@ -2492,10 +2492,10 @@ static void test_nt_wow64(void) MEM_RESERVE | MEM_COMMIT, PAGE_READONLY ); ok( !status, "NtWow64AllocateVirtualMemory64 failed %lx\n", status ); status = pNtWow64WriteVirtualMemory64( process, ptr, str, sizeof(str), &res ); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( status == STATUS_PARTIAL_COPY || broken( status == STATUS_ACCESS_VIOLATION ), "NtWow64WriteVirtualMemory64 failed %lx\n", status ); - todo_wine + todo_wine_if(status == STATUS_SUCCESS) ok( !res || broken(res) /* win10 1709 */, "wrong size %s\n", wine_dbgstr_longlong(res) ); } ptr = 0x9876543210ull; diff --git a/server/ptrace.c b/server/ptrace.c index e065da1f183..16ec91ee978 100644 --- a/server/ptrace.c +++ b/server/ptrace.c @@ -43,7 +43,7 @@ #ifdef HAVE_SYS_THR_H # include <sys/thr.h> #endif -#if defined(HAVE_SYS_UIO_H) && defined(__NR_process_vm_readv) +#if defined(HAVE_SYS_UIO_H) && defined(__NR_process_vm_readv) && defined(__NR_process_vm_writev) # include <sys/uio.h> #define USE_PROCESS_VM #endif @@ -381,11 +381,46 @@ static int read_process_memory_vm( struct thread *thread, client_ptr_t ptr, data set_error( len >= 0 ? STATUS_PARTIAL_COPY : STATUS_ACCESS_DENIED ); return 0; } + +static int write_process_memory_vm( struct thread *thread, client_ptr_t ptr, data_size_t size, const char *src, + data_size_t *written ) +{ + static int not_supported; + struct iovec local, remote; + ssize_t len; + + if (not_supported) return -1; + if (thread->unix_pid == -1 || !is_process_init_done( thread->process )) + { + set_error( STATUS_ACCESS_DENIED ); + return 0; + } + + local.iov_len = remote.iov_len = size; + local.iov_base = (void *)src; + remote.iov_base = (void *)(unsigned long)ptr; + len = syscall( __NR_process_vm_writev, thread->unix_pid, &local, 1, &remote, 1, 0 ); + if (len < 0 && errno == ENOSYS) + { + not_supported = 1; + return -1; + } + + if (written) *written = max( len, 0 ); + if (len != size) set_error( STATUS_PARTIAL_COPY ); + return len == size; +} #else static int read_process_memory_vm( struct thread *thread, client_ptr_t ptr, data_size_t size, char *dest ) { return -1; } + +static int write_process_memory_vm( struct thread *thread, client_ptr_t ptr, data_size_t size, const char *src, + data_size_t *written ) +{ + return -1; +} #endif static int read_process_memory_ptrace( struct thread *thread, client_ptr_t ptr, data_size_t size, char *dest ) @@ -479,25 +514,15 @@ static int check_process_write_access( struct thread *thread, long *addr, data_s return (write_thread_long( thread, addr + len - 1, 0, 0 ) != -1); } -/* write data to a process memory space */ -int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src, - data_size_t *written ) +static int write_process_memory_ptrace( struct thread *thread, client_ptr_t ptr, data_size_t size, const char *src, + data_size_t *written ) { - struct thread *thread = get_ptrace_thread( process ); int ret = 0; long data = 0; data_size_t len; long *addr; unsigned long first_mask, first_offset, last_mask, last_offset; - if (!thread) return 0; - - if ((unsigned long)ptr != ptr) - { - set_error( STATUS_ACCESS_DENIED ); - return 0; - } - /* compute the mask for the first long */ first_mask = ~0; first_offset = ptr % sizeof(long); @@ -525,7 +550,7 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t char procmem[24]; int fd; - snprintf( procmem, sizeof(procmem), "/proc/%u/mem", process->unix_pid ); + snprintf( procmem, sizeof(procmem), "/proc/%u/mem", thread->process->unix_pid ); if ((fd = open( procmem, O_WRONLY )) != -1) { ssize_t r = pwrite( fd, src, size, ptr ); @@ -569,6 +594,25 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t return ret; } +/* write data to a process memory space */ +int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src, + data_size_t *written ) +{ + struct thread *thread = get_ptrace_thread( process ); + int ret; + + if (!thread) return 0; + + if ((unsigned long)ptr != ptr) + { + set_error( STATUS_ACCESS_DENIED ); + return 0; + } + + if ((ret = write_process_memory_vm( thread, ptr, size, src, written )) != -1) return ret; + return write_process_memory_ptrace( thread, ptr, size, src, written ); +} + /* retrieve an LDT selector entry */ void get_selector_entry( struct thread *thread, int entry, unsigned int *base, unsigned int *limit, unsigned char *flags ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8834
The primary motivation is performance, suspending the process with ptrace or using /proc/pid/mem is very slow. As a side effect, proc_vm_ path behaves like modern windows WRT partial writes. It is harder to do for fallback path as requires explicit permission check (looks like ptrace and /proc/pid/mem don't favour read and write page protection) and will probably make it even slower, so I left that as is. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8834#note_113807
There are suspicious test failures right around the functions which do involve ReadProcessMemory for another process. I could not reproduce locally and don't yet understand how / if the change can be causing that but going to look more tomorrow, marking MR as draft for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8834#note_113820
participants (2)
-
Paul Gofman -
Paul Gofman (＠gofman)