[PATCH] ntdll: Avoid the lower 2GB region for large allocations on win64.
Fixes a crash in PaintTool SAI when allocating more than 2GB of memory. Signed-off-by: Elaine Lefler <elaineclefler(a)gmail.com> ---
From what I can tell, Sai basically requests an initial block of memory from VirtualAlloc, then increments that pointer and calls VirtualAlloc again with a fixed address when it wants more ram. Blindly assuming that memory space is non-fragmented enough for the call to succeed. This is definitely incorrect program behavior, but it works on Windows, and after this tweak it's working on Wine too.
Heads up: I initially disabled the code path for all allocations, not just large ones. On macOS this works fine. On Linux it's broken. Somewhere, presumably in the X11 driver, we're truncating pointers to 32 bits. The change is safe for Windows apps, since Windows itself seems to avoid the lower 4GB region, even for functions like HeapAlloc. I can't find any apparent cause for the truncation, other than converting HWNDs to XIDs, which doesn't seem to be the problem. I wonder if there might be a confusion between "long" and "long long" somewhere. Anyway, window data should never be large enough to trigger the new behavior, so the broken behavior stays under containment until it can be fixed properly. --- dlls/ntdll/unix/virtual.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 94b300c5057..eac9bcb1f83 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -1917,7 +1917,11 @@ static NTSTATUS map_view( struct file_view **view_ret, void *base, size_t size, alloc.top_down = top_down; alloc.limit = (void*)(get_zero_bits_mask( zero_bits ) & (UINT_PTR)user_space_limit); - if (mmap_enum_reserved_areas( alloc_reserved_area_callback, &alloc, top_down )) + if ( +#ifdef _WIN64 + size < 2 * 1024 * 1024 && +#endif + mmap_enum_reserved_areas( alloc_reserved_area_callback, &alloc, top_down )) { ptr = alloc.result; TRACE( "got mem in reserved area %p-%p\n", ptr, (char *)ptr + size ); -- 2.32.0 (Apple Git-132)
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=111018 Your paranoid android. === debian11 (64 bit WoW report) === ntdll: info.c:1843: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136 info.c:1860: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136
On Tue, Mar 22, 2022 at 8:06 PM Marvin <testbot(a)winehq.org> wrote:
ntdll: info.c:1843: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136 info.c:1860: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136
I assume this is safe to ignore? The code expects "pvi.VirtualSize >= prev_size + alloc_size" and the numbers check out. It doesn't look like anything is wrong.
March 22, 2022 9:39 PM, "Elaine Lefler" <elaineclefler(a)gmail.com> wrote:
On Tue, Mar 22, 2022 at 8:06 PM Marvin <testbot(a)winehq.org> wrote:
ntdll: info.c:1843: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136 info.c:1860: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136
I assume this is safe to ignore? The code expects "pvi.VirtualSize >= prev_size + alloc_size" and the numbers check out. It doesn't look like anything is wrong.
You're half right--there is nothing wrong with the test itself. But you do need to make a change there. Not all the tests succeed on Wine. The failing tests are inside statements marked "todo_wine" in the test source--in other words, these tests are "expected to fail" (you may be familiar with the term "XFAIL", as used in GCC, LLVM, and some other projects). Your change makes the tests on those two lines, which are inside a todo_wine block, pass. So you need to remove those tests from the todo_wine block. Chip
Got it, will do. Is it better if I resubmit as [PATCH v2 1/2] or should the test be submitted as a separate patch? On Tue, Mar 22, 2022 at 9:29 PM Chip Davis <cdavis(a)codeweavers.com> wrote:
March 22, 2022 9:39 PM, "Elaine Lefler" <elaineclefler(a)gmail.com> wrote:
On Tue, Mar 22, 2022 at 8:06 PM Marvin <testbot(a)winehq.org> wrote:
ntdll: info.c:1843: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136 info.c:1860: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136
I assume this is safe to ignore? The code expects "pvi.VirtualSize >= prev_size + alloc_size" and the numbers check out. It doesn't look like anything is wrong.
You're half right--there is nothing wrong with the test itself. But you do need to make a change there.
Not all the tests succeed on Wine. The failing tests are inside statements marked "todo_wine" in the test source--in other words, these tests are "expected to fail" (you may be familiar with the term "XFAIL", as used in GCC, LLVM, and some other projects). Your change makes the tests on those two lines, which are inside a todo_wine block, pass. So you need to remove those tests from the todo_wine block.
Chip
March 22, 2022 11:38 PM, "Elaine Lefler" <elaineclefler(a)gmail.com> wrote:
Got it, will do. Is it better if I resubmit as [PATCH v2 1/2] or should the test be submitted as a separate patch?
Generally, todo_wine should be removed in the same patch that fixed the tests.
On Tue, Mar 22, 2022 at 9:29 PM Chip Davis <cdavis(a)codeweavers.com> wrote:
March 22, 2022 9:39 PM, "Elaine Lefler" <elaineclefler(a)gmail.com> wrote:
On Tue, Mar 22, 2022 at 8:06 PM Marvin <testbot(a)winehq.org> wrote:
ntdll: info.c:1843: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136 info.c:1860: Test succeeded inside todo block: Expected to be greater than 2045915136, got 2045915136
I assume this is safe to ignore? The code expects "pvi.VirtualSize >= prev_size + alloc_size" and the numbers check out. It doesn't look like anything is wrong.
You're half right--there is nothing wrong with the test itself. But you do need to make a change there.
Not all the tests succeed on Wine. The failing tests are inside statements marked "todo_wine" in the test source--in other words, these tests are "expected to fail" (you may be familiar with the term "XFAIL", as used in GCC, LLVM, and some other projects). Your change makes the tests on those two lines, which are inside a todo_wine block, pass. So you need to remove those tests from the todo_wine block.
Chip
Chip
participants (3)
-
Chip Davis -
Elaine Lefler -
Marvin