[PATCH v5 1/5] combase/tests: Add tests for the hstring_private struct.
The goal of these tests is to show, that the memory layout of hstring_private is different on Windows, than currently used in Wine. This creates issues with the WinRT SDK, which allocates HSTRINGs without using the functions provided by the combase dll. Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/combase/tests/string.c | 80 +++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/dlls/combase/tests/string.c b/dlls/combase/tests/string.c index 5ebf669a426..713f08d342b 100644 --- a/dlls/combase/tests/string.c +++ b/dlls/combase/tests/string.c @@ -479,6 +479,85 @@ static void test_trim(void) ok(WindowsDeleteString(str1) == S_OK, "Failed to delete string\n"); } +static void test_hstring_struct(void) +{ + struct hstring_header + { + UINT32 flags; + UINT32 length; + UINT32 padding1; + UINT32 padding2; + const WCHAR *str; + }; + + struct hstring_private + { + struct hstring_header header; + LONG refcount; + WCHAR buffer[1]; + }; + + HSTRING str; + HSTRING str2; + HSTRING_HEADER hdr; + struct hstring_private* prv; + struct hstring_private* prv2; + + BOOL arch64 = (sizeof(void*) == 8); + + ok(arch64 ? (sizeof(prv->header) == 24) : (sizeof(prv->header) == 20), "hstring_header size incorrect.\n"); + + ok(WindowsCreateString(input_string, 6, &str) == S_OK, "Failed to create string.\n"); + + prv = CONTAINING_RECORD(str, struct hstring_private, header); + + todo_wine + ok(prv->header.flags == 0, "Expected 0 in flags field, got %#x.\n", prv->header.flags); + todo_wine + ok(prv->header.length == 6, "Expected 6 in length field, got %u.\n", prv->header.length); + todo_wine + ok(prv->header.str == prv->buffer, "Expected str to point at buffer, instead pointing at %p.\n", prv->header.str); + todo_wine + ok(prv->refcount == 1, "Expected 1 in refcount, got %u.\n", prv->refcount); + todo_wine + ok(wcscmp(input_string, prv->buffer) == 0, "Expected strings to match.\n"); + todo_wine + ok(prv->buffer[prv->header.length] == '\0', "Expected buffer to be null terminated.\n"); + + ok(WindowsDuplicateString(str, &str2) == S_OK, "Failed to duplicate string.\n"); + + prv2 = CONTAINING_RECORD(str2, struct hstring_private, header); + + todo_wine + ok(prv->refcount == 2, "Expected 2 in refcount, got %u.\n", prv->refcount); + todo_wine + ok(prv2->refcount == 2, "Expected 2 in refcount, got %u.\n", prv2->refcount); + todo_wine + ok(wcscmp(input_string, prv2->buffer) == 0, "Expected strings to match.\n"); + + ok(WindowsDeleteString(str) == S_OK, "Failed to delete string.\n"); + + todo_wine + ok(prv->refcount == 1, "Expected 1 in refcount, got %u.\n", prv->refcount); + + ok(WindowsDeleteString(str) == S_OK, "Failed to delete string.\n"); + + ok(WindowsCreateStringReference(input_string, 6, &hdr, &str) == S_OK, "Failed to create string ref.\n"); + + prv = CONTAINING_RECORD(&hdr, struct hstring_private, header); + prv2 = CONTAINING_RECORD(str, struct hstring_private, header); + + ok(prv == prv2, "Pointers not identical.\n"); + todo_wine + ok(prv2->header.flags == 1, "Expected HSTRING_REFERENCE_FLAG to be set, got %#x.\n", prv2->header.flags); + todo_wine + ok(prv2->header.length == 6, "Expected 6 in length field, got %u.\n", prv2->header.length); + todo_wine + ok(prv2->header.str == input_string, "Expected str to point at input_string, instead pointing at %p.\n", prv2->header.str); + + ok(WindowsDeleteString(str) == S_OK, "Failed to delete string ref.\n"); +} + START_TEST(string) { test_create_delete(); @@ -489,4 +568,5 @@ START_TEST(string) test_concat(); test_compare(); test_trim(); + test_hstring_struct(); } -- 2.34.1
Also move the reference flag to the hstring_header struct. Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/combase/string.c | 44 +++++++++++++++++++++++++------------ dlls/combase/tests/string.c | 2 -- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/dlls/combase/string.c b/dlls/combase/string.c index 2092e4360a3..9443583d90e 100644 --- a/dlls/combase/string.c +++ b/dlls/combase/string.c @@ -26,31 +26,38 @@ WINE_DEFAULT_DEBUG_CHANNEL(winstring); +#define HSTRING_REFERENCE_FLAG 1 + +struct hstring_header +{ + UINT32 flags; +}; + struct hstring_private { + struct hstring_header header; LPWSTR buffer; UINT32 length; - BOOL reference; LONG refcount; }; static const WCHAR empty[1]; -C_ASSERT(sizeof(struct hstring_private) <= sizeof(HSTRING_HEADER)); +C_ASSERT(sizeof(struct hstring_header) <= sizeof(HSTRING_HEADER)); static inline struct hstring_private *impl_from_HSTRING(HSTRING string) { - return (struct hstring_private *)string; + return (struct hstring_private *)string; } static inline struct hstring_private *impl_from_HSTRING_HEADER(HSTRING_HEADER *header) { - return (struct hstring_private *)header; + return CONTAINING_RECORD(header, struct hstring_private, header); } static inline struct hstring_private *impl_from_HSTRING_BUFFER(HSTRING_BUFFER buffer) { - return (struct hstring_private *)buffer; + return CONTAINING_RECORD(buffer, struct hstring_private, buffer); } static BOOL alloc_string(UINT32 len, HSTRING *out) @@ -59,11 +66,13 @@ static BOOL alloc_string(UINT32 len, HSTRING *out) priv = HeapAlloc(GetProcessHeap(), 0, sizeof(*priv) + (len + 1) * sizeof(*priv->buffer)); if (!priv) return FALSE; + + priv->header.flags = 0; priv->buffer = (LPWSTR)(priv + 1); priv->length = len; - priv->reference = FALSE; priv->refcount = 1; priv->buffer[len] = '\0'; + *out = (HSTRING)priv; return TRUE; } @@ -115,10 +124,12 @@ HRESULT WINAPI WindowsCreateStringReference(LPCWSTR ptr, UINT32 len, } if (ptr == NULL) return E_POINTER; + priv->buffer = (LPWSTR)ptr; priv->length = len; - priv->reference = TRUE; - *out = (HSTRING)header; + priv->header.flags = HSTRING_REFERENCE_FLAG; + + *out = (HSTRING)priv; return S_OK; } @@ -133,7 +144,7 @@ HRESULT WINAPI WindowsDeleteString(HSTRING str) if (str == NULL) return S_OK; - if (priv->reference) + if (priv->header.flags == HSTRING_REFERENCE_FLAG) return S_OK; if (InterlockedDecrement(&priv->refcount) == 0) HeapFree(GetProcessHeap(), 0, priv); @@ -156,7 +167,7 @@ HRESULT WINAPI WindowsDuplicateString(HSTRING str, HSTRING *out) *out = NULL; return S_OK; } - if (priv->reference) + if (priv->header.flags == HSTRING_REFERENCE_FLAG) return WindowsCreateString(priv->buffer, priv->length, out); InterlockedIncrement(&priv->refcount); *out = str; @@ -187,7 +198,7 @@ HRESULT WINAPI WindowsPreallocateStringBuffer(UINT32 len, WCHAR **outptr, return E_OUTOFMEMORY; priv = impl_from_HSTRING(str); *outptr = priv->buffer; - *out = (HSTRING_BUFFER)str; + *out = (HSTRING_BUFFER)&priv->buffer; return S_OK; } @@ -196,9 +207,14 @@ HRESULT WINAPI WindowsPreallocateStringBuffer(UINT32 len, WCHAR **outptr, */ HRESULT WINAPI WindowsDeleteStringBuffer(HSTRING_BUFFER buf) { + struct hstring_private *priv = NULL; + TRACE("(%p)\n", buf); - return WindowsDeleteString((HSTRING)buf); + if(buf) + priv = impl_from_HSTRING_BUFFER(buf); + + return WindowsDeleteString((HSTRING)priv); } /*********************************************************************** @@ -217,9 +233,9 @@ HRESULT WINAPI WindowsPromoteStringBuffer(HSTRING_BUFFER buf, HSTRING *out) *out = NULL; return S_OK; } - if (priv->buffer[priv->length] != 0 || priv->reference || priv->refcount != 1) + if (priv->buffer[priv->length] != 0 || priv->header.flags == HSTRING_REFERENCE_FLAG || priv->refcount != 1) return E_INVALIDARG; - *out = (HSTRING)buf; + *out = (HSTRING)priv; return S_OK; } diff --git a/dlls/combase/tests/string.c b/dlls/combase/tests/string.c index 713f08d342b..59a60f7b202 100644 --- a/dlls/combase/tests/string.c +++ b/dlls/combase/tests/string.c @@ -511,7 +511,6 @@ static void test_hstring_struct(void) prv = CONTAINING_RECORD(str, struct hstring_private, header); - todo_wine ok(prv->header.flags == 0, "Expected 0 in flags field, got %#x.\n", prv->header.flags); todo_wine ok(prv->header.length == 6, "Expected 6 in length field, got %u.\n", prv->header.length); @@ -548,7 +547,6 @@ static void test_hstring_struct(void) prv2 = CONTAINING_RECORD(str, struct hstring_private, header); ok(prv == prv2, "Pointers not identical.\n"); - todo_wine ok(prv2->header.flags == 1, "Expected HSTRING_REFERENCE_FLAG to be set, got %#x.\n", prv2->header.flags); todo_wine ok(prv2->header.length == 6, "Expected 6 in length field, got %u.\n", prv2->header.length); -- 2.34.1
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/combase/string.c | 46 ++++++++++++++++++------------------- dlls/combase/tests/string.c | 2 -- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/dlls/combase/string.c b/dlls/combase/string.c index 9443583d90e..a0a025d6889 100644 --- a/dlls/combase/string.c +++ b/dlls/combase/string.c @@ -31,13 +31,13 @@ WINE_DEFAULT_DEBUG_CHANNEL(winstring); struct hstring_header { UINT32 flags; + UINT32 length; }; struct hstring_private { struct hstring_header header; LPWSTR buffer; - UINT32 length; LONG refcount; }; @@ -69,7 +69,7 @@ static BOOL alloc_string(UINT32 len, HSTRING *out) priv->header.flags = 0; priv->buffer = (LPWSTR)(priv + 1); - priv->length = len; + priv->header.length = len; priv->refcount = 1; priv->buffer[len] = '\0'; @@ -126,7 +126,7 @@ HRESULT WINAPI WindowsCreateStringReference(LPCWSTR ptr, UINT32 len, return E_POINTER; priv->buffer = (LPWSTR)ptr; - priv->length = len; + priv->header.length = len; priv->header.flags = HSTRING_REFERENCE_FLAG; *out = (HSTRING)priv; @@ -168,7 +168,7 @@ HRESULT WINAPI WindowsDuplicateString(HSTRING str, HSTRING *out) return S_OK; } if (priv->header.flags == HSTRING_REFERENCE_FLAG) - return WindowsCreateString(priv->buffer, priv->length, out); + return WindowsCreateString(priv->buffer, priv->header.length, out); InterlockedIncrement(&priv->refcount); *out = str; return S_OK; @@ -233,7 +233,7 @@ HRESULT WINAPI WindowsPromoteStringBuffer(HSTRING_BUFFER buf, HSTRING *out) *out = NULL; return S_OK; } - if (priv->buffer[priv->length] != 0 || priv->header.flags == HSTRING_REFERENCE_FLAG || priv->refcount != 1) + if (priv->buffer[priv->header.length] != 0 || priv->header.flags == HSTRING_REFERENCE_FLAG || priv->refcount != 1) return E_INVALIDARG; *out = (HSTRING)priv; return S_OK; @@ -250,7 +250,7 @@ UINT32 WINAPI WindowsGetStringLen(HSTRING str) if (str == NULL) return 0; - return priv->length; + return priv->header.length; } /*********************************************************************** @@ -269,7 +269,7 @@ LPCWSTR WINAPI WindowsGetStringRawBuffer(HSTRING str, UINT32 *len) return empty; } if (len) - *len = priv->length; + *len = priv->header.length; return priv->buffer; } @@ -290,7 +290,7 @@ HRESULT WINAPI WindowsStringHasEmbeddedNull(HSTRING str, BOOL *out) *out = FALSE; return S_OK; } - for (i = 0; i < priv->length; i++) + for (i = 0; i < priv->header.length; i++) { if (priv->buffer[i] == '\0') { @@ -363,16 +363,16 @@ HRESULT WINAPI WindowsConcatString(HSTRING str1, HSTRING str2, HSTRING *out) return WindowsDuplicateString(str2, out); if (str2 == NULL) return WindowsDuplicateString(str1, out); - if (!priv1->length && !priv2->length) + if (!priv1->header.length && !priv2->header.length) { *out = NULL; return S_OK; } - if (!alloc_string(priv1->length + priv2->length, out)) + if (!alloc_string(priv1->header.length + priv2->header.length, out)) return E_OUTOFMEMORY; priv = impl_from_HSTRING(*out); - memcpy(priv->buffer, priv1->buffer, priv1->length * sizeof(*priv1->buffer)); - memcpy(priv->buffer + priv1->length, priv2->buffer, priv2->length * sizeof(*priv2->buffer)); + memcpy(priv->buffer, priv1->buffer, priv1->header.length * sizeof(*priv1->buffer)); + memcpy(priv->buffer + priv1->header.length, priv2->buffer, priv2->header.length * sizeof(*priv2->buffer)); return S_OK; } @@ -387,7 +387,7 @@ BOOL WINAPI WindowsIsStringEmpty(HSTRING str) if (str == NULL) return TRUE; - return priv->length == 0; + return priv->header.length == 0; } /*********************************************************************** @@ -412,12 +412,12 @@ HRESULT WINAPI WindowsCompareStringOrdinal(HSTRING str1, HSTRING str2, INT32 *re if (str1) { buf1 = priv1->buffer; - len1 = priv1->length; + len1 = priv1->header.length; } if (str2) { buf2 = priv2->buffer; - len2 = priv2->length; + len2 = priv2->header.length; } *res = CompareStringOrdinal(buf1, len1, buf2, len2, FALSE) - CSTR_EQUAL; return S_OK; @@ -434,19 +434,19 @@ HRESULT WINAPI WindowsTrimStringStart(HSTRING str1, HSTRING str2, HSTRING *out) TRACE("(%p, %p, %p)\n", str1, str2, out); - if (!out || !str2 || !priv2->length) + if (!out || !str2 || !priv2->header.length) return E_INVALIDARG; if (!str1) { *out = NULL; return S_OK; } - for (start = 0; start < priv1->length; start++) + for (start = 0; start < priv1->header.length; start++) { - if (!wmemchr(priv2->buffer, priv1->buffer[start], priv2->length)) + if (!wmemchr(priv2->buffer, priv1->buffer[start], priv2->header.length)) break; } - return start ? WindowsCreateString(&priv1->buffer[start], priv1->length - start, out) : + return start ? WindowsCreateString(&priv1->buffer[start], priv1->header.length - start, out) : WindowsDuplicateString(str1, out); } @@ -461,18 +461,18 @@ HRESULT WINAPI WindowsTrimStringEnd(HSTRING str1, HSTRING str2, HSTRING *out) TRACE("(%p, %p, %p)\n", str1, str2, out); - if (!out || !str2 || !priv2->length) + if (!out || !str2 || !priv2->header.length) return E_INVALIDARG; if (!str1) { *out = NULL; return S_OK; } - for (len = priv1->length; len > 0; len--) + for (len = priv1->header.length; len > 0; len--) { - if (!wmemchr(priv2->buffer, priv1->buffer[len - 1], priv2->length)) + if (!wmemchr(priv2->buffer, priv1->buffer[len - 1], priv2->header.length)) break; } - return (len < priv1->length) ? WindowsCreateString(priv1->buffer, len, out) : + return (len < priv1->header.length) ? WindowsCreateString(priv1->buffer, len, out) : WindowsDuplicateString(str1, out); } diff --git a/dlls/combase/tests/string.c b/dlls/combase/tests/string.c index 59a60f7b202..9e4c6281866 100644 --- a/dlls/combase/tests/string.c +++ b/dlls/combase/tests/string.c @@ -512,7 +512,6 @@ static void test_hstring_struct(void) prv = CONTAINING_RECORD(str, struct hstring_private, header); ok(prv->header.flags == 0, "Expected 0 in flags field, got %#x.\n", prv->header.flags); - todo_wine ok(prv->header.length == 6, "Expected 6 in length field, got %u.\n", prv->header.length); todo_wine ok(prv->header.str == prv->buffer, "Expected str to point at buffer, instead pointing at %p.\n", prv->header.str); @@ -548,7 +547,6 @@ static void test_hstring_struct(void) ok(prv == prv2, "Pointers not identical.\n"); ok(prv2->header.flags == 1, "Expected HSTRING_REFERENCE_FLAG to be set, got %#x.\n", prv2->header.flags); - todo_wine ok(prv2->header.length == 6, "Expected 6 in length field, got %u.\n", prv2->header.length); todo_wine ok(prv2->header.str == input_string, "Expected str to point at input_string, instead pointing at %p.\n", prv2->header.str); -- 2.34.1
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/combase/string.c | 32 ++++++++++++++++++-------------- dlls/combase/tests/string.c | 1 - 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/dlls/combase/string.c b/dlls/combase/string.c index a0a025d6889..24d1e4dd8f9 100644 --- a/dlls/combase/string.c +++ b/dlls/combase/string.c @@ -32,6 +32,9 @@ struct hstring_header { UINT32 flags; UINT32 length; + UINT32 padding1; + UINT32 padding2; + const WCHAR *str; }; struct hstring_private @@ -70,6 +73,7 @@ static BOOL alloc_string(UINT32 len, HSTRING *out) priv->header.flags = 0; priv->buffer = (LPWSTR)(priv + 1); priv->header.length = len; + priv->header.str = priv->buffer; priv->refcount = 1; priv->buffer[len] = '\0'; @@ -125,7 +129,7 @@ HRESULT WINAPI WindowsCreateStringReference(LPCWSTR ptr, UINT32 len, if (ptr == NULL) return E_POINTER; - priv->buffer = (LPWSTR)ptr; + priv->header.str = ptr; priv->header.length = len; priv->header.flags = HSTRING_REFERENCE_FLAG; @@ -168,7 +172,7 @@ HRESULT WINAPI WindowsDuplicateString(HSTRING str, HSTRING *out) return S_OK; } if (priv->header.flags == HSTRING_REFERENCE_FLAG) - return WindowsCreateString(priv->buffer, priv->header.length, out); + return WindowsCreateString(priv->header.str, priv->header.length, out); InterlockedIncrement(&priv->refcount); *out = str; return S_OK; @@ -270,7 +274,7 @@ LPCWSTR WINAPI WindowsGetStringRawBuffer(HSTRING str, UINT32 *len) } if (len) *len = priv->header.length; - return priv->buffer; + return priv->header.str; } /*********************************************************************** @@ -292,7 +296,7 @@ HRESULT WINAPI WindowsStringHasEmbeddedNull(HSTRING str, BOOL *out) } for (i = 0; i < priv->header.length; i++) { - if (priv->buffer[i] == '\0') + if (priv->header.str[i] == '\0') { *out = TRUE; return S_OK; @@ -321,7 +325,7 @@ HRESULT WINAPI WindowsSubstring(HSTRING str, UINT32 start, HSTRING *out) *out = NULL; return S_OK; } - return WindowsCreateString(&priv->buffer[start], len - start, out); + return WindowsCreateString(&priv->header.str[start], len - start, out); } /*********************************************************************** @@ -343,7 +347,7 @@ HRESULT WINAPI WindowsSubstringWithSpecifiedLength(HSTRING str, UINT32 start, UI *out = NULL; return S_OK; } - return WindowsCreateString(&priv->buffer[start], len, out); + return WindowsCreateString(&priv->header.str[start], len, out); } /*********************************************************************** @@ -371,8 +375,8 @@ HRESULT WINAPI WindowsConcatString(HSTRING str1, HSTRING str2, HSTRING *out) if (!alloc_string(priv1->header.length + priv2->header.length, out)) return E_OUTOFMEMORY; priv = impl_from_HSTRING(*out); - memcpy(priv->buffer, priv1->buffer, priv1->header.length * sizeof(*priv1->buffer)); - memcpy(priv->buffer + priv1->header.length, priv2->buffer, priv2->header.length * sizeof(*priv2->buffer)); + memcpy(priv->buffer, priv1->header.str, priv1->header.length * sizeof(*priv1->buffer)); + memcpy(priv->buffer + priv1->header.length, priv2->header.str, priv2->header.length * sizeof(*priv2->buffer)); return S_OK; } @@ -411,12 +415,12 @@ HRESULT WINAPI WindowsCompareStringOrdinal(HSTRING str1, HSTRING str2, INT32 *re } if (str1) { - buf1 = priv1->buffer; + buf1 = priv1->header.str; len1 = priv1->header.length; } if (str2) { - buf2 = priv2->buffer; + buf2 = priv2->header.str; len2 = priv2->header.length; } *res = CompareStringOrdinal(buf1, len1, buf2, len2, FALSE) - CSTR_EQUAL; @@ -443,10 +447,10 @@ HRESULT WINAPI WindowsTrimStringStart(HSTRING str1, HSTRING str2, HSTRING *out) } for (start = 0; start < priv1->header.length; start++) { - if (!wmemchr(priv2->buffer, priv1->buffer[start], priv2->header.length)) + if (!wmemchr(priv2->header.str, priv1->header.str[start], priv2->header.length)) break; } - return start ? WindowsCreateString(&priv1->buffer[start], priv1->header.length - start, out) : + return start ? WindowsCreateString(&priv1->header.str[start], priv1->header.length - start, out) : WindowsDuplicateString(str1, out); } @@ -470,9 +474,9 @@ HRESULT WINAPI WindowsTrimStringEnd(HSTRING str1, HSTRING str2, HSTRING *out) } for (len = priv1->header.length; len > 0; len--) { - if (!wmemchr(priv2->buffer, priv1->buffer[len - 1], priv2->header.length)) + if (!wmemchr(priv2->header.str, priv1->header.str[len - 1], priv2->header.length)) break; } - return (len < priv1->header.length) ? WindowsCreateString(priv1->buffer, len, out) : + return (len < priv1->header.length) ? WindowsCreateString(priv1->header.str, len, out) : WindowsDuplicateString(str1, out); } diff --git a/dlls/combase/tests/string.c b/dlls/combase/tests/string.c index 9e4c6281866..46cf58a271a 100644 --- a/dlls/combase/tests/string.c +++ b/dlls/combase/tests/string.c @@ -548,7 +548,6 @@ static void test_hstring_struct(void) ok(prv == prv2, "Pointers not identical.\n"); ok(prv2->header.flags == 1, "Expected HSTRING_REFERENCE_FLAG to be set, got %#x.\n", prv2->header.flags); ok(prv2->header.length == 6, "Expected 6 in length field, got %u.\n", prv2->header.length); - todo_wine ok(prv2->header.str == input_string, "Expected str to point at input_string, instead pointing at %p.\n", prv2->header.str); ok(WindowsDeleteString(str) == S_OK, "Failed to delete string ref.\n"); -- 2.34.1
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51017 Put the string buffer at the end of the struct to match the Windows behaviour and avoid unnecessary pointer arithmetic. Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- v5: Split the patch into multiple commits and minor code changes. v4: Remove leftover debugging TRACE and minor style changes. v3: Add nested hstring_header struct to hstring_private and add a test for both. v2: I was mistaken about no reference counting being used. --- dlls/combase/string.c | 8 ++++---- dlls/combase/tests/string.c | 8 -------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/dlls/combase/string.c b/dlls/combase/string.c index 24d1e4dd8f9..fa35262fbf9 100644 --- a/dlls/combase/string.c +++ b/dlls/combase/string.c @@ -40,8 +40,8 @@ struct hstring_header struct hstring_private { struct hstring_header header; - LPWSTR buffer; - LONG refcount; + LONG refcount; + WCHAR buffer[1]; }; static const WCHAR empty[1]; @@ -66,14 +66,14 @@ static inline struct hstring_private *impl_from_HSTRING_BUFFER(HSTRING_BUFFER bu static BOOL alloc_string(UINT32 len, HSTRING *out) { struct hstring_private *priv; - priv = HeapAlloc(GetProcessHeap(), 0, sizeof(*priv) + (len + 1) * sizeof(*priv->buffer)); + priv = HeapAlloc(GetProcessHeap(), 0, offsetof(struct hstring_private, buffer[len])); if (!priv) return FALSE; priv->header.flags = 0; - priv->buffer = (LPWSTR)(priv + 1); priv->header.length = len; priv->header.str = priv->buffer; + priv->refcount = 1; priv->buffer[len] = '\0'; diff --git a/dlls/combase/tests/string.c b/dlls/combase/tests/string.c index 46cf58a271a..232d37304d6 100644 --- a/dlls/combase/tests/string.c +++ b/dlls/combase/tests/string.c @@ -513,29 +513,21 @@ static void test_hstring_struct(void) ok(prv->header.flags == 0, "Expected 0 in flags field, got %#x.\n", prv->header.flags); ok(prv->header.length == 6, "Expected 6 in length field, got %u.\n", prv->header.length); - todo_wine ok(prv->header.str == prv->buffer, "Expected str to point at buffer, instead pointing at %p.\n", prv->header.str); - todo_wine ok(prv->refcount == 1, "Expected 1 in refcount, got %u.\n", prv->refcount); - todo_wine ok(wcscmp(input_string, prv->buffer) == 0, "Expected strings to match.\n"); - todo_wine ok(prv->buffer[prv->header.length] == '\0', "Expected buffer to be null terminated.\n"); ok(WindowsDuplicateString(str, &str2) == S_OK, "Failed to duplicate string.\n"); prv2 = CONTAINING_RECORD(str2, struct hstring_private, header); - todo_wine ok(prv->refcount == 2, "Expected 2 in refcount, got %u.\n", prv->refcount); - todo_wine ok(prv2->refcount == 2, "Expected 2 in refcount, got %u.\n", prv2->refcount); - todo_wine ok(wcscmp(input_string, prv2->buffer) == 0, "Expected strings to match.\n"); ok(WindowsDeleteString(str) == S_OK, "Failed to delete string.\n"); - todo_wine ok(prv->refcount == 1, "Expected 1 in refcount, got %u.\n", prv->refcount); ok(WindowsDeleteString(str) == S_OK, "Failed to delete string.\n"); -- 2.34.1
On Thu, Jan 13, 2022 at 12:34:18AM +0100, Bernhard Kölbl wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51017
Put the string buffer at the end of the struct to match the Windows behaviour and avoid unnecessary pointer arithmetic.
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- v5: Split the patch into multiple commits and minor code changes. v4: Remove leftover debugging TRACE and minor style changes. v3: Add nested hstring_header struct to hstring_private and add a test for both. v2: I was mistaken about no reference counting being used. --- dlls/combase/string.c | 8 ++++---- dlls/combase/tests/string.c | 8 -------- 2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/dlls/combase/string.c b/dlls/combase/string.c index 24d1e4dd8f9..fa35262fbf9 100644 --- a/dlls/combase/string.c +++ b/dlls/combase/string.c @@ -40,8 +40,8 @@ struct hstring_header struct hstring_private { struct hstring_header header; - LPWSTR buffer; - LONG refcount; + LONG refcount; + WCHAR buffer[1]; }
static const WCHAR empty[1]; @@ -66,14 +66,14 @@ static inline struct hstring_private *impl_from_HSTRING_BUFFER(HSTRING_BUFFER bu static BOOL alloc_string(UINT32 len, HSTRING *out) { struct hstring_private *priv; - priv = HeapAlloc(GetProcessHeap(), 0, sizeof(*priv) + (len + 1) * sizeof(*priv->buffer)); + priv = HeapAlloc(GetProcessHeap(), 0, offsetof(struct hstring_private, buffer[len]));
Should be [len + 1] (my fault, but you should have spotted it ;-) Huw.
Whoopsie.. Yeah, my bad. :) I'll send in a v6 shortly. Bernhard Am Mo., 24. Jan. 2022 um 11:47 Uhr schrieb Huw Davies <huw(a)codeweavers.com>:
On Thu, Jan 13, 2022 at 12:34:18AM +0100, Bernhard Kölbl wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51017
Put the string buffer at the end of the struct to match the Windows behaviour and avoid unnecessary pointer arithmetic.
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- v5: Split the patch into multiple commits and minor code changes. v4: Remove leftover debugging TRACE and minor style changes. v3: Add nested hstring_header struct to hstring_private and add a test for both. v2: I was mistaken about no reference counting being used. --- dlls/combase/string.c | 8 ++++---- dlls/combase/tests/string.c | 8 -------- 2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/dlls/combase/string.c b/dlls/combase/string.c index 24d1e4dd8f9..fa35262fbf9 100644 --- a/dlls/combase/string.c +++ b/dlls/combase/string.c @@ -40,8 +40,8 @@ struct hstring_header struct hstring_private { struct hstring_header header; - LPWSTR buffer; - LONG refcount; + LONG refcount; + WCHAR buffer[1]; }
static const WCHAR empty[1]; @@ -66,14 +66,14 @@ static inline struct hstring_private *impl_from_HSTRING_BUFFER(HSTRING_BUFFER bu static BOOL alloc_string(UINT32 len, HSTRING *out) { struct hstring_private *priv; - priv = HeapAlloc(GetProcessHeap(), 0, sizeof(*priv) + (len + 1) * sizeof(*priv->buffer)); + priv = HeapAlloc(GetProcessHeap(), 0, offsetof(struct hstring_private, buffer[len]));
Should be [len + 1] (my fault, but you should have spotted it ;-)
Huw.
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=105204 Your paranoid android. === debian11 (32 bit report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit Arabic:Morocco report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit German report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit French report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit Hebrew:Israel report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit Hindi:India report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit Japanese:Japan report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit Chinese:China report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6. === debian11 (32 bit WoW report) === combase: string.c:517: Test succeeded inside todo block: Expected 6 in length field, got 6. string.c:554: Test succeeded inside todo block: Expected 6 in length field, got 6.
On Thu, Jan 13, 2022 at 12:34:14AM +0100, Bernhard Kölbl wrote:
The goal of these tests is to show, that the memory layout of hstring_private is different on Windows, than currently used in Wine. This creates issues with the WinRT SDK, which allocates HSTRINGs without using the functions provided by the combase dll.
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/combase/tests/string.c | 80 +++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
diff --git a/dlls/combase/tests/string.c b/dlls/combase/tests/string.c index 5ebf669a426..713f08d342b 100644 --- a/dlls/combase/tests/string.c +++ b/dlls/combase/tests/string.c @@ -479,6 +479,85 @@ static void test_trim(void) ok(WindowsDeleteString(str1) == S_OK, "Failed to delete string\n"); }
Did you see that this had test failures? https://testbot.winehq.org/JobDetails.pl?Key=105204 Huw.
Hi Huw, yes I saw the failure, though fixing it only requires to remove a todo_wine. My idea was to await, if you have some other points which need to be changed about the patch series, so I can resend with those changes and the fixed test, to not spam the mailing list too much. Bernhard Huw Davies <huw(a)codeweavers.com> schrieb am Mo., 24. Jan. 2022, 09:30:
On Thu, Jan 13, 2022 at 12:34:14AM +0100, Bernhard Kölbl wrote:
The goal of these tests is to show, that the memory layout of hstring_private is different on Windows, than currently used in Wine. This creates issues with the WinRT SDK, which allocates HSTRINGs without using the functions provided by the combase dll.
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/combase/tests/string.c | 80 +++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
diff --git a/dlls/combase/tests/string.c b/dlls/combase/tests/string.c index 5ebf669a426..713f08d342b 100644 --- a/dlls/combase/tests/string.c +++ b/dlls/combase/tests/string.c @@ -479,6 +479,85 @@ static void test_trim(void) ok(WindowsDeleteString(str1) == S_OK, "Failed to delete string\n"); }
Did you see that this had test failures?
https://testbot.winehq.org/JobDetails.pl?Key=105204
Huw.
participants (3)
-
Bernhard Kölbl -
Huw Davies -
Marvin