[PATCH 0/2] MR8861: shell32: Accept CSIDL values in SHBrowseForFolderW().

[Attila Fidan (＠atticf)]

This fixes an access violation when clicking the button to browse for an installation directory in some installers using the BURIKO (BGI/Ethornell) engine. The affected applications call `SHBrowseForFolderA(&bi)` with `bi.pidlRoot` being 0x11, which causes wine to dereference 0x11 inside InitializeTreeView. On native, this opens the treeview from My Computer.

0x11 is the CSIDL value for My Computer, and passing any pidlRoot from 0 to 2^16 will not crash on native like resource IDs. If an invalid folder value is used then native returns 0 without showing a window. If I delete the My Pictures folder then use `CSIDL_MYPICTURES`, the folder gets recreated. None of this works if `BIF_NEWDIALOGSTYLE` is used, native crashes with access violation.

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

[PATCH 1/2] shell32/tests: Test SHBrowseForFolderW() with CSIDL values.

[Attila Fidan]

From: Attila Fidan <dev(a)print0.net>

---
 dlls/shell32/tests/brsfolder.c | 60 ++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/dlls/shell32/tests/brsfolder.c b/dlls/shell32/tests/brsfolder.c
index caccb13a326..40e571146d7 100644
--- a/dlls/shell32/tests/brsfolder.c
+++ b/dlls/shell32/tests/brsfolder.c
@@ -357,8 +357,68 @@ static void test_selection(void)
     CoUninitialize();
 }
 
+static void CALLBACK ok_timer_callback(HWND hwnd, UINT uMsg, UINT_PTR idEvent, DWORD dwTime)
+{
+    KillTimer(hwnd, idEvent);
+    SendMessageA(hwnd, WM_COMMAND, IDOK, 0);
+}
+
+static int CALLBACK csidl_pidlroot_callback(HWND hwnd, UINT uMsg, LPARAM lParam, LPARAM lpData)
+{
+    switch (uMsg)
+    {
+    case BFFM_INITIALIZED:
+        SetTimer(hwnd, 0, TIMER_WAIT_MS, ok_timer_callback);
+        return 1;
+    default:
+        return 0;
+    }
+}
+
+/*
+ * Demonstrate that setting only the least significant 16 bits of .pidlRoot with
+ * a CSIDL value doesn't crash. A treeview with the corresponding folder
+ * contents is shown. This doesn't work when BIF_NEWDIALOGSTYLE is used.
+ */
+static void test_csidl_pidlroot(void)
+{
+    HRESULT resCoInit;
+    BROWSEINFOW bi = {0};
+    LPITEMIDLIST pidl;
+    const WCHAR title[] = L"test_csidl_pidlroot";
+
+    resCoInit = CoInitialize(NULL);
+    if(!(resCoInit == S_OK || resCoInit == S_FALSE))
+    {
+        skip("COM could not be initialized %lu\n", GetLastError());
+        return;
+    }
+
+    bi.lpszTitle = title;
+    bi.lpfn = csidl_pidlroot_callback;
+
+    bi.pidlRoot = LongToPtr(CSIDL_DRIVES);
+    pidl = SHBrowseForFolderW(&bi);
+    ok(!!pidl, "SHBrowseForFolderW returned NULL pidl\n");
+    CoTaskMemFree(pidl);
+
+    bi.pidlRoot = LongToPtr(CSIDL_DRIVES | 0xff00);
+    pidl = SHBrowseForFolderW(&bi);
+    ok(!!pidl, "SHBrowseForFolderW returned NULL pidl\n");
+    CoTaskMemFree(pidl);
+
+    /* invalid CSIDL folder will not open a window */
+    bi.pidlRoot = LongToPtr(0x000c);
+    pidl = SHBrowseForFolderW(&bi);
+    todo_wine ok(!pidl, "SHBrowseForFolderW returned non-NULL pidl\n");
+    CoTaskMemFree(pidl);
+
+    CoUninitialize();
+}
+
 START_TEST(brsfolder)
 {
     test_click_make_new_folder_button();
     test_selection();
+    if (!winetest_platform_is_wine) test_csidl_pidlroot();
 }
-- 
GitLab


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

[PATCH 2/2] shell32: Accept CSIDL values in SHBrowseForFolderW().

[Attila Fidan]

From: Attila Fidan <dev(a)print0.net>

---
 dlls/shell32/brsfolder.c       | 15 +++++++++++++++
 dlls/shell32/tests/brsfolder.c |  2 +-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/dlls/shell32/brsfolder.c b/dlls/shell32/brsfolder.c
index ebaf01e7989..0fad56d83c9 100644
--- a/dlls/shell32/brsfolder.c
+++ b/dlls/shell32/brsfolder.c
@@ -188,9 +188,23 @@ static void InitializeTreeView( browse_info *info )
     IEnumIDList * pEnumChildren = NULL;
     HTREEITEM item;
     DWORD flags;
+    LPITEMIDLIST pidlCSIDL = NULL;
     LPCITEMIDLIST root = info->lpBrowseInfo->pidlRoot;
 
     TRACE("%p\n", info );
+
+    /* Some BURIKO engine installers depend on being able to pass CSIDL values
+     * in the pidlRoot member like resource identifiers. The folder is created
+     * if it doesn't exist, and this doesn't work on the new dialog style.
+     */
+    if (!(info->lpBrowseInfo->ulFlags & BIF_NEWDIALOGSTYLE) && IS_INTRESOURCE(root)) {
+        hr = SHGetFolderLocation(NULL, LOWORD(root) | CSIDL_FLAG_CREATE, NULL, 0, &pidlCSIDL);
+        if (FAILED(hr)) {
+            WARN("SHGetFolderLocation failed! hr = %08lx\n", hr);
+            return;
+        }
+        root = pidlCSIDL;
+    }
     
     Shell_GetImageLists(NULL, &hImageList);
 
@@ -212,6 +226,7 @@ static void InitializeTreeView( browse_info *info )
     pidlParent = ILClone(root);
     ILRemoveLastID(pidlParent);
     pidlChild = ILClone(ILFindLastID(root));
+    ILFree(pidlCSIDL);
     
     if (_ILIsDesktop(pidlParent)) {
         hr = SHGetDesktopFolder(&lpsfParent);
diff --git a/dlls/shell32/tests/brsfolder.c b/dlls/shell32/tests/brsfolder.c
index 40e571146d7..046da8eca21 100644
--- a/dlls/shell32/tests/brsfolder.c
+++ b/dlls/shell32/tests/brsfolder.c
@@ -420,5 +420,5 @@ START_TEST(brsfolder)
 {
     test_click_make_new_folder_button();
     test_selection();
-    if (!winetest_platform_is_wine) test_csidl_pidlroot();
+    test_csidl_pidlroot();
 }
-- 
GitLab

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

[Attila Fidan (＠atticf)]

I realized the browse_info's pidlRoot is accessed later in BrsFolder_OnSetExpanded when BFFM_SETEXPANDED/BFFM_SETSELECTIONA/W is sent, but when I re-used test_selection's selection_callback and selected_folder_pidl variable, native also crashes with access violation on the tests with FALSE wparam and non-zero lparams. The affected applications don't send these messages.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8861#note_114353