[PATCH 0/2] MR4328: ntdll: Don't allocate vprot entries until commit.
Prevents applications that accidentally attempt to allocate massive amounts of memory from getting OOM killed (An example of such an application is EverQuest's DX11 beta, which has some corrupted asset files that result in it trying to allocate ~18TB of memory) BTW, is it okay to have a test that's likely to get OOM killed on a failing implementation? If not, any recommendations on how else to test this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4328
From: Evan Tang <etang(a)codeweavers.com> Reservations only consume address space, which can be much larger than the amount of ram in the system, so allocating even one byte per page can result in huge amounts of ram used. --- dlls/ntdll/unix/virtual.c | 87 ++++++++++++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 464f343a575..f1afccd0f22 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -898,8 +898,7 @@ static BYTE get_page_vprot( const void *addr ) * Return the size of the region with equal masked vprot byte. * Also return the protections for the first page. * The function assumes that base and size are page aligned, - * base + size does not wrap around and the range is within view so - * vprot bytes are allocated for the range. */ + * base + size does not wrap around, and the range is within view. */ static SIZE_T get_vprot_range_size( char *base, SIZE_T size, BYTE mask, BYTE *vprot ) { static const UINT_PTR word_from_byte = (UINT_PTR)0x101010101010101; @@ -917,11 +916,24 @@ static SIZE_T get_vprot_range_size( char *base, SIZE_T size, BYTE mask, BYTE *vp if (aligned_start_idx > end_idx) aligned_start_idx = end_idx; #ifdef _WIN64 - vprot_ptr = pages_vprot[curr_idx >> pages_vprot_shift] + (curr_idx & pages_vprot_mask); + if (pages_vprot[curr_idx >> pages_vprot_shift]) + { + vprot_ptr = pages_vprot[curr_idx >> pages_vprot_shift] + (curr_idx & pages_vprot_mask); + *vprot = *vprot_ptr; + } + else + { + *vprot = 0; + curr_idx = (curr_idx + pages_vprot_mask + 1) & ~pages_vprot_mask; + if (curr_idx > end_idx) + return size; + vprot_ptr = pages_vprot[curr_idx >> pages_vprot_shift]; + assert(curr_idx >= aligned_start_idx); + } #else vprot_ptr = pages_vprot + curr_idx; -#endif *vprot = *vprot_ptr; +#endif /* Page count page table is at least the multiples of sizeof(UINT_PTR) * so we don't have to worry about crossing the boundary on unaligned idx values. */ @@ -934,7 +946,17 @@ static SIZE_T get_vprot_range_size( char *base, SIZE_T size, BYTE mask, BYTE *vp for (; curr_idx < end_idx; curr_idx += sizeof(UINT_PTR), vprot_ptr += sizeof(UINT_PTR)) { #ifdef _WIN64 - if (!(curr_idx & pages_vprot_mask)) vprot_ptr = pages_vprot[curr_idx >> pages_vprot_shift]; + if (!(curr_idx & pages_vprot_mask)) + { + while (!(vprot_ptr = pages_vprot[curr_idx >> pages_vprot_shift])) + { + if (!(vprot_word & mask_word)) + return (curr_idx - start_idx) << page_shift; + curr_idx += pages_vprot_mask + 1; + if (curr_idx >= end_idx) + return size; + } + } #endif if ((vprot_word ^ *(UINT_PTR *)vprot_ptr) & mask_word) { @@ -946,6 +968,21 @@ static SIZE_T get_vprot_range_size( char *base, SIZE_T size, BYTE mask, BYTE *vp return size; } +#ifdef _WIN64 +/*********************************************************************** + * set_page_vprot_internal + * + * Helper function for set_page_vprot + */ +static void set_pages_vprot_internal( size_t idx, BYTE vprot, size_t len ) +{ + if (pages_vprot[idx >> pages_vprot_shift]) + memset(pages_vprot[idx >> pages_vprot_shift] + (idx & pages_vprot_mask), vprot, len); + else if (vprot) + ERR("vprot table missing entry at %x!\n", (unsigned int)(idx >> pages_vprot_shift)); +} +#endif + /*********************************************************************** * set_page_vprot * @@ -960,10 +997,10 @@ static void set_page_vprot( const void *addr, size_t size, BYTE vprot ) while (idx >> pages_vprot_shift != end >> pages_vprot_shift) { size_t dir_size = pages_vprot_mask + 1 - (idx & pages_vprot_mask); - memset( pages_vprot[idx >> pages_vprot_shift] + (idx & pages_vprot_mask), vprot, dir_size ); + set_pages_vprot_internal(idx, vprot, dir_size); idx += dir_size; } - memset( pages_vprot[idx >> pages_vprot_shift] + (idx & pages_vprot_mask), vprot, end - idx ); + set_pages_vprot_internal(idx, vprot, end - idx); #else memset( pages_vprot + idx, vprot, end - idx ); #endif @@ -981,10 +1018,22 @@ static void set_page_vprot_bits( const void *addr, size_t size, BYTE set, BYTE c size_t end = ((size_t)addr + size + page_mask) >> page_shift; #ifdef _WIN64 - for ( ; idx < end; idx++) + while (idx < end) { - BYTE *ptr = pages_vprot[idx >> pages_vprot_shift] + (idx & pages_vprot_mask); - *ptr = (*ptr & ~clear) | set; + size_t next = (idx + pages_vprot_mask + 1) & ~pages_vprot_mask; + BYTE *ptr = pages_vprot[idx >> pages_vprot_shift]; + if (ptr) + { + size_t i = idx & pages_vprot_mask; + size_t table_end = min(next, end) - (idx & ~pages_vprot_mask); + for (; i < table_end; i++) + ptr[i] = (ptr[i] & ~clear) | set; + } + else if (set) + { + ERR("vprot table missing entry at %x!\n", (unsigned)(idx >> pages_vprot_shift)); + } + idx = next; } #else for ( ; idx < end; idx++) pages_vprot[idx] = (pages_vprot[idx] & ~clear) | set; @@ -1569,6 +1618,10 @@ static void register_view( struct file_view *view ) static NTSTATUS create_view( struct file_view **view_ret, void *base, size_t size, unsigned int vprot ) { struct file_view *view; + /* If uncommitted, delay allocation until commit so we don't make vprot tables for reservations, + * which can be as large as the address space allows (128TB). + * WRITEWATCH doesn't yet support allocating on commit, so make an exception for that. */ + BOOL write_vprot = !!(vprot & (VPROT_COMMITTED | VPROT_WRITEWATCH)); int unix_prot = get_unix_prot( vprot ); assert( !((UINT_PTR)base & page_mask) ); @@ -1586,7 +1639,7 @@ static NTSTATUS create_view( struct file_view **view_ret, void *base, size_t siz delete_view( view ); } - if (!alloc_pages_vprot( base, size )) return STATUS_NO_MEMORY; + if (write_vprot && !alloc_pages_vprot( base, size )) return STATUS_NO_MEMORY; /* Create the view structure */ @@ -1599,7 +1652,7 @@ static NTSTATUS create_view( struct file_view **view_ret, void *base, size_t siz view->base = base; view->size = size; view->protect = vprot; - set_page_vprot( base, size, vprot ); + set_page_vprot( base, size, write_vprot ? vprot : 0 ); register_view( view ); @@ -1737,6 +1790,8 @@ static BOOL set_vprot( struct file_view *view, void *base, size_t size, BYTE vpr return TRUE; } if (mprotect_exec( base, size, unix_prot )) return FALSE; + /* Reserved pages may not have table entries yet, make those now */ + if (vprot && !alloc_pages_vprot( base, size )) return FALSE; set_page_vprot( base, size, vprot ); return TRUE; } @@ -2147,6 +2202,7 @@ done: static SIZE_T get_committed_size( struct file_view *view, void *base, BYTE *vprot, BYTE vprot_mask ) { SIZE_T offset, size; + BYTE extra = 0; base = ROUND_ADDR( base, page_mask ); offset = (char *)base - (char *)view->base; @@ -2167,7 +2223,8 @@ static SIZE_T get_committed_size( struct file_view *view, void *base, BYTE *vpro if (reply->committed) { *vprot |= VPROT_COMMITTED; - set_page_vprot_bits( base, size, VPROT_COMMITTED, 0 ); + extra = VPROT_COMMITTED; + vprot_mask &= ~VPROT_COMMITTED; } } } @@ -2177,7 +2234,9 @@ static SIZE_T get_committed_size( struct file_view *view, void *base, BYTE *vpro } else size = view->size - offset; - return get_vprot_range_size( base, size, vprot_mask, vprot ); + size = get_vprot_range_size( base, size, vprot_mask, vprot ); + *vprot |= extra; + return size; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4328
From: Evan Tang <etang(a)codeweavers.com> --- dlls/ntdll/tests/virtual.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c index a9dec1b5a8a..775d5125b87 100644 --- a/dlls/ntdll/tests/virtual.c +++ b/dlls/ntdll/tests/virtual.c @@ -2676,6 +2676,22 @@ static void test_query_image_information(void) NtClose( file ); } +static void test_massive_memory_reservation(void) +{ + /* No ok() usage here, failing the test == getting killed by the OOM killer */ + SIZE_T i; + for (i = page_size; i; i <<= 1) + { + void* ptr; + ptr = VirtualAlloc(NULL, i, MEM_RESERVE, PAGE_NOACCESS); + if (ptr) + VirtualFree(ptr, i, MEM_RELEASE); + ptr = malloc(i); + if (ptr) + free(ptr); + } +} + START_TEST(virtual) { HMODULE mod; @@ -2726,4 +2742,5 @@ START_TEST(virtual) test_syscalls(); test_query_region_information(); test_query_image_information(); + test_massive_memory_reservation(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4328
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=139647 Your paranoid android. === debian11b (64 bit WoW report) === Report validation errors: kernel32:virtual has no test summary line (early exit of the main process?) kernel32:virtual has unaccounted for todo messages
Alex Henrie (@alexhenrie) commented about dlls/ntdll/tests/virtual.c:
NtClose( file ); }
+static void test_massive_memory_reservation(void) +{ + /* No ok() usage here, failing the test == getting killed by the OOM killer */ + SIZE_T i; + for (i = page_size; i; i <<= 1) + { + void* ptr; + ptr = VirtualAlloc(NULL, i, MEM_RESERVE, PAGE_NOACCESS); + if (ptr)
Does the call to VirtualAlloc always succeed on Windows Vista and later? If so, I think it would be better to have `ok(ptr, ...)` instead of `if (ptr)`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4328#note_51351
Alex Henrie (@alexhenrie) commented about dlls/ntdll/tests/virtual.c:
NtClose( file ); }
+static void test_massive_memory_reservation(void) +{ + /* No ok() usage here, failing the test == getting killed by the OOM killer */ + SIZE_T i; + for (i = page_size; i; i <<= 1) + { + void* ptr; + ptr = VirtualAlloc(NULL, i, MEM_RESERVE, PAGE_NOACCESS); + if (ptr) + VirtualFree(ptr, i, MEM_RELEASE); + ptr = malloc(i); + if (ptr)
It's unnecessary to check for null before calling `free` because `free(NULL)` is guaranteed to do nothing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4328#note_51353
On Tue Nov 7 18:09:52 2023 +0000, Alex Henrie wrote:
Does the call to VirtualAlloc always succeed on Windows Vista and later? If so, I think it would be better to have `ok(ptr, ...)` instead of `if (ptr)`. It does not.
Above 47 bits, it fails with `ERROR_INVALID_PARAMETER`. Around 47 bits, it fails with `ERROR_NOT_ENOUGH_MEMORY`, I assume based on whether it was able to find that much contiguous address space. (The important thing being that none of them kill the program as long as it handles any allocation errors) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4328#note_51546
On Wed Nov 8 18:10:17 2023 +0000, Evan Tang wrote:
It does not. Above 47 bits, it fails with `ERROR_INVALID_PARAMETER`. Around 47 bits, it fails with `ERROR_NOT_ENOUGH_MEMORY`, I assume based on whether it was able to find that much contiguous address space. (The important thing being that none of them kill the program as long as it handles any allocation errors) OK, thanks for the info. I think we can safely say that Windows doesn't (and probably can't due to microarchitectural limitations) support more 47 bits. How about, in addition to the comment about evading the OOM killer, we add `if (i < 48) ok(GetLastError() == 0xdeadbeef || GetLastError() == ERROR_NOT_ENOUGH_MEMORY, ...) else ok(GetLastError() == ERROR_INVALID_PARAMETER, ...)`?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4328#note_51556
participants (4)
-
Alex Henrie (＠alexhenrie) -
Evan Tang
-
Evan Tang (＠etang-cw)
-
Marvin