[PATCH v2 0/1] MR1633: comctl32: Use strdupAW / strdupWA helpers for CRT allocs.

[Rémi Bernon]

Fixing heap mismatches introduced with !1520.

--
  v2: comctl32: Use CRT heap allocation for some A/W string copies.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1633

[PATCH v2 1/1] comctl32: Use CRT heap allocation for some A/W string copies.

[Rémi Bernon]

From: Rémi Bernon <rbernon(a)codeweavers.com>

For strings later freed with CRT heap, to avoid heap mismatch.
---
 dlls/comctl32/comctl32.h | 25 +++++++++++++++++++++++++
 dlls/comctl32/header.c   | 12 +++++-------
 dlls/comctl32/listview.c | 16 +++++++---------
 3 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/dlls/comctl32/comctl32.h b/dlls/comctl32/comctl32.h
index f9ed8b84ffd..6d3148b3759 100644
--- a/dlls/comctl32/comctl32.h
+++ b/dlls/comctl32/comctl32.h
@@ -28,6 +28,7 @@
 #include <stdarg.h>
 #endif
 
+#include <stdlib.h>
 #include "windef.h"
 #include "winbase.h"
 #include "wingdi.h"
@@ -194,6 +195,30 @@ BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) DECLSPEC_HIDDEN;
 BOOL Str_SetPtrWtoA (LPSTR *lppDest, LPCWSTR lpSrc) DECLSPEC_HIDDEN;
 BOOL imagelist_has_alpha(HIMAGELIST, UINT) DECLSPEC_HIDDEN;
 
+static inline WCHAR *strdupAW( const char *src )
+{
+    WCHAR *dst = NULL;
+    if (src)
+    {
+        int len = MultiByteToWideChar( CP_ACP, 0, src, -1, NULL, 0 );
+        if ((dst = malloc( len * sizeof(WCHAR) )))
+            MultiByteToWideChar( CP_ACP, 0, src, -1, dst, len );
+    }
+    return dst;
+}
+
+static inline char *strdupWA( const WCHAR *src )
+{
+    char *dst = NULL;
+    if (src)
+    {
+        int len = WideCharToMultiByte( CP_ACP, 0, src, -1, NULL, 0, NULL, NULL );
+        if ((dst = malloc( len )))
+            WideCharToMultiByte( CP_ACP, 0, src, -1, dst, len, NULL, NULL );
+    }
+    return dst;
+}
+
 #define COMCTL32_VERSION_MINOR 81
 
 /* Our internal stack structure of the window procedures to subclass */
diff --git a/dlls/comctl32/header.c b/dlls/comctl32/header.c
index e905d18e2fa..7a0383909f4 100644
--- a/dlls/comctl32/header.c
+++ b/dlls/comctl32/header.c
@@ -147,9 +147,9 @@ static void HEADER_StoreHDItemInHeader(HEADER_ITEM *lpItem, UINT mask, const HDI
         {
             const WCHAR *pszText = phdi->pszText != NULL ? phdi->pszText : L"";
             if (fUnicode)
-                Str_SetPtrW(&lpItem->pszText, pszText);
+                lpItem->pszText = wcsdup(pszText);
             else
-                Str_SetPtrAtoW(&lpItem->pszText, (LPCSTR)pszText);
+                lpItem->pszText = strdupAW((LPCSTR)pszText);
             lpItem->callbackMask &= ~HDI_TEXT;
         }
         else
@@ -854,15 +854,13 @@ static void HEADER_CopyHDItemForNotify(const HEADER_INFO *infoPtr, HDITEMW *dest
     {
         if (fSourceUnicode && infoPtr->nNotifyFormat != NFR_UNICODE)
         {
-            dest->pszText = NULL;
-            Str_SetPtrWtoA((LPSTR *)&dest->pszText, src->pszText);
+            dest->pszText = (LPWSTR)strdupWA(src->pszText);
             *ppvScratch = dest->pszText;
         }
         
         if (!fSourceUnicode && infoPtr->nNotifyFormat == NFR_UNICODE)
         {
-            dest->pszText = NULL;
-            Str_SetPtrAtoW(&dest->pszText, (LPSTR)src->pszText);
+            dest->pszText = strdupAW((LPSTR)src->pszText);
             *ppvScratch = dest->pszText;
         }
     }
@@ -1029,7 +1027,7 @@ HEADER_PrepareCallbackItems(const HEADER_INFO *infoPtr, INT iItem, INT reqMask)
         }
         else
         {
-            Str_SetPtrAtoW(&lpItem->pszText, (LPSTR)dispInfo.pszText);
+            lpItem->pszText = strdupAW((LPSTR)dispInfo.pszText);
             free(pvBuffer);
         }
     }
diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c
index 116c3c5cf94..53d78d37b02 100644
--- a/dlls/comctl32/listview.c
+++ b/dlls/comctl32/listview.c
@@ -505,16 +505,16 @@ static BOOL textsetptrT(LPWSTR *dest, LPCWSTR src, BOOL isW)
 {
     BOOL bResult = TRUE;
     
+    if (is_text(*dest)) free(*dest);
     if (src == LPSTR_TEXTCALLBACKW)
-    {
-	if (is_text(*dest)) free(*dest);
 	*dest = LPSTR_TEXTCALLBACKW;
-    }
     else
     {
 	LPWSTR pszText = textdupTtoW(src, isW);
-	if (*dest == LPSTR_TEXTCALLBACKW) *dest = NULL;
-	bResult = Str_SetPtrW(dest, pszText);
+	if (!pszText)
+		*dest = NULL;
+	else if (!(*dest = wcsdup(pszText)))
+		bResult = FALSE;
 	textfreeT(pszText, isW);
     }
     return bResult;
@@ -788,16 +788,14 @@ static LRESULT notify_forward_header(const LISTVIEW_INFO *infoPtr, NMHEADERW *lp
         if (lpnmh->pitem->mask & HDI_TEXT)
         {
             text = (LPCWSTR)lpnmh->pitem->pszText;
-            lpnmh->pitem->pszText = NULL;
-            Str_SetPtrWtoA(&lpnmh->pitem->pszText, text);
+            lpnmh->pitem->pszText = strdupWA(text);
         }
         /* convert filter text */
         if ((lpnmh->pitem->mask & HDI_FILTER) && (lpnmh->pitem->type == HDFT_ISSTRING) &&
              lpnmh->pitem->pvFilter)
         {
             filter = (LPCWSTR)((HD_TEXTFILTERA*)lpnmh->pitem->pvFilter)->pszText;
-            ((HD_TEXTFILTERA*)lpnmh->pitem->pvFilter)->pszText = NULL;
-            Str_SetPtrWtoA(&((HD_TEXTFILTERA*)lpnmh->pitem->pvFilter)->pszText, filter);
+            ((HD_TEXTFILTERA*)lpnmh->pitem->pvFilter)->pszText = strdupWA(filter);
         }
     }
     lpnmh->hdr.code = get_ansi_notification(lpnmh->hdr.code);
-- 
GitLab

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

[PATCH v2 1/1] comctl32: Use CRT heap allocation for some A/W string copies.

[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 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=127070

Your paranoid android.


=== debian11 (build log) ===

WineRunWineTest.pl:error: The previous 1 run(s) terminated abnormally

[Rémi Bernon]

v2: Fix one more mismatch in listview.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1633#note_18250

[Rémi Bernon]

Feel free to merge this into https://gitlab.winehq.org/wine/wine/-/merge_requests/1625, or do it differently (like converting all allocations at once).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1633#note_18255

[Nikolay Sivov (＠nsivov)]

On Thu Dec  1 16:17:58 2022 +0000, Rémi Bernon wrote:
> Feel free to merge this into
> https://gitlab.winehq.org/wine/wine/-/merge_requests/1625, or do it
> differently (like converting all allocations at once).
> Yes, converting at once seems better at this point. There is too much to miss. Those _StrW helpers could stay, because of a side effect when setting null strings.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1633#note_18272

[Nikolay Sivov (＠nsivov)]

This was reverted to avoid such mismatches for now, with !1758.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1633#note_19390

[PATCH v2 0/1] MR1633: comctl32: Use strdupAW / strdupWA helpers for CRT allocs. - closed

[Nikolay Sivov (＠nsivov)]

This merge request was closed by Nikolay Sivov.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1633