[PATCH 1/3] shell32/autocomplete: Don't crash when there's another AutoComplete object on the same edit control
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22333 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 99ce23d..5fe48e9 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -93,6 +93,14 @@ static inline IAutoCompleteImpl *impl_from_IAutoCompleteDropDown(IAutoCompleteDr return CONTAINING_RECORD(iface, IAutoCompleteImpl, IAutoCompleteDropDown_iface); } +static void destroy_autocomplete_object(IAutoCompleteImpl *ac) +{ + ac->hwndEdit = NULL; + if (ac->hwndListBox) + DestroyWindow(ac->hwndListBox); + IAutoComplete2_Release(&ac->IAutoComplete2_iface); +} + /* Window procedure for autocompletion */ @@ -276,10 +284,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, RemovePropW(hwnd, autocomplete_propertyW); SetWindowLongPtrW(hwnd, GWLP_WNDPROC, (LONG_PTR)proc); - This->hwndEdit = NULL; - if (This->hwndListBox) - DestroyWindow(This->hwndListBox); - IAutoComplete2_Release(&This->IAutoComplete2_iface); + destroy_autocomplete_object(This); return CallWindowProcW(proc, hwnd, uMsg, wParam, lParam); } default: @@ -434,7 +439,7 @@ static HRESULT WINAPI IAutoComplete2_fnInit( LPCOLESTR pwzsRegKeyPath, LPCOLESTR pwszQuickComplete) { - IAutoCompleteImpl *This = impl_from_IAutoComplete2(iface); + IAutoCompleteImpl *Other, *This = impl_from_IAutoComplete2(iface); TRACE("(%p)->(%p, %p, %s, %s)\n", This, hwndEdit, punkACL, debugstr_w(pwzsRegKeyPath), debugstr_w(pwszQuickComplete)); @@ -461,10 +466,22 @@ static HRESULT WINAPI IAutoComplete2_fnInit( This->initialized = TRUE; This->hwndEdit = hwndEdit; - This->wpOrigEditProc = (WNDPROC) SetWindowLongPtrW( hwndEdit, GWLP_WNDPROC, (LONG_PTR) ACEditSubclassProc); - /* Keep at least one reference to the object until the edit window is destroyed. */ + + /* If another AutoComplete object was previously assigned to this edit control, + release it but keep the same callback on the control, to avoid an infinite + recursive loop in ACEditSubclassProc while the property is set to this object */ + Other = GetPropW(hwndEdit, autocomplete_propertyW); + SetPropW(hwndEdit, autocomplete_propertyW, This); + + if (Other && Other->initialized) { + This->wpOrigEditProc = Other->wpOrigEditProc; + destroy_autocomplete_object(Other); + } + else + This->wpOrigEditProc = (WNDPROC) SetWindowLongPtrW(hwndEdit, GWLP_WNDPROC, (LONG_PTR) ACEditSubclassProc); + + /* Keep at least one reference to the object until the edit window is destroyed */ IAutoComplete2_AddRef(&This->IAutoComplete2_iface); - SetPropW( hwndEdit, autocomplete_propertyW, This ); if (This->options & ACO_AUTOSUGGEST) create_listbox(This); -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This new test will crash under Wine without the bugfix applied. dlls/shell32/tests/autocomplete.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c index 1c51e51..3c8de88 100644 --- a/dlls/shell32/tests/autocomplete.c +++ b/dlls/shell32/tests/autocomplete.c @@ -141,7 +141,7 @@ if (0) static IAutoComplete *test_init(void) { HRESULT r; - IAutoComplete *ac; + IAutoComplete *ac, *ac2; IUnknown *acSource; LONG_PTR user_data; @@ -176,6 +176,15 @@ static IAutoComplete *test_init(void) user_data = GetWindowLongPtrA(hEdit, GWLP_USERDATA); ok(user_data == 0, "Expected the edit control user data to be zero\n"); + /* bind a different object to the same edit control */ + r = CoCreateInstance(&CLSID_AutoComplete, NULL, CLSCTX_INPROC_SERVER, + &IID_IAutoComplete, (LPVOID*)&ac2); + ok(r == S_OK, "no IID_IAutoComplete (0x%08x)\n", r); + + r = IAutoComplete_Init(ac2, hEdit, acSource, NULL, NULL); + ok(r == S_OK, "Init returned 0x%08x\n", r); + IAutoComplete_Release(ac2); + IUnknown_Release(acSource); return ac; -- 1.9.1
Handle heap_alloc failure, reg strings without a \ character at all, try harder to find the reg path (if only value fails the lookup), and properly read the registry value with arbitrary size, even REG_SZ without a NUL terminator (MSDN states they are possible). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 93 +++++++++++++++++++++++++++++---------------- 1 file changed, 61 insertions(+), 32 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 5fe48e9..97ce61a 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -486,40 +486,69 @@ static HRESULT WINAPI IAutoComplete2_fnInit( if (This->options & ACO_AUTOSUGGEST) create_listbox(This); - if (pwzsRegKeyPath) { - WCHAR *key; - WCHAR result[MAX_PATH]; - WCHAR *value; - HKEY hKey = 0; - LONG res; - LONG len; - - /* pwszRegKeyPath contains the key as well as the value, so we split */ - key = heap_alloc((lstrlenW(pwzsRegKeyPath)+1)*sizeof(WCHAR)); - strcpyW(key, pwzsRegKeyPath); - value = strrchrW(key, '\\'); - *value = 0; - value++; - /* Now value contains the value and buffer the key */ - res = RegOpenKeyExW(HKEY_CURRENT_USER, key, 0, KEY_READ, &hKey); - if (res != ERROR_SUCCESS) { - /* if the key is not found, MSDN states we must seek in HKEY_LOCAL_MACHINE */ - res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey); - } - if (res == ERROR_SUCCESS) { - res = RegQueryValueW(hKey, value, result, &len); - if (res == ERROR_SUCCESS) { - This->quickComplete = heap_alloc(len*sizeof(WCHAR)); - strcpyW(This->quickComplete, result); - } - RegCloseKey(hKey); - } - heap_free(key); + if (pwzsRegKeyPath) + { + WCHAR *key, *value; + HKEY hKey; + LSTATUS res; + size_t len; + + /* pwszRegKeyPath contains the key as well as the value, so split it */ + value = strrchrW(pwzsRegKeyPath, '\\'); + len = value - pwzsRegKeyPath; + + if (value && (key = heap_alloc((len+1) * sizeof(*key))) != NULL) { + memcpy(key, pwzsRegKeyPath, len * sizeof(*key)); + key[len] = '\0'; + value++; + + res = RegOpenKeyExW(HKEY_CURRENT_USER, key, 0, KEY_READ, &hKey); + + /* if not found, MSDN states we must seek in HKEY_LOCAL_MACHINE */ + if (res != ERROR_SUCCESS) + res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey); + + if (res == ERROR_SUCCESS) { + WCHAR *qc; + DWORD type, sz; + res = RegQueryValueExW(hKey, value, NULL, &type, NULL, &sz); + + if (res != ERROR_SUCCESS || type != REG_SZ) { + RegCloseKey(hKey); + res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey); + if (res != ERROR_SUCCESS) + goto free_key_str; + res = RegQueryValueExW(hKey, value, NULL, &type, NULL, &sz); + } + + /* the size does include the NUL terminator, however, strings + can be stored without it, so make room for NUL just in case */ + if (res == ERROR_SUCCESS && type == REG_SZ && + (qc = heap_alloc(sz + sizeof(*qc))) != NULL) + { + DWORD old_sz = sz; + res = RegQueryValueExW(hKey, value, NULL, &type, (BYTE*)qc, &sz); + + /* make sure the value wasn't messed with */ + if (res == ERROR_SUCCESS && sz == old_sz && type == REG_SZ) { + qc[sz / sizeof(*qc)] = '\0'; + This->quickComplete = qc; + } + else + heap_free(qc); + } + RegCloseKey(hKey); + } + free_key_str: + heap_free(key); + } } - if ((pwszQuickComplete) && (!This->quickComplete)) { - This->quickComplete = heap_alloc((lstrlenW(pwszQuickComplete)+1)*sizeof(WCHAR)); - lstrcpyW(This->quickComplete, pwszQuickComplete); + if (!This->quickComplete && pwszQuickComplete) + { + size_t len = strlenW(pwszQuickComplete)+1; + if ((This->quickComplete = heap_alloc(len * sizeof(WCHAR))) != NULL) + memcpy(This->quickComplete, pwszQuickComplete, len * sizeof(WCHAR)); } return S_OK; -- 1.9.1
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
+ if (pwzsRegKeyPath) + { + WCHAR *key, *value; + HKEY hKey; + LSTATUS res; + size_t len; + + /* pwszRegKeyPath contains the key as well as the value, so split it */ + value = strrchrW(pwzsRegKeyPath, '\\'); + len = value - pwzsRegKeyPath; + + if (value && (key = heap_alloc((len+1) * sizeof(*key))) != NULL) { + memcpy(key, pwzsRegKeyPath, len * sizeof(*key)); + key[len] = '\0'; + value++; + + res = RegOpenKeyExW(HKEY_CURRENT_USER, key, 0, KEY_READ, &hKey); + + /* if not found, MSDN states we must seek in HKEY_LOCAL_MACHINE */ + if (res != ERROR_SUCCESS) + res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey); + + if (res == ERROR_SUCCESS) { + WCHAR *qc; + DWORD type, sz; + res = RegQueryValueExW(hKey, value, NULL, &type, NULL, &sz); + + if (res != ERROR_SUCCESS || type != REG_SZ) { + RegCloseKey(hKey); + res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey); + if (res != ERROR_SUCCESS) + goto free_key_str; + res = RegQueryValueExW(hKey, value, NULL, &type, NULL, &sz); + } + + /* the size does include the NUL terminator, however, strings + can be stored without it, so make room for NUL just in case */ + if (res == ERROR_SUCCESS && type == REG_SZ && + (qc = heap_alloc(sz + sizeof(*qc))) != NULL) + { + DWORD old_sz = sz; + res = RegQueryValueExW(hKey, value, NULL, &type, (BYTE*)qc, &sz); + + /* make sure the value wasn't messed with */ + if (res == ERROR_SUCCESS && sz == old_sz && type == REG_SZ) { + qc[sz / sizeof(*qc)] = '\0'; + This->quickComplete = qc; + } + else + heap_free(qc); + } + RegCloseKey(hKey); + } + free_key_str: + heap_free(key); + }
This doesn't seem like an improvement. In particular, querying the key twice just to retrieve the length is making things worse, and creating more race conditions. Also note that RegQueryValueExW already adds a terminating null if necessary. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Aug 30, 2018 at 12:59 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
This doesn't seem like an improvement. In particular, querying the key twice just to retrieve the length is making things worse, and creating more race conditions. Also note that RegQueryValueExW already adds a terminating null if necessary.
-- Alexandre Julliard julliard(a)winehq.org
Yes indeed the first query gets the length, but the second one makes sure the length is exactly the same so there can't be any race condition, unless I'm missing something? About the NUL terminator: while it may be true for Wine's implementation, MSDN states that it's not necessarily the case (and tell you to use GetRegValue, which is more convoluted), and I imagine some application might hook RegQueryValueExW and provide a MSDN-compatible implementation or something (remote chance but it doesn't really hurt). Nevertheless, if you really think it's not an issue I will take that extra bit out then.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On Thu, Aug 30, 2018 at 12:59 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
This doesn't seem like an improvement. In particular, querying the key twice just to retrieve the length is making things worse, and creating more race conditions. Also note that RegQueryValueExW already adds a terminating null if necessary.
-- Alexandre Julliard julliard(a)winehq.org
Yes indeed the first query gets the length, but the second one makes sure the length is exactly the same so there can't be any race condition, unless I'm missing something?
There can still be a race, only you are making the function fail in that case, that's not nice behavior. The right way to do that sort of thing is to allocate a reasonable buffer, get the string (without querying the length first), and on overflow restart the loop with a larger buffer.
About the NUL terminator: while it may be true for Wine's implementation, MSDN states that it's not necessarily the case (and tell you to use GetRegValue, which is more convoluted), and I imagine some application might hook RegQueryValueExW and provide a MSDN-compatible implementation or something (remote chance but it doesn't really hurt). Nevertheless, if you really think it's not an issue I will take that extra bit out then.
The Wine implementation replicates the Windows behavior, based on test cases. That's more reliable than MSDN. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Aug 30, 2018 at 2:50 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
There can still be a race, only you are making the function fail in that case, that's not nice behavior. The right way to do that sort of thing is to allocate a reasonable buffer, get the string (without querying the length first), and on overflow restart the loop with a larger buffer.
Okay, you are right. I'll have it get the length first (because this simplifies the case where the value doesn't exist in HKCU but it does in HKLM), and start with that buffer size, then use a loop until it returns success (or some error other than ERROR_MORE_DATA) or its type is not REG_SZ (this should be a failure, right?).
The Wine implementation replicates the Windows behavior, based on test cases. That's more reliable than MSDN.
Yes you are correct but some hooks (like madCodeHook used in jauntePE) might hook the registry APIs to provide virtualized redirections. They *might* follow the MSDN instead of test-cases, because that's what MSDN claims, I really have no idea if they'll return a non-NUL terminated string though. Was just playing it safe. At the very least, should I just forcefully NUL terminate the buffer? (without increasing its size by +1) It would only be a single extra store and doesn't do anything on an already NUL terminated buffer.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On Thu, Aug 30, 2018 at 2:50 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
There can still be a race, only you are making the function fail in that case, that's not nice behavior. The right way to do that sort of thing is to allocate a reasonable buffer, get the string (without querying the length first), and on overflow restart the loop with a larger buffer.
Okay, you are right. I'll have it get the length first (because this simplifies the case where the value doesn't exist in HKCU but it does in HKLM), and start with that buffer size, then use a loop until it returns success (or some error other than ERROR_MORE_DATA) or its type is not REG_SZ (this should be a failure, right?).
You should never be getting the length first. Just get the data right away.
The Wine implementation replicates the Windows behavior, based on test cases. That's more reliable than MSDN.
Yes you are correct but some hooks (like madCodeHook used in jauntePE) might hook the registry APIs to provide virtualized redirections. They *might* follow the MSDN instead of test-cases, because that's what MSDN claims, I really have no idea if they'll return a non-NUL terminated string though.
If anybody is implementing API hooking based only on MSDN descriptions, they are in for a lot of trouble ;-) -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Gabriel Ivăncescu