[PATCH v2 0/2] MR6657: comdlg32: Fix buffer overflow when current_filter is longer than MAX_PATH.

[Charlotte Pabst (＠CharlottePabst)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57293

Without this change, the tests segfault due to a buffer overflow.

Note: This bug was surfaced but not caused by b5cbb5562ca1aaf023a588bdc6ccb89220637fc4. Before b5cbb5562ca1aaf023a588bdc6ccb89220637fc4, the savedlg test segfaults while the opendlg test does not. After it, both segfault.

--
  v2: comdlg32/tests: Add tests for itemdlg filters longer than MAX_PATH.

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

[PATCH v2 1/2] comdlg32: Fix buffer overflow when current_filter is longer than MAX_PATH.

[Charlotte Pabst]

From: Charlotte Pabst

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57293
---
 dlls/comdlg32/itemdlg.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/dlls/comdlg32/itemdlg.c b/dlls/comdlg32/itemdlg.c
index 439a6b2214a..99af524a06f 100644
--- a/dlls/comdlg32/itemdlg.c
+++ b/dlls/comdlg32/itemdlg.c
@@ -558,10 +558,17 @@ static void fill_filename_from_selection(FileDialogImpl *This)
 static LPWSTR get_first_ext_from_spec(LPWSTR buf, LPCWSTR spec)
 {
     WCHAR *endpos, *ext;
+    INT len;

-    lstrcpyW(buf, spec);
-    if( (endpos = StrChrW(buf, ';')) )
-        *endpos = '\0';
+    if( (endpos = StrChrW(spec, ';')) )
+        len = endpos-spec+1;
+    else
+        len = lstrlenW(spec)+1;
+
+    if (len > MAX_PATH)
+        return NULL;
+
+    lstrcpynW(buf, spec, len);

ext = PathFindExtensionW(buf);
     if(StrChrW(ext, '*'))

[PATCH v2 2/2] comdlg32/tests: Add tests for itemdlg filters longer than MAX_PATH.

[Charlotte Pabst]

From: Charlotte Pabst

---
 dlls/comdlg32/tests/itemdlg.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/dlls/comdlg32/tests/itemdlg.c b/dlls/comdlg32/tests/itemdlg.c
index 00ce13a1be1..3048f2a9760 100644
--- a/dlls/comdlg32/tests/itemdlg.c
+++ b/dlls/comdlg32/tests/itemdlg.c
@@ -1430,7 +1430,7 @@ static void test_filename_opendlg_(LPCWSTR set_filename, LPCWSTR set_filename2,

if(fs_count)
     {
-        hr = IFileOpenDialog_SetFileTypes(pfod, 2, filterspec);
+        hr = IFileOpenDialog_SetFileTypes(pfod, fs_count, filterspec);
         ok_(file,line)(hr == S_OK, "SetFileTypes failed: Got 0x%08lx\n", hr);
     }

@@ -1532,6 +1532,8 @@ static void test_filename(void)
     static const WCHAR ext2[] = {'*','.','w','t','2',0};
     static const WCHAR extdef[] = {'*','.','w','t','e',0};
     static const WCHAR complexext[] = {'*','.','w','t','2',';','*','.','w','t','1',0};
+    /* 300 chars, to be longer than MAX_PATH=260 */
+    static const WCHAR *const long_ext = L"*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;*.wt2;";

static const COMDLG_FILTERSPEC filterspec[] = {
         { desc1, ext1 }, { desc2, ext2 }, { descdef, extdef }
@@ -1539,6 +1541,9 @@ static void test_filename(void)
     static const COMDLG_FILTERSPEC filterspec2[] = {
         { desc1, complexext }
     };
+    static const COMDLG_FILTERSPEC filterspec_long[] = {
+        { desc2, long_ext }
+    };

/* No extension */
     test_filename_savedlg(filename_noextW, NULL, NULL, NULL, 0, 0, filename_noextW);
@@ -1562,6 +1567,8 @@ static void test_filename(void)
     test_filename_savedlg(filename_mixedcaseW, NULL, defextW, filterspec, 0, 0, filename_mixedcaseW);
     /* Default extension, filterspec with default extension, filename filter */
     test_filename_savedlg(filename_noextW, ext2, defextW, filterspec, 3, 0, filename_ext2W);
+    /* Default extension, filterspec exceeds MAX_PATH */
+    test_filename_savedlg(filename_noextW, NULL, defextW, filterspec_long, 1, 0, filename_ext2W);

GetCurrentDirectoryW(MAX_PATH, buf);
     ok(!!pSHCreateItemFromParsingName, "SHCreateItemFromParsingName is missing.\n");
@@ -1584,6 +1591,9 @@ static void test_filename(void)
     test_filename_opendlg(filename_noextW, NULL, psi_current, defextW, NULL, 0, 0, filename_defextW);
     /* IFileOpenDialog, default extension and filename filter, noextW deleted */
     test_filename_opendlg(filename_noextW, ext2, psi_current, defextW, NULL, 0, 0, filename_ext2W);
+    /* IFileOpenDialog, default extension, filterspec exceeds MAX_PATH */
+    test_filename_opendlg(filename_noextW, NULL, psi_current, defextW, filterspec_long, 1, 0, filename_ext2W);
+
     if(0) /* Interactive */
     {
     /* IFileOpenDialog, filterspec, no default extension, noextW deleted */

[PATCH v2 2/2] comdlg32/tests: Add tests for itemdlg filters longer than MAX_PATH.

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

Your paranoid android.

=== debian11b (64 bit WoW report) ===

user32:
input.c:5877: Test failed: got pos (49,51)