[PATCH v2 0/2] MR4328: ntdll: Don't allocate vprot entries until commit.

[Evan Tang (＠etang-cw)]

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?

--
  v2: ntdll/tests: Add test for reserving massive amounts of memory.

https://gitlab.winehq.org/wine/wine/-/merge_requests/4328

[PATCH v2 1/2] ntdll: Don't allocate vprot entries until commit.

[Evan Tang]

From: Evan Tang

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;
 }

[PATCH v2 2/2] ntdll/tests: Add test for reserving massive amounts of memory.

[Evan Tang]

From: Evan Tang

---
 dlls/ntdll/tests/virtual.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
index a9dec1b5a8a..808b67b9b27 100644
--- a/dlls/ntdll/tests/virtual.c
+++ b/dlls/ntdll/tests/virtual.c
@@ -2676,6 +2676,31 @@ static void test_query_image_information(void)
     NtClose( file );
 }

+static void test_massive_memory_reservation(void)
+{
+    /* Test to make sure large address space reservations don't use more ram than the system has.
+     * Most failures here will be the test getting killed by the OOM killer. */
+    int i;
+    for (i = 12; i < sizeof(void*) * 8; i++)
+    {
+        void* ptr;
+        BOOL is_ok;
+        SetLastError(0xdeadbeef);
+        ptr = VirtualAlloc(NULL, 1ull << i, MEM_RESERVE, PAGE_NOACCESS);
+        if (i < 40 && sizeof(void*) == 8) /* These should succeed */
+            is_ok = GetLastError() == 0xdeadbeef;
+        else if (i < 47) /* These might run out of address space */
+            is_ok = GetLastError() == 0xdeadbeef || GetLastError() == ERROR_NOT_ENOUGH_MEMORY;
+        else
+            is_ok = GetLastError() == ERROR_INVALID_PARAMETER;
+        ok(is_ok, "Unexpected error allocating 1 << %d: %lx.\n", i, GetLastError());
+        if (ptr)
+            VirtualFree(ptr, 1ull << i, MEM_RELEASE);
+        ptr = malloc(1ull << i);
+        free(ptr);
+    }
+}
+
 START_TEST(virtual)
 {
     HMODULE mod;
@@ -2726,4 +2751,5 @@ START_TEST(virtual)
     test_syscalls();
     test_query_region_information();
     test_query_image_information();
+    test_massive_memory_reservation();
 }

[PATCH v2 2/2] ntdll/tests: Add test for reserving massive amounts of memory.

[Marvin]

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 full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=139740

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w7u_adm (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w7u_el (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w8 (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w8adm (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w1064v1809 (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w1064_tsign (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w10pro64 (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w10pro64_en_AE_u8 (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w11pro64 (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== w7pro64 (64 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 39: 5af.
virtual.c:2696: Test failed: Unexpected error allocating 1 << 40: 5af.
virtual.c:2696: Test failed: Unexpected error allocating 1 << 41: 5af.
virtual.c:2696: Test failed: Unexpected error allocating 1 << 42: 5af.
virtual.c:2696: Test failed: Unexpected error allocating 1 << 43: 57.
virtual.c:2696: Test failed: Unexpected error allocating 1 << 44: 57.
virtual.c:2696: Test failed: Unexpected error allocating 1 << 45: 57.
virtual.c:2696: Test failed: Unexpected error allocating 1 << 46: 57.

=== debian11 (32 bit report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11 (32 bit ar:MA report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11 (32 bit de report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11 (32 bit fr report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11 (32 bit he:IL report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11 (32 bit hi:IN report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11 (32 bit ja:JP report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11 (32 bit zh:CN report) ===

ntdll:
virtual.c:2696: Test failed: Unexpected error allocating 1 << 31: 57.

=== debian11b (build log) ===

WineRunWineTest.pl:error: An error occurred while waiting for the task to complete: network read got a premature EOF (wait2/connect:AgentVersion.h:0/9)
WineRunWineTest.pl:error: The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died)