[PATCH v3 01/17] shell32/autocomplete: Revamp pwzsRegKeyPath handling so it can deal with arbitrary sizes and make it more robust
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 read the registry value with any size. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Supersedes 150480 v2: Retrieve the registry value without failing in rare race conditions. v3: Don't open the HKEY_LOCAL_MACHINE twice under some circumstances. dlls/shell32/autocomplete.c | 90 +++++++++++++++++++++++++++++---------------- 1 file changed, 58 insertions(+), 32 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index cf8da50..051644a 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -486,40 +486,66 @@ 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; + DWORD type, sz = MAX_PATH * sizeof(WCHAR); + BOOL try_HKLM; + BYTE *qc; + 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); + try_HKLM = TRUE; + + /* 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); + try_HKLM = FALSE; + } + + if (res == ERROR_SUCCESS) { + while ((qc = heap_alloc(sz)) != NULL) { + res = RegQueryValueExW(hKey, value, NULL, &type, qc, &sz); + if (res == ERROR_SUCCESS && type == REG_SZ) { + This->quickComplete = heap_realloc(qc, sz); + break; + } + heap_free(qc); + + if (res != ERROR_MORE_DATA || type != REG_SZ) { + if (!try_HKLM) + break; + RegCloseKey(hKey); + res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey); + if (res != ERROR_SUCCESS) + goto free_key_str; + sz = MAX_PATH * sizeof(WCHAR); + try_HKLM = FALSE; + } + } + 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
On Windows, auto-append doesn't happen when the user presses Backspace or Delete, so remove it (not to mention it's totally wrong and full of bugs, see Wine-Bug). However, the autocompletion box does show up for these two keys, so handle that. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22255 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- That being said, the previous code was completely wrong even if auto append actually worked on Backspace or Delete. I honestly have no idea what it tried to accomplish... (not to mention it happens on a WM_KEYUP, which means the control already processed the operation itself) Behavior compared with Windows XP SP2. dlls/shell32/autocomplete.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 051644a..254884b 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -199,16 +199,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, ShowWindow(This->hwndListBox, SW_HIDE); return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); } - if (This->options & ACO_AUTOAPPEND) { - DWORD b; - SendMessageW(hwnd, EM_GETSEL, (WPARAM)&b, 0); - if (b>1) { - hwndText[b-1] = '\0'; - } else { - hwndText[0] = '\0'; - SetWindowTextW(hwnd, hwndText); - } - } break; default: ; @@ -236,7 +226,9 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; if (!strncmpiW(hwndText, strs, len)) { - if (!filled && (This->options & ACO_AUTOAPPEND)) { + if (!filled && (This->options & ACO_AUTOAPPEND) && + wParam != VK_BACK && wParam != VK_DELETE) + { WCHAR buffW[255]; strcpyW(buffW, hwndText); -- 1.9.1
On Wed, Sep 05, 2018 at 07:13:04PM +0300, Gabriel Ivăncescu wrote:
On Windows, auto-append doesn't happen when the user presses Backspace or Delete, so remove it (not to mention it's totally wrong and full of bugs, see Wine-Bug). However, the autocompletion box does show up for these two keys, so handle that.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22255 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
That being said, the previous code was completely wrong even if auto append actually worked on Backspace or Delete. I honestly have no idea what it tried to accomplish... (not to mention it happens on a WM_KEYUP, which means the control already processed the operation itself)
Behavior compared with Windows XP SP2.
dlls/shell32/autocomplete.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 051644a..254884b 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -199,16 +199,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, ShowWindow(This->hwndListBox, SW_HIDE); return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); } - if (This->options & ACO_AUTOAPPEND) { - DWORD b; - SendMessageW(hwnd, EM_GETSEL, (WPARAM)&b, 0); - if (b>1) { - hwndText[b-1] = '\0'; - } else { - hwndText[0] = '\0'; - SetWindowTextW(hwnd, hwndText); - } - }
Right, the original code here is clearly broken.
break; default: ; @@ -236,7 +226,9 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break;
if (!strncmpiW(hwndText, strs, len)) { - if (!filled && (This->options & ACO_AUTOAPPEND)) { + if (!filled && (This->options & ACO_AUTOAPPEND) && + wParam != VK_BACK && wParam != VK_DELETE) + {
Special casing VK_BACK and VK_DELETE like this looks odd. There has to be a cleaner way. I'm also concerned that you're about to rip this code apart in [7/17].
WCHAR buffW[255];
strcpyW(buffW, hwndText); -- 1.9.1
On Thu, Sep 6, 2018 at 1:00 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Special casing VK_BACK and VK_DELETE like this looks odd. There has to be a cleaner way. I'm also concerned that you're about to rip this code apart in [7/17].
Yes it's just a temporary band-aid but I'll just take it out, since it's solved by a later patch anyway, so this one will just fix the reported bug (but not the auto-append).
Besides what's stated in the comments, the quickComplete format can have stuff like %1234s which can make the resulting string much larger, and that's handled via the while loop. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 59 +++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 254884b..ec91474 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -109,7 +109,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW); HRESULT hr; WCHAR hwndText[255]; - WCHAR *hwndQCText; RECT r; BOOL control, filled, displayall = FALSE; int cpt, height, sel; @@ -138,11 +137,27 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, /* If quickComplete is set and control is pressed, replace the string */ control = GetKeyState(VK_CONTROL) & 0x8000; if (control && This->quickComplete) { - hwndQCText = heap_alloc((lstrlenW(This->quickComplete)+lstrlenW(hwndText))*sizeof(WCHAR)); - sel = sprintfW(hwndQCText, This->quickComplete, hwndText); - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)hwndQCText); - SendMessageW(hwnd, EM_SETSEL, 0, sel); - heap_free(hwndQCText); + WCHAR *qc; + size_t sz = strlenW(This->quickComplete) + 1; + sz += max(strlenW(hwndText), 2); /* %s is 2 chars */ + + while ((qc = heap_alloc(sz * sizeof(WCHAR)))) { + /* Pass it 3 times to guard against crap like %*.*s, where the + width and precision are taken from extra args preceeding it, + so at least it won't crash or cause other vulnerabilities */ + sel = snprintfW(qc, sz, This->quickComplete, + hwndText, hwndText, hwndText); + if (sel >= 0 && sel < sz) { + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)qc); + SendMessageW(hwnd, EM_SETSEL, 0, sel); + heap_free(qc); + break; + } + heap_free(qc); + if (sz > 0x800000U) /* too large, give up */ + break; + sz *= 2; + } } ShowWindow(This->hwndListBox, SW_HIDE); @@ -177,14 +192,14 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0); if (sel != -1) { WCHAR *msg; - int len; - - len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); - msg = heap_alloc((len + 1)*sizeof(WCHAR)); - SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg)); - heap_free(msg); + UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); + + if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) { + SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg); + SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg)); + heap_free(msg); + } } else { SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)This->txtbackup); SendMessageW(hwnd, EM_SETSEL, lstrlenW(This->txtbackup), lstrlenW(This->txtbackup)); @@ -291,7 +306,8 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { IAutoCompleteImpl *This = (IAutoCompleteImpl *)GetWindowLongPtrW(hwnd, GWLP_USERDATA); WCHAR *msg; - int sel, len; + UINT len; + int sel; switch (uMsg) { case WM_MOUSEMOVE: @@ -303,12 +319,13 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (sel < 0) break; len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); - msg = heap_alloc((len + 1)*sizeof(WCHAR)); - SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg)); - ShowWindow(hwnd, SW_HIDE); - heap_free(msg); + if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) { + SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); + SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg); + SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg)); + ShowWindow(hwnd, SW_HIDE); + heap_free(msg); + } break; default: return CallWindowProcW(This->wpOrigLBoxProc, hwnd, uMsg, wParam, lParam); -- 1.9.1
On Wed, Sep 05, 2018 at 07:13:05PM +0300, Gabriel Ivăncescu wrote:
Besides what's stated in the comments, the quickComplete format can have stuff like %1234s which can make the resulting string much larger, and that's handled via the while loop.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 59 +++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 254884b..ec91474 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -109,7 +109,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW); HRESULT hr; WCHAR hwndText[255]; - WCHAR *hwndQCText; RECT r; BOOL control, filled, displayall = FALSE; int cpt, height, sel; @@ -138,11 +137,27 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, /* If quickComplete is set and control is pressed, replace the string */ control = GetKeyState(VK_CONTROL) & 0x8000; if (control && This->quickComplete) { - hwndQCText = heap_alloc((lstrlenW(This->quickComplete)+lstrlenW(hwndText))*sizeof(WCHAR)); - sel = sprintfW(hwndQCText, This->quickComplete, hwndText); - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)hwndQCText); - SendMessageW(hwnd, EM_SETSEL, 0, sel); - heap_free(hwndQCText); + WCHAR *qc; + size_t sz = strlenW(This->quickComplete) + 1; + sz += max(strlenW(hwndText), 2); /* %s is 2 chars */ + + while ((qc = heap_alloc(sz * sizeof(WCHAR)))) { + /* Pass it 3 times to guard against crap like %*.*s, where the + width and precision are taken from extra args preceeding it, + so at least it won't crash or cause other vulnerabilities */
Do we have an app that actually passes crazy format strings? What we mainly care about is protecting against crazy user input strings, not app provided strings, so handle the alloc failure by all means, but I'm not sure the rest is really useful.
+ sel = snprintfW(qc, sz, This->quickComplete, + hwndText, hwndText, hwndText); + if (sel >= 0 && sel < sz) { + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)qc); + SendMessageW(hwnd, EM_SETSEL, 0, sel); + heap_free(qc); + break; + } + heap_free(qc); + if (sz > 0x800000U) /* too large, give up */ + break; + sz *= 2; + } }
ShowWindow(This->hwndListBox, SW_HIDE); @@ -177,14 +192,14 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0); if (sel != -1) { WCHAR *msg; - int len; - - len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); - msg = heap_alloc((len + 1)*sizeof(WCHAR)); - SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg)); - heap_free(msg); + UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); + + if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) {
You could just return on heap_alloc failure, that would keep the patch smaller.
+ SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg); + SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg)); + heap_free(msg); + } } else { SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)This->txtbackup); SendMessageW(hwnd, EM_SETSEL, lstrlenW(This->txtbackup), lstrlenW(This->txtbackup)); @@ -291,7 +306,8 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { IAutoCompleteImpl *This = (IAutoCompleteImpl *)GetWindowLongPtrW(hwnd, GWLP_USERDATA); WCHAR *msg; - int sel, len; + UINT len; + int sel;
switch (uMsg) { case WM_MOUSEMOVE: @@ -303,12 +319,13 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (sel < 0) break; len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); - msg = heap_alloc((len + 1)*sizeof(WCHAR)); - SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg)); - ShowWindow(hwnd, SW_HIDE); - heap_free(msg); + if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) {
Again, a break on alloc failure would keep things simpler.
+ SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); + SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg); + SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg)); + ShowWindow(hwnd, SW_HIDE); + heap_free(msg); + } break; default: return CallWindowProcW(This->wpOrigLBoxProc, hwnd, uMsg, wParam, lParam); -- 1.9.1
On Thu, Sep 6, 2018 at 1:24 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Do we have an app that actually passes crazy format strings? What we mainly care about is protecting against crazy user input strings, not app provided strings, so handle the alloc failure by all means, but I'm not sure the rest is really useful.
Yes it can happen because it can read it from the registry. The app can only specify a registry path, and reads whatever the format is from there. But obviously the value in the registry can be written to by any application. Which, IMO, is trivially exploitable. So the application itself doesn't have to be malicious or crazy for this to happen; any other application messing up that registry value can do it. Also note that it's mostly about passing it 3 times (hwndText), the rest is already required even for valid strings like %12s which can be larger than the string itself (or other such format complications, I don't really want to make a printf parser...). I don't think it's a big deal to just pass it 3 times to be safe...
You could just return on heap_alloc failure, that would keep the patch smaller.
Again, a break on alloc failure would keep things simpler.
Okay, will do.
On Thu, Sep 06, 2018 at 04:34:11PM +0300, Gabriel Ivăncescu wrote:
On Thu, Sep 6, 2018 at 1:24 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Do we have an app that actually passes crazy format strings? What we mainly care about is protecting against crazy user input strings, not app provided strings, so handle the alloc failure by all means, but I'm not sure the rest is really useful.
Yes it can happen because it can read it from the registry. The app can only specify a registry path, and reads whatever the format is from there. But obviously the value in the registry can be written to by any application. Which, IMO, is trivially exploitable. So the application itself doesn't have to be malicious or crazy for this to happen; any other application messing up that registry value can do it.
Also note that it's mostly about passing it 3 times (hwndText), the rest is already required even for valid strings like %12s which can be larger than the string itself (or other such format complications, I don't really want to make a printf parser...). I don't think it's a big deal to just pass it 3 times to be safe...
What does Windows do if it's passed %12s for example? Huw.
On Thu, Sep 6, 2018 at 5:51 PM, Huw Davies <huw(a)codeweavers.com> wrote:
What does Windows do if it's passed %12s for example?
Huw.
On Windows XP it works fine (Internet Explorer uses it) and shows what you'd expect from %12s. It's mostly for user interaction anyway, so I think Microsoft have some leeway in changing it in each version. That being said, I honestly don't think that copying security vulnerabilities from Windows is a good idea, even if it's technically "correct". It's not a good idea to crash on such invalid input in my opinion, even if an (unpatched?) Windows version does, especially since said input is external to the application.
On 6 Sep 2018, at 16:05, Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
On Thu, Sep 6, 2018 at 5:51 PM, Huw Davies <huw(a)codeweavers.com> wrote:
What does Windows do if it's passed %12s for example?
Huw.
On Windows XP it works fine (Internet Explorer uses it) and shows what you'd expect from %12s. It's mostly for user interaction anyway, so I think Microsoft have some leeway in changing it in each version.
That being said, I honestly don't think that copying security vulnerabilities from Windows is a good idea, even if it's technically "correct". It's not a good idea to crash on such invalid input in my opinion, even if an (unpatched?) Windows version does, especially since said input is external to the application.
I suggest we do the sprintf ourselves. All we'd need to do is replace the first occurrence of '%s' with the appropriate string. We can ignore width/precision specifiers for now unless we find that an app actually depends on them. Huw.
On Thu, Sep 6, 2018 at 6:28 PM, Huw Davies <huw.davies(a)physics.ox.ac.uk> wrote:
I suggest we do the sprintf ourselves. All we'd need to do is replace the first occurrence of '%s' with the appropriate string. We can ignore width/precision specifiers for now unless we find that an app actually depends on them.
You mean you want the code itself to replace the %s with the string without using sprintf? But then %12s wouldn't work, though I don't think it's really useful... but if you're really fine with that I'll go and do it. This way we won't need patch 4/17 either (which guards against multiple such args, e.g. two %s would use some string off the stack which can lead to crash or vulnerability) since the rest will simply be displayed as %s.
On 6 Sep 2018, at 16:43, Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
On Thu, Sep 6, 2018 at 6:28 PM, Huw Davies <huw.davies(a)physics.ox.ac.uk> wrote:
I suggest we do the sprintf ourselves. All we'd need to do is replace the first occurrence of '%s' with the appropriate string. We can ignore width/precision specifiers for now unless we find that an app actually depends on them.
You mean you want the code itself to replace the %s with the string without using sprintf? But then %12s wouldn't work, though I don't think it's really useful... but if you're really fine with that I'll go and do it.
Yes, as I said, we can always add '%12s' support if we find something that needs it. Huw.
This is especially important since it can be read from the registry, so it's trivial to abuse this from other applications if one application makes use of it. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index ec91474..3d3ec57 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -557,6 +557,27 @@ static HRESULT WINAPI IAutoComplete2_fnInit( memcpy(This->quickComplete, pwszQuickComplete, len * sizeof(WCHAR)); } + /* Guard against more than one format arguments since that leads to either a crash + or leaking stack data out, especially since it can be read from the registry */ + if (This->quickComplete) { + WCHAR *qc = This->quickComplete; + BOOL found = FALSE; + while ((qc = strchrW(qc, '%')) != NULL) + { + if (qc[1] == '%') /* %% is not an arg */ + qc++; + else { + if (found) { + heap_free(This->quickComplete); + This->quickComplete = NULL; + break; + } + found = TRUE; + } + qc++; + } + } + return S_OK; } -- 1.9.1
We can retrieve the length of the string from the SendMessage calls already. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 3d3ec57..e5712e6 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -195,14 +195,16 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) { - SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); + len = SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg)); + SendMessageW(hwnd, EM_SETSEL, len, len); heap_free(msg); } } else { + UINT len; SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)This->txtbackup); - SendMessageW(hwnd, EM_SETSEL, lstrlenW(This->txtbackup), lstrlenW(This->txtbackup)); + len = strlenW(This->txtbackup); + SendMessageW(hwnd, EM_SETSEL, len, len); } } return 0; @@ -320,9 +322,9 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) { - SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); + len = SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg)); + SendMessageW(This->hwndEdit, EM_SETSEL, 0, len); ShowWindow(hwnd, SW_HIDE); heap_free(msg); } -- 1.9.1
There's no need to send a WM_KEYUP anymore since it will now match Windows behavior. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/tests/autocomplete.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c index 3c8de88..b9c6374 100644 --- a/dlls/shell32/tests/autocomplete.c +++ b/dlls/shell32/tests/autocomplete.c @@ -386,8 +386,7 @@ static void test_custom_source(void) ok(hr == S_OK, "IAutoComplete_Init failed: %x\n", hr); SendMessageW(hwnd_edit, WM_CHAR, 'a', 1); - /* Send a keyup message since wine doesn't handle WM_CHAR yet */ - SendMessageW(hwnd_edit, WM_KEYUP, 'u', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'u', 1); Sleep(100); while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) { -- 1.9.1
On Thu, Sep 6, 2018 at 12:52 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Doesn't this need to go after 7/17 then?
Huw.
Sorry, I was just following the guidelines at https://wiki.winehq.org/Submitting_Patches Which says to send the tests as a separate patch before the fix, unless I misunderstood it.
On Thu, Sep 06, 2018 at 04:34:42PM +0300, Gabriel Ivăncescu wrote:
On Thu, Sep 6, 2018 at 12:52 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Doesn't this need to go after 7/17 then?
Huw.
Sorry, I was just following the guidelines at https://wiki.winehq.org/Submitting_Patches
Which says to send the tests as a separate patch before the fix, unless I misunderstood it.
The tests need to succeed after each patch, but they will fail after this patch is applied (and then succeed after the next patch).
On Thu, Sep 6, 2018 at 5:11 PM, Huw Davies <huw(a)codeweavers.com> wrote:
The tests need to succeed after each patch, but they will fail after this patch is applied (and then succeed after the next patch).
I see, that was my original thought as well but I imagined I did something wrong with my last patch in respect to this (I sent the test after the fix). The wiki is definitely confusing in this aspect, that should probably be changed.
AutoComplete currently shows up when the user releases a key, which is wrong. Windows does it when the user presses a key, so use both WM_KEYDOWN and WM_CHAR and redesign it so that it matches Windows behavior. At the same time, dynamically allocate the internal text buffer itself so we can handle any length, while avoiding the buffer overflow. The previous code caps the text at 255 characters (and in such case gives the wrong results), and is prone to exploits, as it assumes the enumerated string fits in 255 characters as well, which can be easily exploited. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This patch does many changes because they are either minor or intertwined and hard to separate without breaking the compilation or Wine. A difficulty here stems from the fact that we need to handle not just WM_KEYDOWN but also WM_CHAR, which is harder than WM_KEYUP especially when trying to share code paths because control characters have to be handled separately (for auto-append purposes). For example VK_BACK is not processed in WM_KEYDOWN because it's done during WM_CHAR as the escape '\b' along the rest. That's not the case with VK_DELETE which has no WM_CHAR escape. This is important since the edit control *must* receive the message before us, otherwise the behavior will be wrong, so they have to be deferred to WM_CHAR and we can't do it in WM_KEYDOWN. That's also why we always make sure to pass the corresponding message to the edit control first. dlls/shell32/autocomplete.c | 169 ++++++++++++++++++++++++++------------------ 1 file changed, 101 insertions(+), 68 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index e5712e6..74728e0 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -108,17 +108,19 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW); HRESULT hr; - WCHAR hwndText[255]; - RECT r; - BOOL control, filled, displayall = FALSE; - int cpt, height, sel; + LRESULT ret; + WCHAR *hwndText; + UINT len, old_len, cpt; + BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND); + INT sel; if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); switch (uMsg) { case CB_SHOWDROPDOWN: - ShowWindow(This->hwndListBox, SW_HIDE); + if (This->options & ACO_AUTOSUGGEST) + ShowWindow(This->hwndListBox, SW_HIDE); break; case WM_KILLFOCUS: if ((This->options & ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) @@ -126,21 +128,19 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, ShowWindow(This->hwndListBox, SW_HIDE); } return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - case WM_KEYUP: - { - int len; - - GetWindowTextW(hwnd, hwndText, ARRAY_SIZE(hwndText)); - + case WM_KEYDOWN: switch(wParam) { case VK_RETURN: /* If quickComplete is set and control is pressed, replace the string */ - control = GetKeyState(VK_CONTROL) & 0x8000; - if (control && This->quickComplete) { + if (This->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000)) { WCHAR *qc; - size_t sz = strlenW(This->quickComplete) + 1; - sz += max(strlenW(hwndText), 2); /* %s is 2 chars */ + size_t sz; + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0) + 1; /* include NUL */ + if (!(hwndText = heap_alloc(len * sizeof(WCHAR)))) + return 0; + len = SendMessageW(hwnd, WM_GETTEXT, len, (LPARAM)hwndText); + sz = strlenW(This->quickComplete)+1 + max(len, 2); /* %s is 2 chars */ while ((qc = heap_alloc(sz * sizeof(WCHAR)))) { /* Pass it 3 times to guard against crap like %*.*s, where the width and precision are taken from extra args preceeding it, @@ -158,31 +158,35 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; sz *= 2; } + if (This->options & ACO_AUTOSUGGEST) + ShowWindow(This->hwndListBox, SW_HIDE); + heap_free(hwndText); + return 0; } - ShowWindow(This->hwndListBox, SW_HIDE); - return 0; - case VK_LEFT: - case VK_RIGHT: - return 0; + if (This->options & ACO_AUTOSUGGEST) + ShowWindow(This->hwndListBox, SW_HIDE); + break; case VK_UP: case VK_DOWN: - /* Two cases here : - - if the listbox is not visible, displays it - with all the entries if the style ACO_UPDOWNKEYDROPSLIST - is present but does not select anything. + /* Two cases here: + - if the listbox is not visible and ACO_UPDOWNKEYDROPSLIST is + set, display it with all the entries, without selecting any - if the listbox is visible, change the selection */ - if ( (This->options & (ACO_AUTOSUGGEST | ACO_UPDOWNKEYDROPSLIST)) - && (!IsWindowVisible(This->hwndListBox) && (! *hwndText)) ) - { - /* We must display all the entries */ - displayall = TRUE; - } else { - if (IsWindowVisible(This->hwndListBox)) { - int count; + if (This->options & ACO_AUTOSUGGEST) { + if (!IsWindowVisible(This->hwndListBox)) { + if (This->options & ACO_UPDOWNKEYDROPSLIST) { + ret = 0; + displayall = TRUE; + noautoappend = TRUE; + goto autocomplete_text; + } + } else { + INT count; count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0); + /* Change the selection */ sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0); if (wParam == VK_UP) @@ -206,34 +210,51 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, len = strlenW(This->txtbackup); SendMessageW(hwnd, EM_SETSEL, len, len); } + return 0; } - return 0; } break; - case VK_BACK: case VK_DELETE: - if ((! *hwndText) && (This->options & ACO_AUTOSUGGEST)) { - ShowWindow(This->hwndListBox, SW_HIDE); - return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - } - break; - default: - ; + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + goto handle_control_char; + } + return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + case WM_CHAR: + case WM_UNICHAR: + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + if (wParam < ' ') + { + handle_control_char: + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + if ((This->options & ACO_AUTOSUGGEST) && len == 0) { + ShowWindow(This->hwndListBox, SW_HIDE); + return ret; + } + if (wParam != 0x16 /* ^V (paste) */) + noautoappend = TRUE; } + else { + autocomplete_text: + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + } + + old_len = len+1; /* include NUL */ + if (!(hwndText = heap_alloc(old_len * sizeof(WCHAR)))) + return ret; + len = SendMessageW(hwnd, WM_GETTEXT, old_len, (LPARAM)hwndText); + if (len+1 != old_len) + hwndText = heap_realloc(hwndText, len+1); SendMessageW(This->hwndListBox, LB_RESETCONTENT, 0, 0); + /* Set txtbackup to point to hwndText itself (which must not be released) */ heap_free(This->txtbackup); - len = strlenW(hwndText); - This->txtbackup = heap_alloc((len + 1)*sizeof(WCHAR)); - lstrcpyW(This->txtbackup, hwndText); + This->txtbackup = hwndText; - /* Returns if there is no text to search and we doesn't want to display all the entries */ - if ((!displayall) && (! *hwndText) ) - break; + if (!displayall && !len) + return ret; IEnumString_Reset(This->enumstr); - filled = FALSE; for(cpt = 0;;) { LPOLESTR strs = NULL; ULONG fetched; @@ -243,62 +264,68 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; if (!strncmpiW(hwndText, strs, len)) { - if (!filled && (This->options & ACO_AUTOAPPEND) && - wParam != VK_BACK && wParam != VK_DELETE) + if (cpt == 0 && noautoappend == FALSE) { - WCHAR buffW[255]; + /* The character capitalization can be different, + so merge hwndText and strs into a new string */ + WCHAR *tmp; + size_t strslen = len + strlenW(&strs[len]); + + if ((tmp = heap_alloc((strslen+1) * sizeof(WCHAR)))) + { + memcpy(tmp, hwndText, len * sizeof(WCHAR)); + memcpy(&tmp[len], &strs[len], (strslen-len+1) * sizeof(WCHAR)); + } + else tmp = strs; + + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)tmp); + SendMessageW(hwnd, EM_SETSEL, len, strslen); + if (tmp != strs) + heap_free(tmp); - strcpyW(buffW, hwndText); - strcatW(buffW, &strs[len]); - SetWindowTextW(hwnd, buffW); - SendMessageW(hwnd, EM_SETSEL, len, strlenW(strs)); if (!(This->options & ACO_AUTOSUGGEST)) { CoTaskMemFree(strs); break; } } - if (This->options & ACO_AUTOSUGGEST) { - SendMessageW(This->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs); - cpt++; - } + if (This->options & ACO_AUTOSUGGEST) + SendMessageW(This->hwndListBox, LB_INSERTSTRING, -1, (LPARAM)strs); - filled = TRUE; + cpt++; } CoTaskMemFree(strs); } if (This->options & ACO_AUTOSUGGEST) { - if (filled) { - height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0); + if (cpt) { + RECT r; + UINT height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0); SendMessageW(This->hwndListBox, LB_CARETOFF, 0, 0); GetWindowRect(hwnd, &r); SetParent(This->hwndListBox, HWND_DESKTOP); /* It seems that Windows XP displays 7 lines at most and otherwise displays a vertical scroll bar */ SetWindowPos(This->hwndListBox, HWND_TOP, - r.left, r.bottom + 1, r.right - r.left, min(height * 7, height*(cpt+1)), + r.left, r.bottom + 1, r.right - r.left, height*min(cpt+1, 7), SWP_SHOWWINDOW ); } else { ShowWindow(This->hwndListBox, SW_HIDE); } } - - break; - } + return ret; case WM_DESTROY: { WNDPROC proc = This->wpOrigEditProc; - RemovePropW(hwnd, autocomplete_propertyW); SetWindowLongPtrW(hwnd, GWLP_WNDPROC, (LONG_PTR)proc); + RemovePropW(hwnd, autocomplete_propertyW); destroy_autocomplete_object(This); return CallWindowProcW(proc, hwnd, uMsg, wParam, lParam); } default: return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - } return 0; @@ -351,6 +378,8 @@ static void create_listbox(IAutoCompleteImpl *This) This->wpOrigLBoxProc = (WNDPROC) SetWindowLongPtrW( This->hwndListBox, GWLP_WNDPROC, (LONG_PTR) ACLBoxSubclassProc); SetWindowLongPtrW( This->hwndListBox, GWLP_USERDATA, (LONG_PTR)This); } + else + This->options &= ~ACO_AUTOSUGGEST; } /************************************************************************** @@ -470,6 +499,10 @@ static HRESULT WINAPI IAutoComplete2_fnInit( return This->hwndEdit ? E_FAIL : E_UNEXPECTED; } + /* Prevent txtbackup from ever being NULL to simplify the code */ + if ((This->txtbackup = heap_alloc_zero(sizeof(WCHAR))) == NULL) + return E_OUTOFMEMORY; + if (FAILED (IUnknown_QueryInterface (punkACL, &IID_IEnumString, (LPVOID*)&This->enumstr))) { WARN("No IEnumString interface\n"); return E_NOINTERFACE; -- 1.9.1
On Wed, Sep 05, 2018 at 07:13:09PM +0300, Gabriel Ivăncescu wrote:
AutoComplete currently shows up when the user releases a key, which is wrong. Windows does it when the user presses a key, so use both WM_KEYDOWN and WM_CHAR and redesign it so that it matches Windows behavior.
At the same time, dynamically allocate the internal text buffer itself so we can handle any length, while avoiding the buffer overflow. The previous code caps the text at 255 characters (and in such case gives the wrong results), and is prone to exploits, as it assumes the enumerated string fits in 255 characters as well, which can be easily exploited.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
This patch does many changes because they are either minor or intertwined and hard to separate without breaking the compilation or Wine.
Well the patch is too big, so start with moving the minor stuff to a separate patch and let's see where we're at. [Hint: any patch that has "At the same time" in its commit message should probably be split].
A difficulty here stems from the fact that we need to handle not just WM_KEYDOWN but also WM_CHAR, which is harder than WM_KEYUP especially when trying to share code paths because control characters have to be handled separately (for auto-append purposes).
For example VK_BACK is not processed in WM_KEYDOWN because it's done during WM_CHAR as the escape '\b' along the rest. That's not the case with VK_DELETE which has no WM_CHAR escape. This is important since the edit control *must* receive the message before us, otherwise the behavior will be wrong, so they have to be deferred to WM_CHAR and we can't do it in WM_KEYDOWN. That's also why we always make sure to pass the corresponding message to the edit control first.
dlls/shell32/autocomplete.c | 169 ++++++++++++++++++++++++++------------------ 1 file changed, 101 insertions(+), 68 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index e5712e6..74728e0 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -108,17 +108,19 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW); HRESULT hr; - WCHAR hwndText[255]; - RECT r; - BOOL control, filled, displayall = FALSE; - int cpt, height, sel; + LRESULT ret; + WCHAR *hwndText; + UINT len, old_len, cpt; + BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND); + INT sel;
if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
switch (uMsg) { case CB_SHOWDROPDOWN: - ShowWindow(This->hwndListBox, SW_HIDE); + if (This->options & ACO_AUTOSUGGEST) + ShowWindow(This->hwndListBox, SW_HIDE); break; case WM_KILLFOCUS: if ((This->options & ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) @@ -126,21 +128,19 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, ShowWindow(This->hwndListBox, SW_HIDE); } return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - case WM_KEYUP: - { - int len; - - GetWindowTextW(hwnd, hwndText, ARRAY_SIZE(hwndText)); - + case WM_KEYDOWN: switch(wParam) { case VK_RETURN: /* If quickComplete is set and control is pressed, replace the string */ - control = GetKeyState(VK_CONTROL) & 0x8000; - if (control && This->quickComplete) { + if (This->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000)) { WCHAR *qc; - size_t sz = strlenW(This->quickComplete) + 1; - sz += max(strlenW(hwndText), 2); /* %s is 2 chars */ + size_t sz; + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0) + 1; /* include NUL */ + if (!(hwndText = heap_alloc(len * sizeof(WCHAR)))) + return 0; + len = SendMessageW(hwnd, WM_GETTEXT, len, (LPARAM)hwndText);
+ sz = strlenW(This->quickComplete)+1 + max(len, 2); /* %s is 2 chars */ while ((qc = heap_alloc(sz * sizeof(WCHAR)))) { /* Pass it 3 times to guard against crap like %*.*s, where the width and precision are taken from extra args preceeding it, @@ -158,31 +158,35 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; sz *= 2; } + if (This->options & ACO_AUTOSUGGEST) + ShowWindow(This->hwndListBox, SW_HIDE); + heap_free(hwndText); + return 0; }
- ShowWindow(This->hwndListBox, SW_HIDE); - return 0; - case VK_LEFT: - case VK_RIGHT: - return 0; + if (This->options & ACO_AUTOSUGGEST) + ShowWindow(This->hwndListBox, SW_HIDE); + break; case VK_UP: case VK_DOWN: - /* Two cases here : - - if the listbox is not visible, displays it - with all the entries if the style ACO_UPDOWNKEYDROPSLIST - is present but does not select anything. + /* Two cases here: + - if the listbox is not visible and ACO_UPDOWNKEYDROPSLIST is + set, display it with all the entries, without selecting any - if the listbox is visible, change the selection */ - if ( (This->options & (ACO_AUTOSUGGEST | ACO_UPDOWNKEYDROPSLIST)) - && (!IsWindowVisible(This->hwndListBox) && (! *hwndText)) ) - { - /* We must display all the entries */ - displayall = TRUE; - } else { - if (IsWindowVisible(This->hwndListBox)) { - int count; + if (This->options & ACO_AUTOSUGGEST) { + if (!IsWindowVisible(This->hwndListBox)) { + if (This->options & ACO_UPDOWNKEYDROPSLIST) { + ret = 0; + displayall = TRUE; + noautoappend = TRUE; + goto autocomplete_text; + } + } else { + INT count;
count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0); + /* Change the selection */ sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0); if (wParam == VK_UP) @@ -206,34 +210,51 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, len = strlenW(This->txtbackup); SendMessageW(hwnd, EM_SETSEL, len, len); } + return 0; } - return 0; } break; - case VK_BACK: case VK_DELETE: - if ((! *hwndText) && (This->options & ACO_AUTOSUGGEST)) { - ShowWindow(This->hwndListBox, SW_HIDE); - return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - } - break; - default: - ; + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + goto handle_control_char; + } + return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + case WM_CHAR: + case WM_UNICHAR: + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + if (wParam < ' ') + { + handle_control_char: + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + if ((This->options & ACO_AUTOSUGGEST) && len == 0) { + ShowWindow(This->hwndListBox, SW_HIDE); + return ret; + } + if (wParam != 0x16 /* ^V (paste) */) + noautoappend = TRUE; } + else { + autocomplete_text: + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + } + + old_len = len+1; /* include NUL */ + if (!(hwndText = heap_alloc(old_len * sizeof(WCHAR)))) + return ret; + len = SendMessageW(hwnd, WM_GETTEXT, old_len, (LPARAM)hwndText); + if (len+1 != old_len) + hwndText = heap_realloc(hwndText, len+1);
SendMessageW(This->hwndListBox, LB_RESETCONTENT, 0, 0);
+ /* Set txtbackup to point to hwndText itself (which must not be released) */ heap_free(This->txtbackup); - len = strlenW(hwndText); - This->txtbackup = heap_alloc((len + 1)*sizeof(WCHAR)); - lstrcpyW(This->txtbackup, hwndText); + This->txtbackup = hwndText;
- /* Returns if there is no text to search and we doesn't want to display all the entries */ - if ((!displayall) && (! *hwndText) ) - break; + if (!displayall && !len) + return ret;
IEnumString_Reset(This->enumstr); - filled = FALSE; for(cpt = 0;;) { LPOLESTR strs = NULL; ULONG fetched; @@ -243,62 +264,68 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break;
if (!strncmpiW(hwndText, strs, len)) { - if (!filled && (This->options & ACO_AUTOAPPEND) && - wParam != VK_BACK && wParam != VK_DELETE) + if (cpt == 0 && noautoappend == FALSE) { - WCHAR buffW[255]; + /* The character capitalization can be different, + so merge hwndText and strs into a new string */ + WCHAR *tmp; + size_t strslen = len + strlenW(&strs[len]); + + if ((tmp = heap_alloc((strslen+1) * sizeof(WCHAR)))) + { + memcpy(tmp, hwndText, len * sizeof(WCHAR)); + memcpy(&tmp[len], &strs[len], (strslen-len+1) * sizeof(WCHAR)); + } + else tmp = strs; + + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)tmp); + SendMessageW(hwnd, EM_SETSEL, len, strslen); + if (tmp != strs) + heap_free(tmp);
- strcpyW(buffW, hwndText); - strcatW(buffW, &strs[len]); - SetWindowTextW(hwnd, buffW); - SendMessageW(hwnd, EM_SETSEL, len, strlenW(strs)); if (!(This->options & ACO_AUTOSUGGEST)) { CoTaskMemFree(strs); break; } }
- if (This->options & ACO_AUTOSUGGEST) { - SendMessageW(This->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs); - cpt++; - } + if (This->options & ACO_AUTOSUGGEST) + SendMessageW(This->hwndListBox, LB_INSERTSTRING, -1, (LPARAM)strs);
- filled = TRUE; + cpt++; }
CoTaskMemFree(strs); }
if (This->options & ACO_AUTOSUGGEST) { - if (filled) { - height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0); + if (cpt) { + RECT r; + UINT height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0); SendMessageW(This->hwndListBox, LB_CARETOFF, 0, 0); GetWindowRect(hwnd, &r); SetParent(This->hwndListBox, HWND_DESKTOP); /* It seems that Windows XP displays 7 lines at most and otherwise displays a vertical scroll bar */ SetWindowPos(This->hwndListBox, HWND_TOP, - r.left, r.bottom + 1, r.right - r.left, min(height * 7, height*(cpt+1)), + r.left, r.bottom + 1, r.right - r.left, height*min(cpt+1, 7), SWP_SHOWWINDOW ); } else { ShowWindow(This->hwndListBox, SW_HIDE); } } - - break; - } + return ret; case WM_DESTROY: { WNDPROC proc = This->wpOrigEditProc;
- RemovePropW(hwnd, autocomplete_propertyW); SetWindowLongPtrW(hwnd, GWLP_WNDPROC, (LONG_PTR)proc); + RemovePropW(hwnd, autocomplete_propertyW); destroy_autocomplete_object(This); return CallWindowProcW(proc, hwnd, uMsg, wParam, lParam); } default: return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - }
return 0; @@ -351,6 +378,8 @@ static void create_listbox(IAutoCompleteImpl *This) This->wpOrigLBoxProc = (WNDPROC) SetWindowLongPtrW( This->hwndListBox, GWLP_WNDPROC, (LONG_PTR) ACLBoxSubclassProc); SetWindowLongPtrW( This->hwndListBox, GWLP_USERDATA, (LONG_PTR)This); } + else + This->options &= ~ACO_AUTOSUGGEST; }
/************************************************************************** @@ -470,6 +499,10 @@ static HRESULT WINAPI IAutoComplete2_fnInit( return This->hwndEdit ? E_FAIL : E_UNEXPECTED; }
+ /* Prevent txtbackup from ever being NULL to simplify the code */ + if ((This->txtbackup = heap_alloc_zero(sizeof(WCHAR))) == NULL) + return E_OUTOFMEMORY; + if (FAILED (IUnknown_QueryInterface (punkACL, &IID_IEnumString, (LPVOID*)&This->enumstr))) { WARN("No IEnumString interface\n"); return E_NOINTERFACE; -- 1.9.1
On Thu, Sep 6, 2018 at 12:55 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Well the patch is too big, so start with moving the minor stuff to a separate patch and let's see where we're at. [Hint: any patch that has "At the same time" in its commit message should probably be split].
I was worried about that fact, but I'll end up rewriting some parts of the minor patch with the second one since most of it is intertwined. I hope that's ok. I'll try to split it up somehow. That said I'll send the revised patches once I know what to do with 1/17 (and maybe 3/17 if you really want to take the 3 args out) since it's quite a large patchset and don't want to pollute the mailing list too much.
On Thu, Sep 06, 2018 at 04:35:28PM +0300, Gabriel Ivăncescu wrote:
On Thu, Sep 6, 2018 at 12:55 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Well the patch is too big, so start with moving the minor stuff to a separate patch and let's see where we're at. [Hint: any patch that has "At the same time" in its commit message should probably be split].
I was worried about that fact, but I'll end up rewriting some parts of the minor patch with the second one since most of it is intertwined. I hope that's ok. I'll try to split it up somehow.
That said I'll send the revised patches once I know what to do with 1/17 (and maybe 3/17 if you really want to take the 3 args out) since it's quite a large patchset and don't want to pollute the mailing list too much.
Limiting the patches to groups of around 7 or so would be good. We'll get the first lot in, then we can work on the next batch - I'm not going to review all 17 patches in one go. Huw.
On Thu, Sep 6, 2018 at 5:13 PM, Huw Davies <huw(a)codeweavers.com> wrote:
On Thu, Sep 06, 2018 at 04:35:28PM +0300, Gabriel Ivăncescu wrote:
Limiting the patches to groups of around 7 or so would be good. We'll get the first lot in, then we can work on the next batch - I'm not going to review all 17 patches in one go.
Huw.
Sounds reasonable, I'll stop after the "big" patch then (well, once I split it up) for this batch then.
When going up past the topmost item in the listbox, go through txtbackup first before wrapping around, just like when going down. This matches Windows behavior. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 74728e0..4305f65 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -183,14 +183,12 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, goto autocomplete_text; } } else { - INT count; - - count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0); + INT count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0); /* Change the selection */ sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0); if (wParam == VK_UP) - sel = ((sel-1) < 0) ? count-1 : sel-1; + sel = ((sel-1) < -1) ? count-1 : sel-1; else sel = ((sel+1) >= count) ? -1 : sel+1; SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0); -- 1.9.1
Windows' AutoComplete seems to bypass hijacking in some cases. However, WM_GETTEXT seems to be able to get hijacked. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/tests/autocomplete.c | 70 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c index b9c6374..403b7d5 100644 --- a/dlls/shell32/tests/autocomplete.c +++ b/dlls/shell32/tests/autocomplete.c @@ -236,6 +236,36 @@ static void createMainWnd(void) CW_USEDEFAULT, CW_USEDEFAULT, 130, 105, NULL, NULL, GetModuleHandleA(NULL), 0); } +static WNDPROC HijackerWndProc_prev; +static const WCHAR HijackerWndProc_txt[] = {'H','i','j','a','c','k','e','d',0}; +static LRESULT CALLBACK HijackerWndProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + switch(msg) { + case WM_GETTEXT: + { + size_t len = min(wParam, ARRAY_SIZE(HijackerWndProc_txt)); + memcpy((void*)lParam, HijackerWndProc_txt, len * sizeof(WCHAR)); + return len; + } + case WM_GETTEXTLENGTH: + return ARRAY_SIZE(HijackerWndProc_txt)-1; + } + return CallWindowProcW(HijackerWndProc_prev, hWnd, msg, wParam, lParam); +} + +static LRESULT CALLBACK HijackerWndProc2(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + switch(msg) { + case EM_SETSEL: + lParam = wParam; + break; + case WM_SETTEXT: + lParam = (LPARAM)HijackerWndProc_txt; + break; + } + return CallWindowProcW(HijackerWndProc_prev, hWnd, msg, wParam, lParam); +} + struct string_enumerator { IEnumString IEnumString_iface; @@ -363,6 +393,7 @@ static void test_custom_source(void) static WCHAR str_alpha[] = {'t','e','s','t','1',0}; static WCHAR str_alpha2[] = {'t','e','s','t','2',0}; static WCHAR str_beta[] = {'a','u','t','o',' ','c','o','m','p','l','e','t','e',0}; + static WCHAR str_au[] = {'a','u',0}; static WCHAR *suggestions[] = { str_alpha, str_alpha2, str_beta }; IUnknown *enumerator; IAutoComplete2 *autocomplete; @@ -395,6 +426,45 @@ static void test_custom_source(void) } SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); ok(lstrcmpW(str_beta, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_beta), wine_dbgstr_w(buffer)); + SendMessageW(hwnd_edit, EM_SETSEL, 0, -1); + SendMessageW(hwnd_edit, WM_CHAR, '\b', 1); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(buffer[0] == '\0', "Expected empty string, got %s\n", wine_dbgstr_w(buffer)); + + /* hijack the window procedure */ + HijackerWndProc_prev = (WNDPROC)SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(lstrcmpW(HijackerWndProc_txt, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(HijackerWndProc_txt), wine_dbgstr_w(buffer)); + + SendMessageW(hwnd_edit, WM_CHAR, 'a', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'u', 1); + Sleep(100); + SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc_prev); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(lstrcmpW(str_au, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_au), wine_dbgstr_w(buffer)); + SendMessageW(hwnd_edit, EM_SETSEL, 0, -1); + SendMessageW(hwnd_edit, WM_CHAR, '\b', 1); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(buffer[0] == '\0', "Expected empty string, got %s\n", wine_dbgstr_w(buffer)); + + HijackerWndProc_prev = (WNDPROC)SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc2); + SendMessageW(hwnd_edit, WM_CHAR, 'a', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'u', 1); + Sleep(100); + SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc_prev); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(lstrcmpW(str_beta, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_beta), wine_dbgstr_w(buffer)); + /* end of hijacks */ ShowWindow(hMainWnd, SW_HIDE); DestroyWindow(hwnd_edit); -- 1.9.1
Send the messages directly to the edit control's window procedure (and likewise for the AutoComplete listbox) to reduce needless overhead and match Windows behavior. However, it appears that Windows allows hijacking WM_GETTEXT and WM_GETTEXTLENGTH for autocompletion, so handle that case separately. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 88 +++++++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 35 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 4305f65..b71cbc0 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -93,6 +93,21 @@ static inline IAutoCompleteImpl *impl_from_IAutoCompleteDropDown(IAutoCompleteDr return CONTAINING_RECORD(iface, IAutoCompleteImpl, IAutoCompleteDropDown_iface); } +static inline LRESULT send_to_LB(IAutoCompleteImpl *This, HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + return CallWindowProcW(This->wpOrigLBoxProc, hwnd, msg, wParam, lParam); +} + +static void set_text_and_selection(IAutoCompleteImpl *This, HWND hwnd, WCHAR *text, WPARAM start, LPARAM end) +{ + /* Send it directly to the edit control to match Windows XP behavior */ + WNDPROC proc = This->wpOrigEditProc; + + /* FIXME: send WM_SETREDRAW when the edit control supports it */ + if (CallWindowProcW(proc, hwnd, WM_SETTEXT, 0, (LPARAM)text)) + CallWindowProcW(proc, hwnd, EM_SETSEL, start, end); +} + static void destroy_autocomplete_object(IAutoCompleteImpl *ac) { ac->hwndEdit = NULL; @@ -110,6 +125,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, HRESULT hr; LRESULT ret; WCHAR *hwndText; + WNDPROC edit_proc; UINT len, old_len, cpt; BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND); INT sel; @@ -135,10 +151,14 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (This->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000)) { WCHAR *qc; size_t sz; - len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0) + 1; /* include NUL */ + edit_proc = (WNDPROC)GetWindowLongPtrW(hwnd, GWLP_WNDPROC); + edit_proc = edit_proc == ACEditSubclassProc ? This->wpOrigEditProc : edit_proc; + + len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXTLENGTH, 0, 0); + len++; /* include NUL */ if (!(hwndText = heap_alloc(len * sizeof(WCHAR)))) return 0; - len = SendMessageW(hwnd, WM_GETTEXT, len, (LPARAM)hwndText); + len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXT, len, (LPARAM)hwndText); sz = strlenW(This->quickComplete)+1 + max(len, 2); /* %s is 2 chars */ while ((qc = heap_alloc(sz * sizeof(WCHAR)))) { @@ -148,8 +168,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, sel = snprintfW(qc, sz, This->quickComplete, hwndText, hwndText, hwndText); if (sel >= 0 && sel < sz) { - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)qc); - SendMessageW(hwnd, EM_SETSEL, 0, sel); + set_text_and_selection(This, hwnd, qc, 0, sel); heap_free(qc); break; } @@ -183,30 +202,27 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, goto autocomplete_text; } } else { - INT count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0); + INT count = send_to_LB(This, This->hwndListBox, LB_GETCOUNT, 0, 0); /* Change the selection */ - sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0); + sel = send_to_LB(This, This->hwndListBox, LB_GETCURSEL, 0, 0); if (wParam == VK_UP) sel = ((sel-1) < -1) ? count-1 : sel-1; else sel = ((sel+1) >= count) ? -1 : sel+1; - SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0); + send_to_LB(This, This->hwndListBox, LB_SETCURSEL, sel, 0); if (sel != -1) { WCHAR *msg; - UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); + UINT len = send_to_LB(This, This->hwndListBox, LB_GETTEXTLEN, sel, 0); if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) { - len = SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(hwnd, EM_SETSEL, len, len); + len = send_to_LB(This, This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); + set_text_and_selection(This, hwnd, msg, len, len); heap_free(msg); } } else { - UINT len; - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)This->txtbackup); - len = strlenW(This->txtbackup); - SendMessageW(hwnd, EM_SETSEL, len, len); + UINT len = strlenW(This->txtbackup); + set_text_and_selection(This, hwnd, This->txtbackup, len, len); } return 0; } @@ -223,7 +239,9 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (wParam < ' ') { handle_control_char: - len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + edit_proc = (WNDPROC)GetWindowLongPtrW(hwnd, GWLP_WNDPROC); + edit_proc = edit_proc == ACEditSubclassProc ? This->wpOrigEditProc : edit_proc; + len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXTLENGTH, 0, 0); if ((This->options & ACO_AUTOSUGGEST) && len == 0) { ShowWindow(This->hwndListBox, SW_HIDE); return ret; @@ -233,17 +251,19 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, } else { autocomplete_text: - len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + edit_proc = (WNDPROC)GetWindowLongPtrW(hwnd, GWLP_WNDPROC); + edit_proc = edit_proc == ACEditSubclassProc ? This->wpOrigEditProc : edit_proc; + len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXTLENGTH, 0, 0); } old_len = len+1; /* include NUL */ if (!(hwndText = heap_alloc(old_len * sizeof(WCHAR)))) return ret; - len = SendMessageW(hwnd, WM_GETTEXT, old_len, (LPARAM)hwndText); + len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXT, old_len, (LPARAM)hwndText); if (len+1 != old_len) hwndText = heap_realloc(hwndText, len+1); - SendMessageW(This->hwndListBox, LB_RESETCONTENT, 0, 0); + send_to_LB(This, This->hwndListBox, LB_RESETCONTENT, 0, 0); /* Set txtbackup to point to hwndText itself (which must not be released) */ heap_free(This->txtbackup); @@ -276,8 +296,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, } else tmp = strs; - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)tmp); - SendMessageW(hwnd, EM_SETSEL, len, strslen); + set_text_and_selection(This, hwnd, tmp, len, strslen); if (tmp != strs) heap_free(tmp); @@ -288,7 +307,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, } if (This->options & ACO_AUTOSUGGEST) - SendMessageW(This->hwndListBox, LB_INSERTSTRING, -1, (LPARAM)strs); + send_to_LB(This, This->hwndListBox, LB_INSERTSTRING, -1, (LPARAM)strs); cpt++; } @@ -299,8 +318,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (This->options & ACO_AUTOSUGGEST) { if (cpt) { RECT r; - UINT height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0); - SendMessageW(This->hwndListBox, LB_CARETOFF, 0, 0); + UINT height = send_to_LB(This, This->hwndListBox, LB_GETITEMHEIGHT, 0, 0); + send_to_LB(This, This->hwndListBox, LB_CARETOFF, 0, 0); GetWindowRect(hwnd, &r); SetParent(This->hwndListBox, HWND_DESKTOP); /* It seems that Windows XP displays 7 lines at most @@ -338,24 +357,23 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, switch (uMsg) { case WM_MOUSEMOVE: - sel = SendMessageW(hwnd, LB_ITEMFROMPOINT, 0, lParam); - SendMessageW(hwnd, LB_SETCURSEL, sel, 0); + sel = send_to_LB(This, hwnd, LB_ITEMFROMPOINT, 0, lParam); + send_to_LB(This, hwnd, LB_SETCURSEL, sel, 0); break; case WM_LBUTTONDOWN: - sel = SendMessageW(hwnd, LB_GETCURSEL, 0, 0); + sel = send_to_LB(This, hwnd, LB_GETCURSEL, 0, 0); if (sel < 0) break; - len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); + len = send_to_LB(This, hwnd, LB_GETTEXTLEN, sel, 0); if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) { - len = SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(This->hwndEdit, EM_SETSEL, 0, len); + len = send_to_LB(This, hwnd, LB_GETTEXT, sel, (LPARAM)msg); + set_text_and_selection(This, This->hwndEdit, msg, 0, len); ShowWindow(hwnd, SW_HIDE); heap_free(msg); } break; default: - return CallWindowProcW(This->wpOrigLBoxProc, hwnd, uMsg, wParam, lParam); + return send_to_LB(This, hwnd, uMsg, wParam, lParam); } return 0; } @@ -708,14 +726,14 @@ static HRESULT WINAPI IAutoCompleteDropDown_fnGetDropDownStatus( if (dropped) { int sel; - sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0); + sel = send_to_LB(This, This->hwndListBox, LB_GETCURSEL, 0, 0); if (sel >= 0) { DWORD len; - len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); + len = send_to_LB(This, This->hwndListBox, LB_GETTEXTLEN, sel, 0); *ppwszString = CoTaskMemAlloc((len+1)*sizeof(WCHAR)); - SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)*ppwszString); + send_to_LB(This, This->hwndListBox, LB_GETTEXT, sel, (LPARAM)*ppwszString); } else *ppwszString = NULL; -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- For some reason the continuous SetParent made it flicker badly when it changed its contents, so just set it once at listbox creation. It also removes unnecessary overhead at the same time. dlls/shell32/autocomplete.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index b71cbc0..1678d87 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -263,8 +263,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (len+1 != old_len) hwndText = heap_realloc(hwndText, len+1); - send_to_LB(This, This->hwndListBox, LB_RESETCONTENT, 0, 0); - /* Set txtbackup to point to hwndText itself (which must not be released) */ heap_free(This->txtbackup); This->txtbackup = hwndText; @@ -272,6 +270,10 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (!displayall && !len) return ret; + if (This->options & ACO_AUTOSUGGEST) { + send_to_LB(This, This->hwndListBox, WM_SETREDRAW, FALSE, 0); + send_to_LB(This, This->hwndListBox, LB_RESETCONTENT, 0, 0); + } IEnumString_Reset(This->enumstr); for(cpt = 0;;) { LPOLESTR strs = NULL; @@ -321,12 +323,12 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, UINT height = send_to_LB(This, This->hwndListBox, LB_GETITEMHEIGHT, 0, 0); send_to_LB(This, This->hwndListBox, LB_CARETOFF, 0, 0); GetWindowRect(hwnd, &r); - SetParent(This->hwndListBox, HWND_DESKTOP); /* It seems that Windows XP displays 7 lines at most and otherwise displays a vertical scroll bar */ SetWindowPos(This->hwndListBox, HWND_TOP, r.left, r.bottom + 1, r.right - r.left, height*min(cpt+1, 7), SWP_SHOWWINDOW ); + send_to_LB(This, This->hwndListBox, WM_SETREDRAW, TRUE, 0); } else { ShowWindow(This->hwndListBox, SW_HIDE); } @@ -393,6 +395,7 @@ static void create_listbox(IAutoCompleteImpl *This) if (This->hwndListBox) { This->wpOrigLBoxProc = (WNDPROC) SetWindowLongPtrW( This->hwndListBox, GWLP_WNDPROC, (LONG_PTR) ACLBoxSubclassProc); SetWindowLongPtrW( This->hwndListBox, GWLP_USERDATA, (LONG_PTR)This); + SetParent(This->hwndListBox, HWND_DESKTOP); } else This->options &= ~ACO_AUTOSUGGEST; -- 1.9.1
When selecting an item from the AutoComplete's listbox, the Return key should act the same as a left click on it (place the text, select it, and hide the listbox). This matches Windows behavior. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Also fixes quickComplete since apparently a VK_RETURN is sent as a WM_CHAR of '\n' when CTRL is pressed. dlls/shell32/autocomplete.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 1678d87..449e8a4 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -67,8 +67,10 @@ typedef struct IAutoComplete2 IAutoComplete2_iface; IAutoCompleteDropDown IAutoCompleteDropDown_iface; LONG ref; - BOOL initialized; - BOOL enabled; + BOOLEAN initialized; + BOOLEAN enabled; + UCHAR no_fwd_char; + AUTOCOMPLETEOPTIONS options; HWND hwndEdit; HWND hwndListBox; WNDPROC wpOrigEditProc; @@ -76,7 +78,6 @@ typedef struct WCHAR *txtbackup; WCHAR *quickComplete; IEnumString *enumstr; - AUTOCOMPLETEOPTIONS options; } IAutoCompleteImpl; static const WCHAR autocomplete_propertyW[] = {'W','i','n','e',' ','A','u','t','o', @@ -177,14 +178,32 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; sz *= 2; } + + This->no_fwd_char = '\n'; /* CTRL+RETURN char */ if (This->options & ACO_AUTOSUGGEST) ShowWindow(This->hwndListBox, SW_HIDE); heap_free(hwndText); return 0; } - if (This->options & ACO_AUTOSUGGEST) + if (This->options & ACO_AUTOSUGGEST) { + if (IsWindowVisible(This->hwndListBox)) { + HWND hwndListBox = This->hwndListBox; + sel = send_to_LB(This, hwndListBox, LB_GETCURSEL, 0, 0); + if (sel >= 0) { + len = send_to_LB(This, hwndListBox, LB_GETTEXTLEN, sel, 0); + if ((hwndText = heap_alloc((len + 1)*sizeof(WCHAR)))) { + len = send_to_LB(This, hwndListBox, LB_GETTEXT, sel, (LPARAM)hwndText); + set_text_and_selection(This, hwnd, hwndText, 0, len); + This->no_fwd_char = '\r'; /* RETURN char */ + ShowWindow(hwndListBox, SW_HIDE); + heap_free(hwndText); + return 0; + } + } + } ShowWindow(This->hwndListBox, SW_HIDE); + } break; case VK_UP: case VK_DOWN: @@ -232,9 +251,14 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); goto handle_control_char; } + This->no_fwd_char = '\0'; return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); case WM_CHAR: case WM_UNICHAR: + if (wParam == This->no_fwd_char) + return 0; + This->no_fwd_char = '\0'; + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); if (wParam < ' ') { -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This matches Windows behavior and gets rid of annoying caret movement. dlls/shell32/autocomplete.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 449e8a4..9bc617b 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -308,7 +308,13 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; if (!strncmpiW(hwndText, strs, len)) { - if (cpt == 0 && noautoappend == FALSE) + DWORD sel_start; + + /* Don't auto-append unless the caret is at the end */ + if (cpt == 0 && noautoappend == FALSE + && (CallWindowProcW(edit_proc, hwnd, EM_GETSEL, (WPARAM)&sel_start, 0), + sel_start == len) + ) { /* The character capitalization can be different, so merge hwndText and strs into a new string */ -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 9bc617b..c5bc582 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -109,6 +109,14 @@ static void set_text_and_selection(IAutoCompleteImpl *This, HWND hwnd, WCHAR *te CallWindowProcW(proc, hwnd, EM_SETSEL, start, end); } +static void hide_listbox(IAutoCompleteImpl *ac, HWND hwnd) +{ + /* hide the AutoComplete listbox but also reset its contents to avoid + memory use buildup for tons of edit controls with AutoComplete */ + ShowWindow(hwnd, SW_HIDE); + send_to_LB(ac, hwnd, LB_RESETCONTENT, 0, 0); +} + static void destroy_autocomplete_object(IAutoCompleteImpl *ac) { ac->hwndEdit = NULL; @@ -137,12 +145,12 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { case CB_SHOWDROPDOWN: if (This->options & ACO_AUTOSUGGEST) - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); break; case WM_KILLFOCUS: if ((This->options & ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) { - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); } return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); case WM_KEYDOWN: @@ -181,7 +189,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, This->no_fwd_char = '\n'; /* CTRL+RETURN char */ if (This->options & ACO_AUTOSUGGEST) - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); heap_free(hwndText); return 0; } @@ -196,13 +204,13 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, len = send_to_LB(This, hwndListBox, LB_GETTEXT, sel, (LPARAM)hwndText); set_text_and_selection(This, hwnd, hwndText, 0, len); This->no_fwd_char = '\r'; /* RETURN char */ - ShowWindow(hwndListBox, SW_HIDE); + hide_listbox(This, hwndListBox); heap_free(hwndText); return 0; } } } - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); } break; case VK_UP: @@ -267,7 +275,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, edit_proc = edit_proc == ACEditSubclassProc ? This->wpOrigEditProc : edit_proc; len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXTLENGTH, 0, 0); if ((This->options & ACO_AUTOSUGGEST) && len == 0) { - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); return ret; } if (wParam != 0x16 /* ^V (paste) */) @@ -360,7 +368,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, SWP_SHOWWINDOW ); send_to_LB(This, This->hwndListBox, WM_SETREDRAW, TRUE, 0); } else { - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); } } return ret; @@ -400,7 +408,7 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) { len = send_to_LB(This, hwnd, LB_GETTEXT, sel, (LPARAM)msg); set_text_and_selection(This, This->hwndEdit, msg, 0, len); - ShowWindow(hwnd, SW_HIDE); + hide_listbox(This, hwnd); heap_free(msg); } break; -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index c5bc582..c145dc0 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -521,6 +521,8 @@ static HRESULT WINAPI IAutoComplete2_fnEnable( TRACE("(%p)->(%s)\n", This, (fEnable)?"true":"false"); + if (This->enabled && !fEnable && This->hwndListBox) + hide_listbox(This, This->hwndListBox); This->enabled = fEnable; return hr; @@ -706,6 +708,8 @@ static HRESULT WINAPI IAutoComplete2_fnSetOptions( if ((This->options & ACO_AUTOSUGGEST) && This->hwndEdit && !This->hwndListBox) create_listbox(This); + else if (!(This->options & ACO_AUTOSUGGEST) && This->hwndListBox) + hide_listbox(This, This->hwndListBox); return hr; } -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- I had to remove the #include of initguid.h because it gave a duplicate IID_ShellFolderView definition with shlobj.h, which is needed for IACList. dlls/shell32/tests/autocomplete.c | 144 +++++++++++++++++++++++++++++++++++--- 1 file changed, 136 insertions(+), 8 deletions(-) diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c index 403b7d5..d4c4545 100644 --- a/dlls/shell32/tests/autocomplete.c +++ b/dlls/shell32/tests/autocomplete.c @@ -25,8 +25,8 @@ #include "windows.h" #include "shobjidl.h" #include "shlguid.h" -#include "initguid.h" #include "shldisp.h" +#include "shlobj.h" #include "wine/heap.h" #include "wine/test.h" @@ -269,10 +269,13 @@ static LRESULT CALLBACK HijackerWndProc2(HWND hWnd, UINT msg, WPARAM wParam, LPA struct string_enumerator { IEnumString IEnumString_iface; + IACList IACList_iface; LONG ref; WCHAR **data; int data_len; int cur; + UINT num_expand; + WCHAR last_expand[32]; }; static struct string_enumerator *impl_from_IEnumString(IEnumString *iface) @@ -282,15 +285,19 @@ static struct string_enumerator *impl_from_IEnumString(IEnumString *iface) static HRESULT WINAPI string_enumerator_QueryInterface(IEnumString *iface, REFIID riid, void **ppv) { + struct string_enumerator *this = impl_from_IEnumString(iface); if (IsEqualGUID(riid, &IID_IEnumString) || IsEqualGUID(riid, &IID_IUnknown)) + *ppv = &this->IEnumString_iface; + else if (IsEqualGUID(riid, &IID_IACList)) + *ppv = &this->IACList_iface; + else { - IUnknown_AddRef(iface); - *ppv = iface; - return S_OK; + *ppv = NULL; + return E_NOINTERFACE; } - *ppv = NULL; - return E_NOINTERFACE; + IUnknown_AddRef(&this->IEnumString_iface); + return S_OK; } static ULONG WINAPI string_enumerator_AddRef(IEnumString *iface) @@ -361,7 +368,7 @@ static HRESULT WINAPI string_enumerator_Clone(IEnumString *iface, IEnumString ** return E_NOTIMPL; } -static IEnumStringVtbl string_enumerator_vtlb = +static IEnumStringVtbl string_enumerator_vtbl = { string_enumerator_QueryInterface, string_enumerator_AddRef, @@ -372,22 +379,141 @@ static IEnumStringVtbl string_enumerator_vtlb = string_enumerator_Clone }; +static struct string_enumerator *impl_from_IACList(IACList *iface) +{ + return CONTAINING_RECORD(iface, struct string_enumerator, IACList_iface); +} + +static HRESULT WINAPI aclist_QueryInterface(IACList *iface, REFIID riid, void **ppv) +{ + return string_enumerator_QueryInterface(&impl_from_IACList(iface)->IEnumString_iface, riid, ppv); +} + +static ULONG WINAPI aclist_AddRef(IACList *iface) +{ + return string_enumerator_AddRef(&impl_from_IACList(iface)->IEnumString_iface); +} + +static ULONG WINAPI aclist_Release(IACList *iface) +{ + return string_enumerator_Release(&impl_from_IACList(iface)->IEnumString_iface); +} + +static HRESULT WINAPI aclist_Expand(IACList *iface, const WCHAR *expand) +{ + struct string_enumerator *this = impl_from_IACList(iface); + + /* see what we get called with and how many times, + don't actually do any expansion of the strings */ + memcpy(this->last_expand, expand, min((lstrlenW(expand)+1)*sizeof(WCHAR), sizeof(this->last_expand))); + this->last_expand[ARRAY_SIZE(this->last_expand)-1] = '\0'; + this->num_expand++; + + return S_OK; +} + +static IACListVtbl aclist_vtbl = +{ + aclist_QueryInterface, + aclist_AddRef, + aclist_Release, + aclist_Expand +}; + static HRESULT string_enumerator_create(void **ppv, WCHAR **suggestions, int count) { struct string_enumerator *object; object = heap_alloc_zero(sizeof(*object)); - object->IEnumString_iface.lpVtbl = &string_enumerator_vtlb; + object->IEnumString_iface.lpVtbl = &string_enumerator_vtbl; + object->IACList_iface.lpVtbl = &aclist_vtbl; object->ref = 1; object->data = suggestions; object->data_len = count; object->cur = 0; + object->num_expand = 0; *ppv = &object->IEnumString_iface; return S_OK; } +static void test_aclist_expand(HWND hwnd_edit, void *enumerator) +{ + struct string_enumerator *obj = (struct string_enumerator*)enumerator; + static WCHAR str1[] = {'t','e','s','t',0}; + static WCHAR str1a[] = {'t','e','s','t','\\',0}; + static WCHAR str2[] = {'t','e','s','t','\\','f','o','o','\\','b','a','r','\\','b','a',0}; + static WCHAR str2a[] = {'t','e','s','t','\\','f','o','o','\\','b','a','r','\\',0}; + static WCHAR str2b[] = {'t','e','s','t','\\','f','o','o','\\','b','a','r','\\','b','a','z','_','b','b','q','\\',0}; + MSG msg; + + ok(obj->num_expand == 0, "Expected 0 expansions, got %u\n", obj->num_expand); + SendMessageW(hwnd_edit, WM_SETTEXT, 0, (LPARAM)str1); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str1)-1, ARRAY_SIZE(str1)-1); + SendMessageW(hwnd_edit, WM_CHAR, '\\', 1); + Sleep(100); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + ok(obj->num_expand == 1, "Expected 1 expansion, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str1a) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str1a), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, WM_SETTEXT, 0, (LPARAM)str2); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str2)-1, ARRAY_SIZE(str2)-1); + SendMessageW(hwnd_edit, WM_CHAR, 'z', 1); + Sleep(100); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + ok(obj->num_expand == 2, "Expected 2 expansions, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str2a) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str2a), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, WM_CHAR, '_', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'b', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'b', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'q', 1); + Sleep(100); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + ok(obj->num_expand == 2, "Expected 2 expansions, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str2a) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str2a), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, WM_CHAR, '\\', 1); + Sleep(100); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + ok(obj->num_expand == 3, "Expected 3 expansions, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str2b) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str2b), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str1a)-1, -1); + SendMessageW(hwnd_edit, WM_CHAR, 'x', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'y', 1); + Sleep(100); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + ok(obj->num_expand == 4, "Expected 4 expansions, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str1a) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str1a), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str1)-1, -1); + SendMessageW(hwnd_edit, WM_CHAR, 'x', 1); + Sleep(100); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + ok(obj->num_expand == 4, "Expected 4 expansions, got %u\n", obj->num_expand); +} + static void test_custom_source(void) { static WCHAR str_alpha[] = {'t','e','s','t','1',0}; @@ -466,6 +592,8 @@ static void test_custom_source(void) ok(lstrcmpW(str_beta, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_beta), wine_dbgstr_w(buffer)); /* end of hijacks */ + test_aclist_expand(hwnd_edit, enumerator); + ShowWindow(hMainWnd, SW_HIDE); DestroyWindow(hwnd_edit); } -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This enables Total Commander to show full paths on many edit boxes, like on Windows, instead of just the current directory's entries. I couldn't get the / delimiter to work at all on Windows XP, so MSDN is probably wrong here. Despite that, I've actually added support for it in this patch, as a Wine extension, to help with unix-style paths autocompletion since we also support those. After this patch series, it now matches Windows behavior very closely, and it finally makes AutoComplete useable for me. :) dlls/shell32/autocomplete.c | 75 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index c145dc0..5262a9f 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -2,6 +2,7 @@ * AutoComplete interfaces implementation. * * Copyright 2004 Maxime Bellengé <maxime.bellenge(a)laposte.net> + * Copyright 2018 Gabriel Ivăncescu <gabrielopcode(a)gmail.com> * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -23,7 +24,7 @@ - ACO_AUTOAPPEND style - ACO_AUTOSUGGEST style - ACO_UPDOWNKEYDROPSLIST style - + - IACList::Expand - Handle pwzsRegKeyPath and pwszQuickComplete in Init TODO: @@ -78,6 +79,7 @@ typedef struct WCHAR *txtbackup; WCHAR *quickComplete; IEnumString *enumstr; + IACList *aclist; } IAutoCompleteImpl; static const WCHAR autocomplete_propertyW[] = {'W','i','n','e',' ','A','u','t','o', @@ -117,6 +119,61 @@ static void hide_listbox(IAutoCompleteImpl *ac, HWND hwnd) send_to_LB(ac, hwnd, LB_RESETCONTENT, 0, 0); } +static void aclist_expand(IAutoCompleteImpl *ac, WCHAR *txt) +{ + /* call IACList::Expand only when needed + '/' is allowed as a delim for unix paths */ + WCHAR c, *p, *last_delim, *old_txt = ac->txtbackup; + size_t i = 0; + + /* skip the shared prefix */ + while ((c = tolowerW(txt[i])) == tolowerW(old_txt[i])) { + if (c == '\0') + return; + i++; + } + + /* they differ at this point, check for a delim further in txt */ + p = &txt[i]; + while (*p != '\0') { + if (*p == '\\' || *p == '/') { + last_delim = p; + do { + p++; + if (*p == '\\' || *p == '/') + last_delim = p; + } while(*p != '\0'); + goto expand; + } + p++; + } + + /* txt has no delim after i, check for a delim further in old_txt */ + p = &old_txt[i]; + while (*p != '\0') { + if (*p == '\\' || *p == '/') { + /* find the delim before i (if any) */ + while (i--) { + if (txt[i] == '\\' || txt[i] == '/') { + last_delim = &txt[i]; + goto expand; + } + } + break; /* Windows doesn't expand without a delim */ + } + p++; + } + + /* they differ, but without any different delims, so no need to expand */ + return; + +expand: + c = last_delim[1]; + last_delim[1] = '\0'; + IACList_Expand(ac->aclist, txt); + last_delim[1] = c; +} + static void destroy_autocomplete_object(IAutoCompleteImpl *ac) { ac->hwndEdit = NULL; @@ -295,6 +352,16 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (len+1 != old_len) hwndText = heap_realloc(hwndText, len+1); + /* Reset it here to simplify the logic in aclist_expand for + empty strings, since it tracks changes using txtbackup, + and Reset needs to be called before IACList::Expand */ + IEnumString_Reset(This->enumstr); + if (This->aclist) { + aclist_expand(This, hwndText); + if (hwndText[len-1] == '\\' || hwndText[len-1] == '/') + noautoappend = TRUE; + } + /* Set txtbackup to point to hwndText itself (which must not be released) */ heap_free(This->txtbackup); This->txtbackup = hwndText; @@ -306,7 +373,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, send_to_LB(This, This->hwndListBox, WM_SETREDRAW, FALSE, 0); send_to_LB(This, This->hwndListBox, LB_RESETCONTENT, 0, 0); } - IEnumString_Reset(This->enumstr); for(cpt = 0;;) { LPOLESTR strs = NULL; ULONG fetched; @@ -504,6 +570,8 @@ static ULONG WINAPI IAutoComplete2_fnRelease( heap_free(This->txtbackup); if (This->enumstr) IEnumString_Release(This->enumstr); + if (This->aclist) + IACList_Release(This->aclist); heap_free(This); } return refCount; @@ -567,6 +635,9 @@ static HRESULT WINAPI IAutoComplete2_fnInit( return E_NOINTERFACE; } + if (FAILED (IUnknown_QueryInterface (punkACL, &IID_IACList, (LPVOID*)&This->aclist))) + This->aclist = NULL; + This->initialized = TRUE; This->hwndEdit = hwndEdit; -- 1.9.1
On Wed, Sep 05, 2018 at 07:13:03PM +0300, Gabriel Ivăncescu wrote:
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 read the registry value with any size.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
Supersedes 150480
v2: Retrieve the registry value without failing in rare race conditions. v3: Don't open the HKEY_LOCAL_MACHINE twice under some circumstances.
dlls/shell32/autocomplete.c | 90 +++++++++++++++++++++++++++++---------------- 1 file changed, 58 insertions(+), 32 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index cf8da50..051644a 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -486,40 +486,66 @@ 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; + DWORD type, sz = MAX_PATH * sizeof(WCHAR); + BOOL try_HKLM; + BYTE *qc; + 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); + try_HKLM = TRUE;
This try_HKLM thing is ugly. I'd suggest having a outer loop over the two registry root keys (HKCU and HKLM) which tries to access the supplied key / value. If the access succeeds then break out of the loop. Also, be consistent about your brace style (I know the function is already mixed). I'd be happy if you moved all of the opening braces, in code you're rewriting, to a new line.
+ + /* 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); + try_HKLM = FALSE; + } + + if (res == ERROR_SUCCESS) { + while ((qc = heap_alloc(sz)) != NULL) { + res = RegQueryValueExW(hKey, value, NULL, &type, qc, &sz); + if (res == ERROR_SUCCESS && type == REG_SZ) { + This->quickComplete = heap_realloc(qc, sz); + break; + } + heap_free(qc); + + if (res != ERROR_MORE_DATA || type != REG_SZ) { + if (!try_HKLM) + break; + RegCloseKey(hKey); + res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, key, 0, KEY_READ, &hKey); + if (res != ERROR_SUCCESS) + goto free_key_str; + sz = MAX_PATH * sizeof(WCHAR); + try_HKLM = FALSE; + } + } + 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
This try_HKLM thing is ugly. I'd suggest having a outer loop over the two registry root keys (HKCU and HKLM) which tries to access the supplied key / value. If the access succeeds then break out of the loop.
I'm afraid I don't understand how to do it that way without duplicating the RegQueryValue part of the code. The problem is that even if the key exists under HKCU, the value might not. This means RegQueryValueEx would fail, even though the key itself is open under HKCU. But then I have to retry under HKLM. Which means I would have to duplicate the RegQueryValueEx at least twice (if not 3 times) just to see if the value exists first. And then have to keep it in the real loop until the buffer is satisfied. Originally I had "duplicated" this with just getting the size initially, but Alexandre told me to get the string right away with a reasonable size (here MAX_PATH like the original code) so I had to devise this ugly BOOL thing to reduce the amount of code duplication and keep it to a minimum. Any rough sketches of how it's supposed to be done without duplicating the code? Or should I duplicate it?
Also, be consistent about your brace style (I know the function is already mixed). I'd be happy if you moved all of the opening braces, in code you're rewriting, to a new line.
I actually also prefer it on its own line, but I was trying to copy the style of surrounding code so it doesn't feel out of place. I'll send it on its own line then :-)
On Thu, Sep 06, 2018 at 04:31:04PM +0300, Gabriel Ivăncescu wrote:
This try_HKLM thing is ugly. I'd suggest having a outer loop over the two registry root keys (HKCU and HKLM) which tries to access the supplied key / value. If the access succeeds then break out of the loop.
I'm afraid I don't understand how to do it that way without duplicating the RegQueryValue part of the code.
The problem is that even if the key exists under HKCU, the value might not. This means RegQueryValueEx would fail, even though the key itself is open under HKCU. But then I have to retry under HKLM. Which means I would have to duplicate the RegQueryValueEx at least twice (if not 3 times) just to see if the value exists first. And then have to keep it in the real loop until the buffer is satisfied. Originally I had "duplicated" this with just getting the size initially, but Alexandre told me to get the string right away with a reasonable size (here MAX_PATH like the original code) so I had to devise this ugly BOOL thing to reduce the amount of code duplication and keep it to a minimum.
Any rough sketches of how it's supposed to be done without duplicating the code? Or should I duplicate it?
Well something like: HKEY roots[] = {HKCU, HKLM}; int i; for (i = 0; i < ARRAY_SIZE(roots); i++) { if (RegOpenKeyEx(roots[i], key, ..., &hkey) == ERROR_SUCCESS) { ret = query_value(hkey, value); RegCloseKey(hkey); if (ret == ERROR_SUCCESS) { This->quick_complete = data; break; } } } where query_value does the appropriate calls to RegQueryValueEx(). Huw.
On Thu, Sep 6, 2018 at 5:08 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Well something like:
HKEY roots[] = {HKCU, HKLM}; int i;
for (i = 0; i < ARRAY_SIZE(roots); i++) { if (RegOpenKeyEx(roots[i], key, ..., &hkey) == ERROR_SUCCESS) { ret = query_value(hkey, value); RegCloseKey(hkey); if (ret == ERROR_SUCCESS) { This->quick_complete = data; break; } } }
where query_value does the appropriate calls to RegQueryValueEx().
Huw.
Thanks, I completely misunderstood it before, I thought you wanted just a dummy loop for the break (like a localized goto). Now it is more reasonable indeed :-)
participants (3)
-
Gabriel Ivăncescu -
Huw Davies -
Huw Davies