[PATCH 0/3] MR5117: rpcrt4/tests: Allocate stub buffers with NdrOleAllocate.
This fixes crashes on Windows 7-10. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117
From: Alex Henrie <alexhenrie24(a)gmail.com> NdrFree is an internal Wine function. --- dlls/rpcrt4/tests/ndr_marshall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/rpcrt4/tests/ndr_marshall.c b/dlls/rpcrt4/tests/ndr_marshall.c index d86f3b0556e..da6e57b2587 100644 --- a/dlls/rpcrt4/tests/ndr_marshall.c +++ b/dlls/rpcrt4/tests/ndr_marshall.c @@ -273,7 +273,7 @@ static void test_pointer_marshal(const unsigned char *formattypes, ok(StubMsg.Buffer - StubMsg.BufferStart == wiredatalen, "%s: Buffer %p Start %p len %ld\n", msgpfx, StubMsg.Buffer, StubMsg.BufferStart, wiredatalen); ok(StubMsg.MemorySize == 0, "%s: memorysize %ld\n", msgpfx, StubMsg.MemorySize); ok(my_alloc_called == num_additional_allocs, "%s: my_alloc got called %d times\n", msgpfx, my_alloc_called); - /* On Windows 7+ unmarshalling may involve calls to NdrFree, for unclear reasons. */ + /* On Windows 7+ unmarshalling may involve calls to StubMsg.pfnFree, for unclear reasons. */ my_free_called = 0; NdrPointerFree(&StubMsg, mem, formattypes); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5117
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/rpcrt4/tests/ndr_marshall.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/rpcrt4/tests/ndr_marshall.c b/dlls/rpcrt4/tests/ndr_marshall.c index da6e57b2587..f3bb3bc0133 100644 --- a/dlls/rpcrt4/tests/ndr_marshall.c +++ b/dlls/rpcrt4/tests/ndr_marshall.c @@ -2633,7 +2633,9 @@ static void test_ndr_buffer(void) NdrClientInitializeNew(&RpcMessage, &StubMsg, &StubDesc, 5); + my_alloc_called = 0; ret = NdrGetBuffer(&StubMsg, 10, Handle); + ok(!my_alloc_called, "my_alloc got called\n"); ok(ret == StubMsg.Buffer, "NdrGetBuffer should have returned the same value as StubMsg.Buffer instead of %p\n", ret); ok(RpcMessage.Handle != NULL, "RpcMessage.Handle should not have been NULL\n"); ok(RpcMessage.Buffer != NULL, "RpcMessage.Buffer should not have been NULL\n"); @@ -2654,7 +2656,9 @@ static void test_ndr_buffer(void) prev_buffer_length = RpcMessage.BufferLength; StubMsg.BufferLength = 1; + my_free_called = 0; NdrFreeBuffer(&StubMsg); + ok(!my_free_called, "my_free got called\n"); ok(RpcMessage.Handle != NULL, "RpcMessage.Handle should not have been NULL\n"); ok(RpcMessage.Buffer != NULL, "RpcMessage.Buffer should not have been NULL\n"); ok(RpcMessage.BufferLength == prev_buffer_length, "RpcMessage.BufferLength should have been left as %ld instead of %d\n", prev_buffer_length, RpcMessage.BufferLength); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5117
From: Alex Henrie <alexhenrie24(a)gmail.com> This fixes crashes on Windows 7-10. --- dlls/rpcrt4/tests/ndr_marshall.c | 52 ++++++++++++++++---------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/dlls/rpcrt4/tests/ndr_marshall.c b/dlls/rpcrt4/tests/ndr_marshall.c index f3bb3bc0133..72f282b75ff 100644 --- a/dlls/rpcrt4/tests/ndr_marshall.c +++ b/dlls/rpcrt4/tests/ndr_marshall.c @@ -125,14 +125,14 @@ static void determine_pointer_marshalling_style(void) 0); StubMsg.BufferLength = 8; - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); NdrPointerMarshall(&StubMsg, (unsigned char*)&ch, fmtstr_up_char); ok(StubMsg.Buffer == StubMsg.BufferStart + 5, "%p %p\n", StubMsg.Buffer, StubMsg.BufferStart); use_pointer_ids = (*(unsigned int *)StubMsg.BufferStart != (UINT_PTR)&ch); trace("Pointer marshalling using %s\n", use_pointer_ids ? "pointer ids" : "pointer value"); - free(StubMsg.BufferStart); + NdrOleFree(StubMsg.BufferStart); } static void test_ndr_simple_type(void) @@ -152,7 +152,7 @@ static void test_ndr_simple_type(void) 0); StubMsg.BufferLength = 16; - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.Buffer + StubMsg.BufferLength; l = 0xcafebabe; NdrSimpleTypeMarshall(&StubMsg, (unsigned char*)&l, FC_LONG); @@ -169,7 +169,7 @@ static void test_ndr_simple_type(void) ok(StubMsg.Buffer == StubMsg.BufferStart + 8, "%p %p\n", StubMsg.Buffer, StubMsg.BufferStart); ok(l2 == l, "%ld\n", l2); - free(StubMsg.BufferStart); + NdrOleFree(StubMsg.BufferStart); } static void test_pointer_marshal(const unsigned char *formattypes, @@ -206,7 +206,7 @@ static void test_pointer_marshal(const unsigned char *formattypes, ok(StubMsg.BufferLength >= wiredatalen, "%s: length %ld\n", msgpfx, StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; memset(StubMsg.BufferStart, 0x0, StubMsg.BufferLength); /* This is a hack to clear the padding between the ptr and longlong/double */ @@ -491,7 +491,7 @@ static void test_pointer_marshal(const unsigned char *formattypes, else ok(my_free_called == num_additional_allocs, "%s: my_free got called %d times\n", msgpfx, my_free_called); - free(StubMsg.BufferStart); + NdrOleFree(StubMsg.BufferStart); } static int deref_cmp(const void *s1, const void *s2, size_t num) @@ -756,7 +756,7 @@ static void test_nontrivial_pointer_types(void) ok(StubMsg.BufferLength >= 5, "length %ld\n", StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrPointerMarshall( &StubMsg, (unsigned char *)p1, &fmtstr_ref_unique_out[4] ); @@ -864,7 +864,7 @@ static void test_nontrivial_pointer_types(void) my_free(mem); free(mem_orig); - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); } static void test_simple_struct_marshal(const unsigned char *formattypes, @@ -894,7 +894,7 @@ static void test_simple_struct_marshal(const unsigned char *formattypes, StubMsg.BufferLength = 0; NdrSimpleStructBufferSize( &StubMsg, memsrc, formattypes ); ok(StubMsg.BufferLength >= wiredatalen, "%s: length %ld\n", msgpfx, StubMsg.BufferLength); - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrSimpleStructMarshall( &StubMsg, memsrc, formattypes ); ok(ptr == NULL, "%s: ret %p\n", msgpfx, ptr); @@ -1032,7 +1032,7 @@ static void test_simple_struct_marshal(const unsigned char *formattypes, my_free(mem); free(mem_orig); - free(StubMsg.BufferStart); + NdrOleFree(StubMsg.BufferStart); } typedef struct @@ -1251,7 +1251,7 @@ static void test_struct_align(void) StubMsg.BufferLength = 0; NdrComplexStructBufferSize(&StubMsg, (unsigned char *)memsrc, fmtstr); - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrComplexStructMarshall(&StubMsg, (unsigned char *)memsrc, fmtstr); @@ -1266,7 +1266,7 @@ static void test_struct_align(void) ok(!memcmp(mem, memsrc, sizeof(*memsrc)), "struct wasn't unmarshalled correctly\n"); StubMsg.pfnFree(mem); - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); free(memsrc_orig); } @@ -1359,7 +1359,7 @@ static void test_iface_ptr(void) StubMsg.BufferLength = 0; NdrInterfacePointerBufferSize(&StubMsg, (unsigned char *)&client_obj.IPersist_iface, fmtstr_ip); - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; /* server -> client */ @@ -1539,7 +1539,7 @@ static void test_iface_ptr(void) NdrInterfacePointerFree(&StubMsg, (unsigned char *)proxy, fmtstr_ip); ok(client_obj.ref == 1, "got %ld references\n", client_obj.ref); - free(StubMsg.BufferStart); + NdrOleFree(StubMsg.BufferStart); CoUninitialize(); } @@ -1947,7 +1947,7 @@ static void test_conformant_array(void) ok(StubMsg.BufferLength >= 20, "length %ld\n", StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrConformantArrayMarshall( &StubMsg, memsrc, fmtstr_conf_array ); @@ -2021,7 +2021,7 @@ static void test_conformant_array(void) StubMsg.pfnFree(mem); StubMsg.pfnFree(mem_orig); - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); } static void test_conformant_string(void) @@ -2058,7 +2058,7 @@ static void test_conformant_string(void) ok(StubMsg.BufferLength >= sizeof(memsrc) + 12, "length %ld\n", StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrPointerMarshall( &StubMsg, (unsigned char *)memsrc, fmtstr_conf_str ); @@ -2146,7 +2146,7 @@ static void test_conformant_string(void) ok(my_free_called == 1, "free called %d\n", my_free_called); free(mem_orig); - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); } static void test_nonconformant_string(void) @@ -2183,7 +2183,7 @@ static void test_nonconformant_string(void) ok(StubMsg.BufferLength >= strlen((char *)memsrc) + 1 + 8, "length %ld\n", StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrNonConformantStringMarshall( &StubMsg, memsrc, fmtstr_nonconf_str ); @@ -2241,7 +2241,7 @@ static void test_nonconformant_string(void) ok(my_alloc_called == 0, "alloc called %d\n", my_alloc_called); free(mem_orig); - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); /* length = size */ NdrClientInitializeNew( @@ -2256,7 +2256,7 @@ static void test_nonconformant_string(void) ok(StubMsg.BufferLength >= strlen((char *)memsrc2) + 1 + 8, "length %ld\n", StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrNonConformantStringMarshall( &StubMsg, memsrc2, fmtstr_nonconf_str ); @@ -2314,7 +2314,7 @@ static void test_nonconformant_string(void) ok(my_alloc_called == 0, "alloc called %d\n", my_alloc_called); free(mem_orig); - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); } static void test_conf_complex_struct(void) @@ -2382,7 +2382,7 @@ static void test_conf_complex_struct(void) ok(StubMsg.BufferLength >= 92, "length %ld\n", StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; ptr = NdrComplexStructMarshall(&StubMsg, (unsigned char *)memsrc, &fmtstr_complex_struct[12]); @@ -2408,7 +2408,7 @@ static void test_conf_complex_struct(void) ok(mem->array[0] == 0, "mem->array[0] wasn't unmarshalled correctly (%u)\n", mem->array[0]); StubMsg.pfnFree(mem); - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); free(memsrc); } @@ -2525,7 +2525,7 @@ static void test_conf_complex_array(void) ok(StubMsg.BufferLength >= expected_length, "length %ld\n", StubMsg.BufferLength); /*NdrGetBuffer(&_StubMsg, _StubMsg.BufferLength, NULL);*/ - StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = malloc(StubMsg.BufferLength); + StubMsg.RpcMsg->Buffer = StubMsg.BufferStart = StubMsg.Buffer = NdrOleAllocate(StubMsg.BufferLength); StubMsg.BufferEnd = StubMsg.BufferStart + StubMsg.BufferLength; #ifdef _WIN64 @@ -2589,7 +2589,7 @@ static void test_conf_complex_array(void) NdrSimpleStructFree( &StubMsg, (unsigned char*)mem, &fmtstr_complex_array[32]); #endif - free(StubMsg.RpcMsg->Buffer); + NdrOleFree(StubMsg.RpcMsg->Buffer); for(i = 0; i < memsrc.dim1; i++) free(memsrc.array[i]); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5117
Do we know why it helps? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62478
On Mon Feb 26 05:08:14 2024 +0000, Nikolay Sivov wrote:
Do we know why it helps? As far as I can tell, Windows does not resize or move these buffers, and does not call `HeapSize` to check whether they are big enough. So no, I don't know why it cares how they are allocated. Do you have any idea?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62593
On Mon Feb 26 05:08:14 2024 +0000, Alex Henrie wrote:
As far as I can tell, Windows does not resize or move these buffers, and does not call `HeapSize` to check whether they are big enough. So no, I don't know why it cares how they are allocated. Do you have any idea? If I knew I wouldn't be asking. Maybe this allocation way aligns and over-allocates, maybe not.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62714
On Mon Feb 26 20:06:48 2024 +0000, Nikolay Sivov wrote:
If I knew I wouldn't be asking. Maybe this allocation way aligns and over-allocates, maybe not. For what it's worth, changing `malloc` back to `HeapAlloc` is another way to avoid the crashes, and I don't think 32-bit `HeapAlloc` aligns or overallocates any more than `malloc` does. I also tried allocating 10 times the required memory and it did not fix anything.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62781
On Tue Feb 27 03:56:30 2024 +0000, Alex Henrie wrote:
For what it's worth, changing `malloc` back to `HeapAlloc` is another way to avoid the crashes, and I don't think 32-bit `HeapAlloc` aligns or overallocates any more than `malloc` does. I also tried allocating 10 times the required memory and it did not fix anything. NdrOle*() are going through allocation spy thing, but internally are probably same HeapAlloc(). Maybe they reallocate, or something else. How do you know it's not calling HeapSize() ?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62807
On Tue Feb 27 13:32:48 2024 +0000, Nikolay Sivov wrote:
NdrOle*() are going through allocation spy thing, but internally are probably same HeapAlloc(). Maybe they reallocate, or something else. How do you know it's not calling HeapSize() ? The marshalling and unmarshalling functions crash instead of returning an error code if the buffer is too small. Furthermore, the pointers to the buffer do not move and `HeapSize` returns the same value before and after. I can't be perfectly certain that Windows isn't calling `HeapSize` internally, but I haven't found any evidence that it is.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62817
On Tue Feb 27 15:48:46 2024 +0000, Alex Henrie wrote:
The marshalling and unmarshalling functions crash instead of returning an error code if the buffer is too small. Furthermore, the pointers to the buffer do not move and `HeapSize` returns the same value before and after. I can't be perfectly certain that Windows isn't calling `HeapSize` internally, but I haven't found any evidence that it is. Then it's probably safe to say that restoring original calls is enough, but we'll need to make sure that internally we use the same ones. That's a good example of side effects from switching to crt functions, that are not really that interesting to spend time investigating.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62826
On Tue Feb 27 16:11:03 2024 +0000, Nikolay Sivov wrote:
Then it's probably safe to say that restoring original calls is enough, but we'll need to make sure that internally we use the same ones. That's a good example of side effects from switching to crt functions, that are not really that interesting to spend time investigating. Yes, I am sorry for the trouble. If I had noticed these problems earlier, I would not have touched this file at all. My best guess, from reading what little documentation is on MSDN, is that Windows expects the stub buffers to be allocated with NdrOleAllocate.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62835
On Tue Feb 27 17:25:55 2024 +0000, Alex Henrie wrote:
Yes, I am sorry for the trouble. If I had noticed these problems earlier, I would not have touched this file at all. My best guess, from reading what little documentation is on MSDN, is that Windows expects the stub buffers to be allocated with NdrOleAllocate. It's not clear to me that NdrOle* is what has to be used everywhere. Do you see them used in some generated code? Regarding this particular test function, there appears to be a mismatch with what NdrClientInitializeNew() would use for memory management pointers, and what test would use in this explicit allocation.
This might be an opportunity to clarify this in some comment, or maybe look for a way to use some Ndr* helpers to avoid managing this memory explicitly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62839
Do you see them used in some generated code?
No, I didn't see any example of how to allocate the buffer in MIDL_STUB_MESSAGE. It's just my best guess that NdrOleAllocate is meant to be used here.
there appears to be a mismatch with what NdrClientInitializeNew() would use for memory management pointers, and what test would use in this explicit allocation
You mean that it's odd that these buffers aren't allocated with pfnAllocate and pfnFree? That was strange to me too. I added some tests to confirm that NdrGetBuffer and NdrFreeBuffer don't use pfnAllocate and pfnFree either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5117#note_62871
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Nikolay Sivov (＠nsivov)