From 84a5bd929213b9e0f059d3bc8c5b738e9fe4e620 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 9 May 2014 16:40:10 -0400 Subject: Stop using mach_host_self and host_page_size, fixing a port right leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is incorrect to use mach_host_self without disposing of the send right to the host port with mach_port_deallocate when done with it. http://crbug.com/105513 shows the sorts of problems that can arise when send rights aren’t properly deallocated. mach_host_self was only used by mach_override to be able to call host_page_size. host_page_size is unnecessary, because it always returns a constant value, PAGE_SIZE, which is also known at user-land compile time. See libsyscall/mach/mach_init.c. User code is better off just using this macro directly, and not fumbling with the system calls to obtain and properly dispose of a send right to the host port. (You need to mach_port_deallocate the ports you get from mach_host_self and mach_thread_self, but you must not normally deallocate the one from mach_task_self, because mach_task_self is actually just a macro that references a global variable. It doesn’t add any port rights at all. See . If you bypass the macro and call the real mach_task_self system call, you do need to call mach_port_deallocate, but this situation is incredibly rare.) --- mach_override.c | 77 ++++++++++++++++++++++++--------------------------------- 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/mach_override.c b/mach_override.c index d0ca355..85a75e5 100644 --- a/mach_override.c +++ b/mach_override.c @@ -9,7 +9,6 @@ #endif #include -#include #include #include #include @@ -160,12 +159,10 @@ fixupInstructions( #if defined(__i386__) || defined(__x86_64__) mach_error_t makeIslandExecutable(void *address) { mach_error_t err = err_none; - vm_size_t pageSize; - host_page_size( mach_host_self(), &pageSize ); - uintptr_t page = (uintptr_t)address & ~(uintptr_t)(pageSize-1); + uintptr_t page = (uintptr_t)address & ~(uintptr_t)(PAGE_SIZE - 1); int e = err_none; - e |= mprotect((void *)page, pageSize, PROT_EXEC | PROT_READ); - e |= msync((void *)page, pageSize, MS_INVALIDATE ); + e |= mprotect((void *)page, PAGE_SIZE, PROT_EXEC | PROT_READ); + e |= msync((void *)page, PAGE_SIZE, MS_INVALIDATE ); if (e) { err = err_cannot_override; } @@ -392,50 +389,46 @@ allocateBranchIsland( mach_error_t err = err_none; if( allocateHigh ) { - vm_size_t pageSize; - err = host_page_size( mach_host_self(), &pageSize ); - if( !err ) { - assert( sizeof( BranchIsland ) <= pageSize ); - vm_address_t page = 0; + assert( sizeof( BranchIsland ) <= PAGE_SIZE ); + vm_address_t page = 0; #if defined(__i386__) - err = vm_allocate( mach_task_self(), &page, pageSize, VM_FLAGS_ANYWHERE ); - if( err == err_none ) - *island = (BranchIsland*) page; + err = vm_allocate( mach_task_self(), &page, PAGE_SIZE, VM_FLAGS_ANYWHERE ); + if( err == err_none ) + *island = (BranchIsland*) page; #else #if defined(__ppc__) || defined(__POWERPC__) - vm_address_t first = 0xfeffffff; - vm_address_t last = 0xfe000000 + pageSize; + vm_address_t first = 0xfeffffff; + vm_address_t last = 0xfe000000 + PAGE_SIZE; #elif defined(__x86_64__) - // 64-bit ASLR is in bits 13-28 - vm_address_t first = ((uint64_t)originalFunctionAddress & ~( (0xFUL << 28) | (pageSize - 1) ) ) | (0x1UL << 31); - vm_address_t last = (uint64_t)originalFunctionAddress & ~((0x1UL << 32) - 1); + // 64-bit ASLR is in bits 13-28 + vm_address_t first = ((uint64_t)originalFunctionAddress & ~( (0xFUL << 28) | (PAGE_SIZE - 1) ) ) | (0x1UL << 31); + vm_address_t last = (uint64_t)originalFunctionAddress & ~((0x1UL << 32) - 1); #endif - page = first; - int allocated = 0; - vm_map_t task_self = mach_task_self(); - - while( !err && !allocated && page != last ) { + page = first; + int allocated = 0; + vm_map_t task_self = mach_task_self(); + + while( !err && !allocated && page != last ) { - err = vm_allocate( task_self, &page, pageSize, 0 ); - if( err == err_none ) - allocated = 1; - else if( err == KERN_NO_SPACE ) { + err = vm_allocate( task_self, &page, PAGE_SIZE, 0 ); + if( err == err_none ) + allocated = 1; + else if( err == KERN_NO_SPACE ) { #if defined(__x86_64__) - page -= pageSize; + page -= PAGE_SIZE; #else - page += pageSize; + page += PAGE_SIZE; #endif - err = err_none; - } + err = err_none; } - if( allocated ) - *island = (BranchIsland*) page; - else if( !allocated && !err ) - err = KERN_NO_SPACE; -#endif } + if( allocated ) + *island = (BranchIsland*) page; + else if( !allocated && !err ) + err = KERN_NO_SPACE; +#endif } else { void *block = malloc( sizeof( BranchIsland ) ); if( block ) @@ -468,14 +461,8 @@ freeBranchIsland( mach_error_t err = err_none; if( island->allocatedHigh ) { - vm_size_t pageSize; - err = host_page_size( mach_host_self(), &pageSize ); - if( !err ) { - assert( sizeof( BranchIsland ) <= pageSize ); - err = vm_deallocate( - mach_task_self(), - (vm_address_t) island, pageSize ); - } + assert( sizeof( BranchIsland ) <= PAGE_SIZE ); + err = vm_deallocate(mach_task_self(), (vm_address_t) island, PAGE_SIZE ); } else { free( island ); } -- cgit v1.2.3