[PATCH v3 01/17] shell32/autocomplete: Revamp pwzsRegKeyPath handling so it can deal with arbitrary sizes and make it more robust

[Gabriel Ivăncescu]

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 
---

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;

[PATCH 02/17] shell32/autocomplete: Fix handling of Backspace and Delete so that it matches Windows

[Gabriel Ivăncescu]

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 
---

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);

[PATCH 02/17] shell32/autocomplete: Fix handling of Backspace and Delete so that it matches Windows

[Huw Davies]

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
>
> 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

[PATCH 02/17] shell32/autocomplete: Fix handling of Backspace and Delete so that it matches Windows

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 1:00 PM, Huw Davies  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).

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Gabriel Ivăncescu]

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 
---
 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);

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Huw Davies]

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
>
> 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

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 1:24 PM, Huw Davies  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.

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Huw Davies]

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  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.

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 5:51 PM, Huw Davies  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.

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Huw Davies]

On 6 Sep 2018, at 16:05, Gabriel Ivăncescu  wrote:
> On Thu, Sep 6, 2018 at 5:51 PM, Huw Davies  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.

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 6:28 PM, Huw Davies  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.

[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Huw Davies]

On 6 Sep 2018, at 16:43, Gabriel Ivăncescu  wrote:
> On Thu, Sep 6, 2018 at 6:28 PM, Huw Davies  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.

[PATCH 04/17] shell32/autocomplete: Guard against multiple format args and malicious format strings for quickComplete

[Gabriel Ivăncescu]

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 
---
 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;
 }

[PATCH 05/17] shell32/autocomplete: Reduce the strlen calls because they are redundant

[Gabriel Ivăncescu]

We can retrieve the length of the string from the SendMessage calls already.

Signed-off-by: Gabriel Ivăncescu 
---
 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);
             }

[PATCH 06/17] shell32/tests: Send WM_CHAR instead of WM_KEYUP

[Gabriel Ivăncescu]

There's no need to send a WM_KEYUP anymore since it will now match Windows
behavior.

Signed-off-by: Gabriel Ivăncescu 
---
 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))
     {

[PATCH 06/17] shell32/tests: Send WM_CHAR instead of WM_KEYUP

[Huw Davies]

On Wed, Sep 05, 2018 at 07:13:08PM +0300, Gabriel Ivăncescu wrote:
> There's no need to send a WM_KEYUP anymore since it will now match Windows
> behavior.

Doesn't this need to go after 7/17 then?

Huw.

[PATCH 06/17] shell32/tests: Send WM_CHAR instead of WM_KEYUP

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 12:52 PM, Huw Davies  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.

[PATCH 06/17] shell32/tests: Send WM_CHAR instead of WM_KEYUP

[Huw Davies]

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  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).

[PATCH 06/17] shell32/tests: Send WM_CHAR instead of WM_KEYUP

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 5:11 PM, Huw Davies  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.

[PATCH 07/17] shell32/autocomplete: Redesign the window proc to trigger on key presses, handle arbitrary sizes and avoid another buffer overflow

[Gabriel Ivăncescu]

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 
---

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;

[PATCH 07/17] shell32/autocomplete: Redesign the window proc to trigger on key presses, handle arbitrary sizes and avoid another buffer overflow

[Huw Davies]

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
>
> 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

[PATCH 07/17] shell32/autocomplete: Redesign the window proc to trigger on key presses, handle arbitrary sizes and avoid another buffer overflow

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 12:55 PM, Huw Davies  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.

[PATCH 07/17] shell32/autocomplete: Redesign the window proc to trigger on key presses, handle arbitrary sizes and avoid another buffer overflow

[Huw Davies]

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  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.

[PATCH 07/17] shell32/autocomplete: Redesign the window proc to trigger on key presses, handle arbitrary sizes and avoid another buffer overflow

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 5:13 PM, Huw Davies  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.

[PATCH 08/17] shell32/autocomplete: Fix going up through the AutoComplete listbox

[Gabriel Ivăncescu]

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 
---
 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);

[PATCH 09/17] shell32/tests: Test hijacking the edit control's procedure after AutoComplete

[Gabriel Ivăncescu]

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 
---
 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);

[PATCH 10/17] shell32/autocomplete: Send messages directly to the edit control and AutoComplete listbox

[Gabriel Ivăncescu]

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 
---
 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;

[PATCH 11/17] shell32/autocomplete: Remove flicker while redrawing the AutoComplete listbox

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---

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;

[PATCH 12/17] shell32/autocomplete: Fix handling of Return key when an auto-suggestion item is selected

[Gabriel Ivăncescu]

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 
---

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 < ' ')
             {

[PATCH 13/17] shell32/autocomplete: Don't auto-append unless the caret is at the end

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---

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 */

[PATCH 14/17] shell32/autocomplete: Reset the contents of the AutoComplete listbox when hiding it

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 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;

[PATCH 15/17] shell32/autocomplete: Hide the AutoComplete listbox when auto-suggest is turned off or when it's disabled

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---
 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;
 }

[PATCH 16/17] shell32/tests: Test when IACList::Expand gets called and with what string

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---

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);
 }

[PATCH 17/17] shell32/autocomplete: Use the optional IACList interface and IACList::Expand, if available

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu 
---

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é 
+ *	Copyright 2018	Gabriel Ivăncescu 
  *
  * 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;

[Huw Davies]

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
>
> 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

[Gabriel Ivăncescu]

> 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 :-)

[Huw Davies]

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.

[Gabriel Ivăncescu]

On Thu, Sep 6, 2018 at 5:08 PM, Huw Davies  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 :-)