[PATCH 0/1] MR8239: shell32: Retrieve shell autocompletion strings one at a time.

[Andrew Nguyen (＠arethusa)]

When an instance of IEnumString in the ETQW World Editor is used to provide shell autocompletion strings, a crash can occur because its implementation of IEnumString::Next never initializes the output number of strings returned on success, which results in the uninitialized count being used to expand the enumerated strings array without bound.

To determine if a string was successfully retrieved from IEnumString::Next, the enumeration of autocompletion strings now retrieves only one string at a time and checks the returned HRESULT for appropriate success. This avoids reliance on the output count for determining success.

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

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

[PATCH 1/1] shell32: Retrieve shell autocompletion strings one at a time.

[Andrew Nguyen]

From: Andrew Nguyen <arethusa26(a)gmail.com>

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51630
---
 dlls/shell32/autocomplete.c       |  15 ++--
 dlls/shell32/tests/autocomplete.c | 123 ++++++++++++++++++++++++++----
 2 files changed, 120 insertions(+), 18 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index e8b064c6daf..d027722cd50 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -205,7 +205,7 @@ static void enumerate_strings(IAutoCompleteImpl *ac, enum prefix_filtering pfx_f
 {
     UINT cur = 0, array_size = 1024;
     LPOLESTR *strs = NULL, *tmp;
-    ULONG read;
+    BOOL str_read = FALSE;
 
     do
     {
@@ -215,12 +215,17 @@ static void enumerate_strings(IAutoCompleteImpl *ac, enum prefix_filtering pfx_f
 
         do
         {
-            if (FAILED(IEnumString_Next(ac->enumstr, array_size - cur, &strs[cur], &read)))
-                read = 0;
-        } while (read != 0 && (cur += read) < array_size);
+            /* An implementation of IEnumString::Next in the ETQW World Editor
+               never initializes the output number of strings returned, so to
+               determine if a string was successfully retrieved, just enumerate
+               one string at a time and check the returned HRESULT. */
+            ULONG dummy;
+            HRESULT hr = IEnumString_Next(ac->enumstr, 1, &strs[cur], &dummy);
+            str_read = SUCCEEDED(hr) && hr != S_FALSE;
+        } while (str_read && ++cur < array_size);
 
         array_size *= 2;
-    } while (read != 0);
+    } while (str_read);
 
     /* Allocate even if there were zero strings enumerated, to mark it non-NULL */
     if ((tmp = realloc(strs, cur * sizeof(*strs))))
diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c
index aa5f023b858..42ab7af4c67 100644
--- a/dlls/shell32/tests/autocomplete.c
+++ b/dlls/shell32/tests/autocomplete.c
@@ -275,6 +275,9 @@ struct string_enumerator
     int cur;
     UINT num_resets;
     UINT num_expand;
+    UINT num_next;
+    HRESULT no_next_str_hr;
+    HRESULT next_str_hr;
     WCHAR last_expand[32];
 };
 
@@ -324,24 +327,27 @@ static ULONG WINAPI string_enumerator_Release(IEnumString *iface)
 static HRESULT WINAPI string_enumerator_Next(IEnumString *iface, ULONG num, LPOLESTR *strings, ULONG *num_returned)
 {
     struct string_enumerator *this = impl_from_IEnumString(iface);
-    int i, len;
+    int len;
 
-    *num_returned = 0;
-    for (i = 0; i < num; i++)
-    {
-        if (this->cur >= this->data_len)
-            return S_FALSE;
+    this->num_next++;
 
-        len = lstrlenW(this->data[this->cur]) + 1;
+    /* Strings are always enumerated one at a time. */
+    ok(num == 1, "Expected request for only 1 string, got %lu\n", num);
 
-        strings[i] = CoTaskMemAlloc(len * sizeof(WCHAR));
-        memcpy(strings[i], this->data[this->cur], len * sizeof(WCHAR));
+    /* Write a bogus value through num_returned to show it is completely ignored. */
+    *num_returned = 0xdeadbeef;
 
-        (*num_returned)++;
-        this->cur++;
-    }
+    if (this->cur >= this->data_len)
+        return this->no_next_str_hr;
 
-    return S_OK;
+    len = lstrlenW(this->data[this->cur]) + 1;
+
+    strings[0] = CoTaskMemAlloc(len * sizeof(WCHAR));
+    memcpy(strings[0], this->data[this->cur], len * sizeof(WCHAR));
+
+    this->cur++;
+
+    return this->next_str_hr;
 }
 
 static HRESULT WINAPI string_enumerator_Reset(IEnumString *iface)
@@ -432,6 +438,8 @@ static HRESULT string_enumerator_create(void **ppv, WCHAR **suggestions, int cou
     object->data = suggestions;
     object->data_len = count;
     object->cur = 0;
+    object->no_next_str_hr = S_FALSE;
+    object->next_str_hr = S_OK;
 
     *ppv = &object->IEnumString_iface;
 
@@ -687,6 +695,94 @@ static void test_prefix_filtering(HWND hwnd_edit)
     IUnknown_Release(enumerator);
 }
 
+static void test_string_enumerator(HWND hwnd_edit)
+{
+    static WCHAR str0[] = L"aa";
+    static WCHAR *suggestions[] = { str0 };
+    IUnknown *enumerator;
+    struct string_enumerator *obj;
+    IAutoComplete2 *autocomplete;
+    IAutoCompleteDropDown *acdropdown;
+    WCHAR buffer[20];
+    HRESULT hr;
+
+    hr = CoCreateInstance(&CLSID_AutoComplete, NULL, CLSCTX_INPROC_SERVER, &IID_IAutoComplete2, (void**)&autocomplete);
+    ok(hr == S_OK, "CoCreateInstance failed: %lx\n", hr);
+
+    hr = IAutoComplete2_QueryInterface(autocomplete, &IID_IAutoCompleteDropDown, (LPVOID*)&acdropdown);
+    ok(hr == S_OK, "No IAutoCompleteDropDown interface: %lx\n", hr);
+
+    string_enumerator_create((void**)&enumerator, suggestions, ARRAY_SIZE(suggestions));
+    obj = (struct string_enumerator*)enumerator;
+
+    hr = IAutoComplete2_SetOptions(autocomplete, ACO_AUTOSUGGEST | ACO_AUTOAPPEND);
+    ok(hr == S_OK, "IAutoComplete2_SetOptions failed: %lx\n", hr);
+    hr = IAutoComplete2_Init(autocomplete, hwnd_edit, enumerator, NULL, NULL);
+    ok(hr == S_OK, "IAutoComplete_Init failed: %lx\n", hr);
+    ok(obj->num_next == 0, "Expected 0 next calls, got %u\n", obj->num_next);
+
+    /* Returning S_FALSE immediately from the string enumerator Next method yields
+     * no usable suggestion strings. */
+    obj->data_len = 0;
+    obj->num_next = 0;
+    obj->no_next_str_hr = S_FALSE;
+    obj->next_str_hr = S_OK;
+    SendMessageW(hwnd_edit, EM_SETSEL, 0, -1);
+    SendMessageW(hwnd_edit, WM_CHAR, 'a', 1);
+    dispatch_messages();
+    ok(obj->num_next == 1, "Expected 1 next call, got %u\n", obj->num_next);
+    SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer);
+    ok(lstrcmpW(L"a", buffer) == 0, "Expected \"a\", got %s\n", wine_dbgstr_w(buffer));
+    hr = IAutoCompleteDropDown_ResetEnumerator(acdropdown);
+    ok(hr == S_OK, "IAutoCompleteDropDown_ResetEnumerator failed: %lx\n", hr);
+
+    /* Returning a failure HRESULT in the same scenario yields the same behavior. */
+    obj->data_len = 0;
+    obj->num_next = 0;
+    obj->no_next_str_hr = E_NOTIMPL;
+    obj->next_str_hr = S_OK;
+    SendMessageW(hwnd_edit, EM_SETSEL, 0, -1);
+    SendMessageW(hwnd_edit, WM_CHAR, 'a', 1);
+    dispatch_messages();
+    ok(obj->num_next == 1, "Expected 1 next call, got %u\n", obj->num_next);
+    SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer);
+    ok(lstrcmpW(L"a", buffer) == 0, "Expected \"a\", got %s\n", wine_dbgstr_w(buffer));
+    hr = IAutoCompleteDropDown_ResetEnumerator(acdropdown);
+    ok(hr == S_OK, "IAutoCompleteDropDown_ResetEnumerator failed: %lx\n", hr);
+
+    /* Any string retrieved when returning S_FALSE from the string enumerator
+     * Next method is ignored. */
+    obj->data_len = ARRAY_SIZE(suggestions);
+    obj->num_next = 0;
+    obj->no_next_str_hr = S_FALSE;
+    obj->next_str_hr = S_FALSE;
+    SendMessageW(hwnd_edit, EM_SETSEL, 0, -1);
+    SendMessageW(hwnd_edit, WM_CHAR, 'a', 1);
+    dispatch_messages();
+    ok(obj->num_next == 1, "Expected 1 next call, got %u\n", obj->num_next);
+    SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer);
+    ok(lstrcmpW(L"a", buffer) == 0, "Expected \"a\", got %s\n", wine_dbgstr_w(buffer));
+    hr = IAutoCompleteDropDown_ResetEnumerator(acdropdown);
+    ok(hr == S_OK, "IAutoCompleteDropDown_ResetEnumerator failed: %lx\n", hr);
+
+    /* Returning a failure HRESULT in the same scenario yields the same behavior. */
+    obj->data_len = ARRAY_SIZE(suggestions);
+    obj->num_next = 0;
+    obj->no_next_str_hr = S_FALSE;
+    obj->next_str_hr = E_NOTIMPL;
+    SendMessageW(hwnd_edit, EM_SETSEL, 0, -1);
+    SendMessageW(hwnd_edit, WM_CHAR, 'a', 1);
+    dispatch_messages();
+    ok(obj->num_next == 1, "Expected 1 next call, got %u\n", obj->num_next);
+    SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer);
+    ok(lstrcmpW(L"a", buffer) == 0, "Expected \"a\", got %s\n", wine_dbgstr_w(buffer));
+    hr = IAutoCompleteDropDown_ResetEnumerator(acdropdown);
+    ok(hr == S_OK, "IAutoCompleteDropDown_ResetEnumerator failed: %lx\n", hr);
+
+    IAutoComplete2_Release(autocomplete);
+    IUnknown_Release(enumerator);
+}
+
 static void test_custom_source(void)
 {
     static WCHAR str_alpha[] = L"test1";
@@ -805,6 +901,7 @@ static void test_custom_source(void)
     IUnknown_Release(enumerator);
 
     test_prefix_filtering(hwnd_edit);
+    test_string_enumerator(hwnd_edit);
 
     ShowWindow(hMainWnd, SW_HIDE);
     DestroyWindow(hwnd_edit);
-- 
GitLab

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