[PATCH v9 0/6] MR4826: Mach vm operations improvements
For all mach vm operations this removes the task suspend and resume, which are not needed. This uses `mach_vm_read_overwrite` to read into a caller-specified buffer, saving the `mach_vm_deallocate` call (bringing all read operations down to 1 syscall from 4). The only alignment restriction on `mach_vm_write` according to the original CMU documentation is that data is
[pointer to page aligned in array of bytes] An array of data to be written.
(In practice it also works with arbitrary addresses on macOS, but it probably doesn't hurt to follow the original specifications here). The only other reference that these read/writes should be page-aligned is from the GNU Hurd documentation
The current implementation requires that address, data and data_count all be page-aligned. Otherwise, KERN_INVALID_ARGUMENT is returned.
which I assume was the reason why this was originally done (plus it sounds to me like they will fix that in the future and 4fe19777 already broke GNU Hurd support anyways, if that was supposed to be working). Also this includes the missing mach part of 5b1f3b14, which was only applied to the ptrace backend, and together with the `write_process_memory` rework, this gets rid of all fixmes in mach.c -- v9: server: Do not page align address in write_process_memory. server: Do not suspend mach task in write_process_memory. server: Use mach_vm_read_overwrite in get_selector_entry. server: Do not suspend mach task in get_selector_entry. server: Use mach_vm_read_overwrite in read_process_memory. https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/server/mach.c b/server/mach.c index e4bb0a46d40..44a9a589952 100644 --- a/server/mach.c +++ b/server/mach.c @@ -411,12 +411,6 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t return 0; } - if ((ret = task_suspend( process_port )) != KERN_SUCCESS) - { - mach_set_error( ret ); - return 0; - } - offset = ptr % page_size; aligned_address = (mach_vm_address_t)(ptr - offset); aligned_size = (size + offset + page_size - 1) / page_size * page_size; @@ -428,7 +422,6 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t memcpy( dest, (char *)data + offset, size ); mach_vm_deallocate( mach_task_self(), data, bytes_read ); } - task_resume( process_port ); return (ret == KERN_SUCCESS); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/server/mach.c b/server/mach.c index 44a9a589952..796f82ac64a 100644 --- a/server/mach.c +++ b/server/mach.c @@ -392,12 +392,7 @@ int send_thread_signal( struct thread *thread, int sig ) int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, char *dest ) { kern_return_t ret; - mach_msg_type_number_t bytes_read; - mach_vm_offset_t offset; - vm_offset_t data; - mach_vm_address_t aligned_address; - mach_vm_size_t aligned_size; - unsigned int page_size = get_page_size(); + mach_vm_size_t bytes_read; mach_port_t process_port = get_process_port( process ); if (!process_port) @@ -411,17 +406,8 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t return 0; } - offset = ptr % page_size; - aligned_address = (mach_vm_address_t)(ptr - offset); - aligned_size = (size + offset + page_size - 1) / page_size * page_size; - - ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); - if (ret != KERN_SUCCESS) mach_set_error( ret ); - else - { - memcpy( dest, (char *)data + offset, size ); - mach_vm_deallocate( mach_task_self(), data, bytes_read ); - } + ret = mach_vm_read_overwrite( process_port, (mach_vm_address_t)ptr, (mach_vm_size_t)size, (mach_vm_address_t)dest, &bytes_read ); + mach_set_error( ret ); return (ret == KERN_SUCCESS); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/server/mach.c b/server/mach.c index 796f82ac64a..cbd36d6ecd8 100644 --- a/server/mach.c +++ b/server/mach.c @@ -522,25 +522,20 @@ void get_selector_entry( struct thread *thread, int entry, unsigned int *base, return; } - if ((ret = task_suspend( process_port )) == KERN_SUCCESS) - { - mach_vm_offset_t offset = process->ldt_copy % page_size; - mach_vm_address_t aligned_address = (mach_vm_address_t)(process->ldt_copy - offset); - mach_vm_size_t aligned_size = (total_size + offset + page_size - 1) / page_size * page_size; + mach_vm_offset_t offset = process->ldt_copy % page_size; + mach_vm_address_t aligned_address = (mach_vm_address_t)(process->ldt_copy - offset); + mach_vm_size_t aligned_size = (total_size + offset + page_size - 1) / page_size * page_size; - ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); - if (ret != KERN_SUCCESS) mach_set_error( ret ); - else - { - const int *ldt = (const int *)((char *)data + offset); - memcpy( base, ldt + entry, sizeof(int) ); - memcpy( limit, ldt + entry + 8192, sizeof(int) ); - memcpy( flags, (char *)(ldt + 2 * 8192) + entry, 1 ); - mach_vm_deallocate( mach_task_self(), data, bytes_read ); - } - task_resume( process_port ); + ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); + if (ret != KERN_SUCCESS) mach_set_error( ret ); + else + { + const int *ldt = (const int *)((char *)data + offset); + memcpy( base, ldt + entry, sizeof(int) ); + memcpy( limit, ldt + entry + 8192, sizeof(int) ); + memcpy( flags, (char *)(ldt + 2 * 8192) + entry, 1 ); + mach_vm_deallocate( mach_task_self(), data, bytes_read ); } - else mach_set_error( ret ); } #endif /* USE_MACH */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/server/mach.c b/server/mach.c index cbd36d6ecd8..16abc3f56fb 100644 --- a/server/mach.c +++ b/server/mach.c @@ -505,10 +505,9 @@ void get_selector_entry( struct thread *thread, int entry, unsigned int *base, { const unsigned int total_size = (2 * sizeof(int) + 1) * 8192; struct process *process = thread->process; - unsigned int page_size = get_page_size(); - vm_offset_t data; + mach_vm_address_t data; kern_return_t ret; - mach_msg_type_number_t bytes_read; + mach_vm_size_t bytes_read; mach_port_t process_port = get_process_port( thread->process ); if (!process->ldt_copy || !process_port) @@ -518,24 +517,27 @@ void get_selector_entry( struct thread *thread, int entry, unsigned int *base, } if (entry >= 8192) { - set_error( STATUS_INVALID_PARAMETER ); /* FIXME */ + set_error( STATUS_ACCESS_VIOLATION ); return; } - mach_vm_offset_t offset = process->ldt_copy % page_size; - mach_vm_address_t aligned_address = (mach_vm_address_t)(process->ldt_copy - offset); - mach_vm_size_t aligned_size = (total_size + offset + page_size - 1) / page_size * page_size; + if (!(data = (mach_vm_address_t)malloc( total_size ))) + { + set_error( STATUS_NO_MEMORY ); + return; + } - ret = mach_vm_read( process_port, aligned_address, aligned_size, &data, &bytes_read ); + ret = mach_vm_read_overwrite( process_port, (mach_vm_address_t)process->ldt_copy, (mach_vm_size_t)total_size, data, &bytes_read ); if (ret != KERN_SUCCESS) mach_set_error( ret ); else { - const int *ldt = (const int *)((char *)data + offset); + const int *ldt = (const int *)data; memcpy( base, ldt + entry, sizeof(int) ); memcpy( limit, ldt + entry + 8192, sizeof(int) ); memcpy( flags, (char *)(ldt + 2 * 8192) + entry, 1 ); - mach_vm_deallocate( mach_task_self(), data, bytes_read ); } + + free( (void *)data ); } #endif /* USE_MACH */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/server/mach.c b/server/mach.c index 16abc3f56fb..1dca47cd5af 100644 --- a/server/mach.c +++ b/server/mach.c @@ -440,12 +440,6 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t aligned_address = (mach_vm_address_t)(ptr - offset); aligned_size = (size + offset + page_size - 1) / page_size * page_size; - if ((ret = task_suspend( process_port )) != KERN_SUCCESS) - { - mach_set_error( ret ); - return 0; - } - ret = mach_vm_read( process_port, aligned_address, aligned_size, &task_mem, &bytes_read ); if (ret != KERN_SUCCESS) { @@ -489,13 +483,11 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); /* restore protection */ mach_vm_protect( process_port, aligned_address, aligned_size, 0, info.protection ); - task_resume( process_port ); return 1; } failed: if (task_mem) mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); - task_resume( process_port ); return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- server/mach.c | 66 +++++++-------------------------------------------- 1 file changed, 9 insertions(+), 57 deletions(-) diff --git a/server/mach.c b/server/mach.c index 1dca47cd5af..bd7b7e33037 100644 --- a/server/mach.c +++ b/server/mach.c @@ -415,15 +415,9 @@ int read_process_memory( struct process *process, client_ptr_t ptr, data_size_t int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t size, const char *src ) { kern_return_t ret; - mach_vm_address_t aligned_address, region_address; - mach_vm_size_t aligned_size, region_size; - mach_msg_type_number_t info_size, bytes_read; - mach_vm_offset_t offset; - vm_offset_t task_mem = 0; - struct vm_region_basic_info_64 info; - mach_port_t dummy; unsigned int page_size = get_page_size(); mach_port_t process_port = get_process_port( process ); + mach_vm_offset_t data; if (!process_port) { @@ -435,60 +429,18 @@ int write_process_memory( struct process *process, client_ptr_t ptr, data_size_t set_error( STATUS_ACCESS_DENIED ); return 0; } - - offset = ptr % page_size; - aligned_address = (mach_vm_address_t)(ptr - offset); - aligned_size = (size + offset + page_size - 1) / page_size * page_size; - - ret = mach_vm_read( process_port, aligned_address, aligned_size, &task_mem, &bytes_read ); - if (ret != KERN_SUCCESS) - { - mach_set_error( ret ); - goto failed; - } - region_address = aligned_address; - info_size = sizeof(info); - ret = mach_vm_region( process_port, ®ion_address, ®ion_size, VM_REGION_BASIC_INFO_64, - (vm_region_info_t)&info, &info_size, &dummy ); - if (ret != KERN_SUCCESS) + if (posix_memalign( (void **)&data, page_size, size )) { - mach_set_error( ret ); - goto failed; - } - if (region_address > aligned_address || - region_address + region_size < aligned_address + aligned_size) - { - /* FIXME: should support multiple regions */ - set_error( ERROR_ACCESS_DENIED ); - goto failed; - } - ret = mach_vm_protect( process_port, aligned_address, aligned_size, 0, VM_PROT_READ | VM_PROT_WRITE ); - if (ret != KERN_SUCCESS) - { - mach_set_error( ret ); - goto failed; + set_error( STATUS_NO_MEMORY ); + return 0; } - /* FIXME: there's an optimization that can be made: check first and last */ - /* pages for writability; read first and last pages; write interior */ - /* pages to task without ever reading&modifying them; if that succeeds, */ - /* modify first and last pages and write them. */ - - memcpy( (char*)task_mem + offset, src, size ); + memcpy( (void *)data, src, size ); - ret = mach_vm_write( process_port, aligned_address, task_mem, bytes_read ); - if (ret != KERN_SUCCESS) mach_set_error( ret ); - else - { - mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); - /* restore protection */ - mach_vm_protect( process_port, aligned_address, aligned_size, 0, info.protection ); - return 1; - } - -failed: - if (task_mem) mach_vm_deallocate( mach_task_self(), task_mem, bytes_read ); - return 0; + ret = mach_vm_write( process_port, (mach_vm_address_t)ptr, data, (mach_msg_type_number_t)size ); + free( (void *)data ); + mach_set_error( ret ); + return (ret == KERN_SUCCESS); } /* retrieve an LDT selector entry */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4826
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=150140 Your paranoid android. === debian11b (64 bit WoW report) === Report validation errors: mshtml:script crashed (c0000005)
participants (3)
-
Marc-Aurel Zent -
Marc-Aurel Zent (＠mzent)
-
Marvin