[PATCH 0/1] MR8204: comctl32: Fix use-after-free when page creation fails.

[Yuxuan Shui (＠yshui)]

When page creation fails in PROPSHEET_SetCurSel, the dialog window is destroyed, which means the
attached PropSheetInfo (psInfo) is freed. Because there is a call to PROPSHEET_SetCurSel in the
handling of WM_INITDIAG, any use of psInfo thereafter could be invalid, which wasn't taken into
account.

* * *

this feels a bit iffy but i think this is the right fix? previously we got 0 return value from `PropertySheet` because `do_loop` calls `IsWindow(hwnd)`, and when page creation failed the `hwnd` is destroyed, which resulted in `do_loop` returning 0. i just duplicated this check in `PROPSHEET_PropertySheet` too. as for the change in `DiaglogProc`, if `PROPSHEET_SetCurSel` destroyed the window then the SendMessageW isn't necessary, i believe?

also, this bit is redundant i think?

```c
      /* wizards set their focus during init */
      if (psInfo->ppshheader.dwFlags & INTRNL_ANY_WIZARD)
          return FALSE;

      return FALSE;
```

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

[PATCH 1/1] comctl32: Fix use-after-free when page creation fails.

[Yuxuan Shui]

From: Yuxuan Shui <yshui(a)codeweavers.com>

When page creation fails in PROPSHEET_SetCurSel, the dialog window is destroyed, which means the
attached PropSheetInfo (psInfo) is freed. Because there is a call to PROPSHEET_SetCurSel in the
handling of WM_INITDIAG, any use of psInfo thereafter could be invalid, which wasn't taken into
account.
---
 dlls/comctl32/propsheet.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/dlls/comctl32/propsheet.c b/dlls/comctl32/propsheet.c
index 96a35338648..18ccbd270b9 100644
--- a/dlls/comctl32/propsheet.c
+++ b/dlls/comctl32/propsheet.c
@@ -3009,7 +3009,9 @@ static INT_PTR PROPSHEET_PropertySheet(PropSheetInfo* psInfo, BOOL unicode)
       if (parent) EnableWindow(parent, FALSE);
   }
   bRet = PROPSHEET_CreateDialog(psInfo);
-  if(!psInfo->isModeless)
+  if (!IsWindow((HWND)bRet))
+      bRet = 0;
+  else if(!psInfo->isModeless)
       bRet = do_loop(psInfo);
   return bRet;
 }
@@ -3661,7 +3663,8 @@ PROPSHEET_DialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
       idx = psInfo->active_page;
       psInfo->active_page = -1;
 
-      PROPSHEET_SetCurSel(hwnd, idx, 1, psInfo->proppage[idx].hpage);
+      if (!PROPSHEET_SetCurSel(hwnd, idx, 1, psInfo->proppage[idx].hpage))
+          return FALSE;
 
       /* doing TCM_SETCURSEL seems to be needed even in case of PSH_WIZARD,
        * as some programs call TCM_GETCURSEL to get the current selection
-- 
GitLab

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

[Nikolay Sivov (＠nsivov)]

I think there are two parts here. If PROPSHEET_SetCurSel() fails in WM_INITDIALOG we could probably check for that and use EndDialog() or something like that. DestroyWindow does not look right in this context. It's too complicated to tell right away what's best, for example PROPSHEET_CreatePage() failing calls PROPSHEET_RemovePage() which at the very least is counterintuitive - why remove something that we couldn't create. The other question is if PROPSHEET_SetCurSel() should be able to destroy the whole thing to begin with.

To me it looks like one of those changes that could require some rework and manual testing of what we already have. Checking just IsWindow() on some value that we know is not always a window handle looks like a quick fix.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8204#note_105361