[PATCH] ntdll: Add a shortcut to NtReadVirtualMemory for the local process
EnumProcessModules uses ReadProcessMemory (which forwards to NtReadVirtualMemory) for reading the tables of a process, even for the current process. When running wine within docker, NtReadVirtualMemory currently fails unless the SYS_PTRACE capability is explicitly added. LLVM's libunwind uses EnumProcessModules for locating .eh_frame sections in all loaded modules when unwinding dwarf exceptions (used on i686). As it is used for locating sections within the current process, libunwind only calls EnumProcessModules for the current process. Therefore, exception handling in executables with libunwind on i686 doesn't work when run with wine within docker. Even for setups where NtReadVirtualMemory is allowed, it is significantly slower than just reading the memory directly within the process. Currently, handling 10000 thrown exceptions with libunwind on i686 takes 24 seconds when run in wine, while it runs in around 1.4 seconds with this patch. Signed-off-by: Martin Storsjo <martin(a)martin.st> --- dlls/ntdll/virtual.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c index d15b49f6fd..566b5982a7 100644 --- a/dlls/ntdll/virtual.c +++ b/dlls/ntdll/virtual.c @@ -3521,6 +3521,26 @@ NTSTATUS WINAPI NtReadVirtualMemory( HANDLE process, const void *addr, void *buf if (virtual_check_buffer_for_write( buffer, size )) { + if (process == GetCurrentProcess()) + { + sigset_t sigset; + + server_enter_uninterrupted_section( &csVirtual, &sigset ); + if (virtual_check_buffer_for_read( addr, size )) + { + memcpy(buffer, addr, size); + status = STATUS_SUCCESS; + } + else + { + size = 0; + status = STATUS_ACCESS_VIOLATION; + } + server_leave_uninterrupted_section( &csVirtual, &sigset ); + if (bytes_read) *bytes_read = size; + return status; + } + SERVER_START_REQ( read_process_memory ) { req->handle = wine_server_obj_handle( process ); -- 2.17.1
On Tue, Oct 29, 2019 at 9:04 AM Martin Storsjo <martin(a)martin.st> wrote:
... Therefore, exception handling in executables with libunwind on i686 doesn't work when run with wine within docker.
I personally don't think this is a great rationale for changing the behavior of wine. It sounds like a rationale for docker to allow SYS_PTRACE. But I am not a wine maintainer, so my only real concern is that the correctness of NtReadVirtualMemory is not affected as it is used by anti-cheat and other programs.
... + server_enter_uninterrupted_section( &csVirtual, &sigset ); + if (virtual_check_buffer_for_read( addr, size )) + { + memcpy(buffer, addr, size); + status = STATUS_SUCCESS; + }
This does not have the same semantics as the original code. The original code used kernel system calls (e.g. pread) which will not cause a page fault. virtual_check_buffer_for_read explicitly reads each page to trigger a fault. At a minimum, this causes the behavior to diverge from Windows for guard pages. It may have other differences as well. A potential fix may be to implement something similar to virtual_uninterrupted_read_memory. Or use kernel system calls (e.g. read, write, pread, etc.) to access the memory, which will not cause a page fault. In any case, I think more testing is needed. Here is a sample program using VPROT_GUARD that triggers divergent behavior: #include <windows.h> #include <stdio.h> typedef NTSTATUS WINAPI (*pNtReadVirtualMemory)( HANDLE, const void *, void *, SIZE_T, SIZE_T *); int main() { char tmp[64]; char *p; HANDLE ntdll; NTSTATUS status; SIZE_T bytes_read; ntdll = GetModuleHandle("ntdll"); pNtReadVirtualMemory NtReadVirtualMemory = (pNtReadVirtualMemory)GetProcAddress(ntdll, "NtReadVirtualMemory"); p = (char *)VirtualAlloc(NULL, 0x10000, MEM_COMMIT | MEM_RESERVE, PAGE_GUARD | PAGE_READWRITE); printf("p = %p\n", p); status = NtReadVirtualMemory(GetCurrentProcess(), p, tmp, sizeof(tmp), &bytes_read); printf("status = %x\n", status); status = NtReadVirtualMemory(GetCurrentProcess(), p, tmp, sizeof(tmp), &bytes_read); printf("status = %x\n", status); return 0; } -Andrew
On Tue, 29 Oct 2019, Andrew Wesie wrote:
On Tue, Oct 29, 2019 at 9:04 AM Martin Storsjo <martin(a)martin.st> wrote:
... Therefore, exception handling in executables with libunwind on i686 doesn't work when run with wine within docker.
I personally don't think this is a great rationale for changing the behavior of wine. It sounds like a rationale for docker to allow SYS_PTRACE.
It can be enabled, but it's not generally enabled by default. In any case: I believe it is fair to hope that EnumProcessModules(GetCurrentProcess()) would work, without privileges that let you read the memory of other processes. Exactly how to achieve that is deabatable though. The same end result can be achieved by adding a special case for the local process within EnumProcessModules itself as well. That leads to a bit more code duplication, and only gives this benefit to exactly this one function and nothing else though.
But I am not a wine maintainer, so my only real concern is that the correctness of NtReadVirtualMemory is not affected as it is used by anti-cheat and other programs.
That's a fair concern. I wasn't very familiar with this function from the beginning, and this was my initial attempt at achieving my goal with a minimum of code duplication and giving the same benefit to other introspection functions as well (in case there are any).
... + server_enter_uninterrupted_section( &csVirtual, &sigset ); + if (virtual_check_buffer_for_read( addr, size )) + { + memcpy(buffer, addr, size); + status = STATUS_SUCCESS; + }
This does not have the same semantics as the original code. The original code used kernel system calls (e.g. pread) which will not cause a page fault. virtual_check_buffer_for_read explicitly reads each page to trigger a fault. At a minimum, this causes the behavior to diverge from Windows for guard pages. It may have other differences as well.
A potential fix may be to implement something similar to virtual_uninterrupted_read_memory.
FWIW, I tried using virtual_uninterrupted_read_memory for doing the reading in NtReadVirtualMemory if the target process was the current one, but that turned out to fail on some of the memory regions that EnumProcessModules reads, as some of the views it reads have VPROT_SYSTEM set, and virtual_uninterrupted_read_memory doesn't read those. I'm not familiar with these internals to have much clue about what to make of that though. // Martin
Martin Storsjö <martin(a)martin.st> writes:
On Tue, 29 Oct 2019, Andrew Wesie wrote:
On Tue, Oct 29, 2019 at 9:04 AM Martin Storsjo <martin(a)martin.st> wrote:
... Therefore, exception handling in executables with libunwind on i686 doesn't work when run with wine within docker.
I personally don't think this is a great rationale for changing the behavior of wine. It sounds like a rationale for docker to allow SYS_PTRACE.
It can be enabled, but it's not generally enabled by default.
In any case: I believe it is fair to hope that EnumProcessModules(GetCurrentProcess()) would work, without privileges that let you read the memory of other processes.
Not being able to use ptrace isn't a very interesting case, particularly if you are talking about exception handling, since you'll also need ptrace for the debug registers. I don't think it's worth adding complicated workarounds for this. -- Alexandre Julliard julliard(a)winehq.org
On Wed, 30 Oct 2019, Alexandre Julliard wrote:
Martin Storsjö <martin(a)martin.st> writes:
On Tue, 29 Oct 2019, Andrew Wesie wrote:
On Tue, Oct 29, 2019 at 9:04 AM Martin Storsjo <martin(a)martin.st> wrote:
... Therefore, exception handling in executables with libunwind on i686 doesn't work when run with wine within docker.
I personally don't think this is a great rationale for changing the behavior of wine. It sounds like a rationale for docker to allow SYS_PTRACE.
It can be enabled, but it's not generally enabled by default.
In any case: I believe it is fair to hope that EnumProcessModules(GetCurrentProcess()) would work, without privileges that let you read the memory of other processes.
Not being able to use ptrace isn't a very interesting case, particularly if you are talking about exception handling, since you'll also need ptrace for the debug registers.
Hmm, are the debug registers used somewhere in exception unwinding? SEH based exception handling works fine without ptrace at the moment.
I don't think it's worth adding complicated workarounds for this.
Ok, fair enough. I presume you're not willing to add a special case directly within EnumProcessModules either? That has much less risk of vaguely changing semantics of other functions, but it does add a bit of code duplication though. // Martin
Martin Storsjö <martin(a)martin.st> writes:
On Wed, 30 Oct 2019, Alexandre Julliard wrote:
Not being able to use ptrace isn't a very interesting case, particularly if you are talking about exception handling, since you'll also need ptrace for the debug registers.
Hmm, are the debug registers used somewhere in exception unwinding? SEH based exception handling works fine without ptrace at the moment.
They would be used if the app has changed them.
I don't think it's worth adding complicated workarounds for this.
Ok, fair enough.
I presume you're not willing to add a special case directly within EnumProcessModules either? That has much less risk of vaguely changing semantics of other functions, but it does add a bit of code duplication though.
That could be acceptable, depending on what the resulting code looks like. -- Alexandre Julliard julliard(a)winehq.org
participants (4)
-
Alexandre Julliard -
Andrew Wesie -
Martin Storsjo -
Martin Storsjö