[PATCH v4 01/11] 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 <gabrielopcode(a)gmail.com>
---

This patch series supersedes the series starting with 150630 (second half will come after these are committed)

v2: Retrieve the registry value without failing in rare race conditions.
v3: Don't open the HKEY_LOCAL_MACHINE twice under some circumstances.
v4: Adjusted with a loop as suggested by Huw Davies.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

 dlls/shell32/autocomplete.c | 81 +++++++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 32 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index cf8da50..b2c388e 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -486,40 +486,57 @@ 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)
+    {
+        static const HKEY roots[] = { HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE };
+        WCHAR *key, *value;
+        DWORD type, sz;
+        BYTE *qc;
+        HKEY hKey;
+        LSTATUS res;
+        size_t len;
+        UINT i;
+
+        /* 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++;
+
+            for (i = 0; i < ARRAY_SIZE(roots); i++)
+            {
+                if (RegOpenKeyExW(roots[i], key, 0, KEY_READ, &hKey) != ERROR_SUCCESS)
+                    continue;
+                sz = MAX_PATH * sizeof(WCHAR);
+
+                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);
+                        i = ARRAY_SIZE(roots);
+                        break;
+                    }
+                    heap_free(qc);
+                    if (res != ERROR_MORE_DATA || type != REG_SZ)
+                        break;
+                }
+                RegCloseKey(hKey);
+            }
+            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

[PATCH v2 02/11] shell32/autocomplete: Fix handling of Backspace and Delete

[Gabriel Ivăncescu]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=22255
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/shell32/autocomplete.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index b2c388e..4ec7387 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -199,19 +199,7 @@ 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:
-                    ;
             }
 
             SendMessageW(This->hwndListBox, LB_RESETCONTENT, 0, 0);
-- 
1.9.1

[PATCH v2 02/11] shell32/autocomplete: Fix handling of Backspace and Delete

[Huw Davies]

Signed-off-by: Huw Davies <huw(a)codeweavers.com>

[PATCH v2 03/11] shell32/autocomplete: Handle heap_alloc failure and fix a vulnerability by avoiding the use of sprintf

[Gabriel Ivăncescu]

The quickComplete format can have stuff like %1234s or %*.* or more than
one format argument, which can be exploited since the format string can be
read from the registry.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/shell32/autocomplete.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index 4ec7387..22f1a61 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,30 @@ 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 *buf;
+                        size_t len = strlenW(hwndText);
+                        size_t sz = strlenW(This->quickComplete) + 1;
+                        sz += max(len, 2);  /* %s is 2 chars */
+
+                        /* Replace the first %s directly without using snprintf, to avoid
+                           exploits since the format string can be retrieved from the registry */
+                        if ((buf = heap_alloc(sz * sizeof(WCHAR))))
+                        {
+                            WCHAR *qc = This->quickComplete, *dst = buf;
+                            do
+                            {
+                                if (qc[0] == '%' && qc[1] == 's')
+                                {
+                                    memcpy(dst, hwndText, len * sizeof(WCHAR));
+                                    strcpyW(dst + len, qc + 2);
+                                    break;
+                                }
+                                *dst++ = *qc++;
+                            } while (qc[-1] != '\0');
+                            SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)buf);
+                            SendMessageW(hwnd, EM_SETSEL, 0, sz-1);
+                            heap_free(buf);
+                        }
                     }
 
                     ShowWindow(This->hwndListBox, SW_HIDE);
@@ -177,10 +195,10 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                             SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0);
                             if (sel != -1) {
                                 WCHAR *msg;
-                                int len;
+                                UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
 
-                                len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
-                                msg = heap_alloc((len + 1)*sizeof(WCHAR));
+                                if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
+                                    return 0;
                                 SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg);
                                 SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg);
                                 SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg));
@@ -287,7 +305,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:
@@ -299,7 +318,8 @@ 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));
+            if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
+                break;
             SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg);
             SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg);
             SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg));
-- 
1.9.1

[PATCH v2 03/11] shell32/autocomplete: Handle heap_alloc failure and fix a vulnerability by avoiding the use of sprintf

[Huw Davies]

On Thu, Sep 06, 2018 at 09:26:13PM +0300, Gabriel Ivăncescu wrote:
> The quickComplete format can have stuff like %1234s or %*.* or more than
> one format argument, which can be exploited since the format string can be
> read from the registry.
>
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>  dlls/shell32/autocomplete.c | 42 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
> index 4ec7387..22f1a61 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,30 @@ 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);

I thought we'd agreed to split this into two patches?
> +                        WCHAR *buf;
> +                        size_t len = strlenW(hwndText);
> +                        size_t sz = strlenW(This->quickComplete) + 1;
> +                        sz += max(len, 2);  /* %s is 2 chars */
> +
> +                        /* Replace the first %s directly without using snprintf, to avoid
> +                           exploits since the format string can be retrieved from the registry */
> +                        if ((buf = heap_alloc(sz * sizeof(WCHAR))))
> +                        {
> +                            WCHAR *qc = This->quickComplete, *dst = buf;
> +                            do
> +                            {
> +                                if (qc[0] == '%' && qc[1] == 's')
> +                                {
> +                                    memcpy(dst, hwndText, len * sizeof(WCHAR));
> +                                    strcpyW(dst + len, qc + 2);
> +                                    break;
> +                                }
> +                                *dst++ = *qc++;
> +                            } while (qc[-1] != '\0');

Moving the sprintf replacement to a helper function would be good.  Also,
it should probably cope with unescaping "%%".
> +                            SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)buf);
> +                            SendMessageW(hwnd, EM_SETSEL, 0, sz-1);
> +                            heap_free(buf);
> +                        }
>                      }
>
> ShowWindow(This->hwndListBox, SW_HIDE);
> @@ -177,10 +195,10 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
>                              SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0);
>                              if (sel != -1) {
>                                  WCHAR *msg;
> -                                int len;
> +                                UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
>
> -                                len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
> -                                msg = heap_alloc((len + 1)*sizeof(WCHAR));
> +                                if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
> +                                    return 0;

Err, this suggests two thing to me:

1. You're not testing properly.
2. You're not reviewing you patches properly.

In addition, spaces around the '*' would be nice.
> SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg);
>                                  SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg);
>                                  SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg));
> @@ -287,7 +305,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:
> @@ -299,7 +318,8 @@ 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));
> +            if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
> +                break;

Same issue here.
> SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg);
>              SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg);
>              SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg));
> -- 
> 1.9.1

[PATCH v2 03/11] shell32/autocomplete: Handle heap_alloc failure and fix a vulnerability by avoiding the use of sprintf

[Gabriel Ivăncescu]

On Fri, Sep 7, 2018 at 12:02 PM, Huw Davies <huw(a)codeweavers.com> wrote:
> I thought we'd agreed to split this into two patches?
> > +                        WCHAR *buf;
> > +                        size_t len = strlenW(hwndText);
> > +                        size_t sz = strlenW(This->quickComplete) + 1;
> > +                        sz += max(len, 2);  /* %s is 2 chars */
> > +
> > +                        /* Replace the first %s directly without using snprintf, to avoid
> > +                           exploits since the format string can be retrieved from the registry */
> > +                        if ((buf = heap_alloc(sz * sizeof(WCHAR))))
> > +                        {
> > +                            WCHAR *qc = This->quickComplete, *dst = buf;
> > +                            do
> > +                            {
> > +                                if (qc[0] == '%' && qc[1] == 's')
> > +                                {
> > +                                    memcpy(dst, hwndText, len * sizeof(WCHAR));
> > +                                    strcpyW(dst + len, qc + 2);
> > +                                    break;
> > +                                }
> > +                                *dst++ = *qc++;
> > +                            } while (qc[-1] != '\0');
>
> Moving the sprintf replacement to a helper function would be good.  Also,
> it should probably cope with unescaping "%%".

I must have missed the splitting part. And totally forgot about the
%%, and the max(len, 2) is useless now I'll just get rid of it (and
keep it as simply len).
> Err, this suggests two thing to me:
>
> 1. You're not testing properly.
> 2. You're not reviewing you patches properly.
>
> In addition, spaces around the '*' would be nice.

Oops, I rushed a bit with that change in and forgot to test because I
thought it was trivial, I was scared to be too slow after replying to
your points. Sorry about that.

[PATCH v2 03/11] shell32/autocomplete: Handle heap_alloc failure and fix a vulnerability by avoiding the use of sprintf

[Huw Davies]

On Fri, Sep 07, 2018 at 01:33:48PM +0300, Gabriel Ivăncescu wrote:
> Oops, I rushed a bit with that change in and forgot to test because I
> thought it was trivial, I was scared to be too slow after replying to
> your points. Sorry about that.

Please don't rush.  I'd much rather you spend the time fixing and
reviewing the patches yourself as opposed to sending in patches which
I then have to correct.  Sending in version n+1 five minutes after
I've commented on version n isn't going to fill me with confidence.
It's not a race, Wine is over 25 years old, it can wait another day or
so!

Huw.

[PATCH v2 04/11] 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 <gabrielopcode(a)gmail.com>
---
 dlls/shell32/autocomplete.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index 22f1a61..b5a165c 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -199,13 +199,15 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
 
                                 if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
                                     return 0;
-                                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,
             len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
             if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
                 break;
-            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);
             break;
-- 
1.9.1

[PATCH v2 05/11] shell32/autocomplete: Prevent txtbackup from being NULL to simplify the code

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/shell32/autocomplete.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index b5a165c..2630517 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -469,6 +469,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 v2 05/11] shell32/autocomplete: Prevent txtbackup from being NULL to simplify the code

[Huw Davies]

On Thu, Sep 06, 2018 at 09:26:15PM +0300, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>  dlls/shell32/autocomplete.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
> index b5a165c..2630517 100644
> --- a/dlls/shell32/autocomplete.c
> +++ b/dlls/shell32/autocomplete.c
> @@ -469,6 +469,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;
> +

How exactly is this simplifiying the code?  It looks like it's just
adding code to me.
> if (FAILED (IUnknown_QueryInterface (punkACL, &IID_IEnumString, (LPVOID*)&This->enumstr))) {
>          WARN("No IEnumString interface\n");
>          return E_NOINTERFACE;
> -- 
> 1.9.1

[PATCH v2 05/11] shell32/autocomplete: Prevent txtbackup from being NULL to simplify the code

[Gabriel Ivăncescu]

On Fri, Sep 7, 2018 at 12:03 PM, Huw Davies <huw(a)codeweavers.com> wrote:
> On Thu, Sep 06, 2018 at 09:26:15PM +0300, Gabriel Ivăncescu wrote:
>
> How exactly is this simplifiying the code?  It looks like it's just
> adding code to me.

Well when txtbackup is used currently (when using the arrow keys),
there's no check for NULL so I think the current code is wrong.
Instead of adding a check for NULL there, this simplifies it.

Please note that this will be more evident later in the patch series
when I add the IACList::Expand usage. Instead of checking for NULL
everytime which really only happens the first time it is ever used, I
just initialize it to something non-NULL. Less overhead and less
redundancy (no need to check for NULL twice later).

It probably doesn't make sense directly since it was split from the
larger patch which sets txtbackup to hwndText (so it can never be NULL
after the first time). Should I move it after the WM_KEYDOWN patch in
the series?

[PATCH v2 06/11] shell32/autocomplete: Don't call ShowWindow unless auto-suggest listbox is enabled

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 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 2630517..071c5bf 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -118,7 +118,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
     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))
@@ -163,7 +164,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                         }
                     }
 
-                    ShowWindow(This->hwndListBox, SW_HIDE);
+                    if (This->options & ACO_AUTOSUGGEST)
+                        ShowWindow(This->hwndListBox, SW_HIDE);
                     return 0;
                 case VK_LEFT:
                 case VK_RIGHT:
@@ -297,7 +299,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
         }
         default:
             return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
-
     }
 
     return 0;
@@ -350,6 +351,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;
 }
 
 /**************************************************************************
-- 
1.9.1

[PATCH v2 07/11] shell32/autocomplete: Get rid of control and filler BOOLs and simplify the code

[Gabriel Ivăncescu]

There's no need to have filler since cpt already can provide the same
information.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/shell32/autocomplete.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index 071c5bf..4485167 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -110,7 +110,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
     HRESULT hr;
     WCHAR hwndText[255];
     RECT r;
-    BOOL control, filled, displayall = FALSE;
+    BOOL displayall = FALSE;
     int cpt, height, sel;
 
     if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
@@ -136,8 +136,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
             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 *buf;
                         size_t len = strlenW(hwndText);
                         size_t sz = strlenW(This->quickComplete) + 1;
@@ -236,7 +236,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                 break;
 
             IEnumString_Reset(This->enumstr);
-            filled = FALSE;
             for(cpt = 0;;) {
                 LPOLESTR strs = NULL;
                 ULONG fetched;
@@ -246,7 +245,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                     break;
 
                 if (!strncmpiW(hwndText, strs, len)) {
-                    if (!filled && (This->options & ACO_AUTOAPPEND)) {
+                    if (cpt == 0 && (This->options & ACO_AUTOAPPEND)) {
                         WCHAR buffW[255];
 
                         strcpyW(buffW, hwndText);
@@ -259,19 +258,17 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                         }
                     }
 
-                    if (This->options & ACO_AUTOSUGGEST) {
+                    if (This->options & ACO_AUTOSUGGEST)
                         SendMessageW(This->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs);
-                        cpt++;
-                    }
 
-                    filled = TRUE;
+                    cpt++;
                 }
 
                 CoTaskMemFree(strs);
             }
 
             if (This->options & ACO_AUTOSUGGEST) {
-                if (filled) {
+                if (cpt) {
                     height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0);
                     SendMessageW(This->hwndListBox, LB_CARETOFF, 0, 0);
                     GetWindowRect(hwnd, &r);
-- 
1.9.1

[PATCH v2 08/11] shell32/autocomplete: Redesign the window proc to trigger on key presses instead of key release

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

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

v2: Most of it has been split into multiple patches, and I split it as much
as I could without introducing extra code that would just get removed on
the very next patch in the series.

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.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

 dlls/shell32/autocomplete.c | 111 ++++++++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 44 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index 4485167..fa965c0 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -108,10 +108,12 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
 {
     IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW);
     HRESULT hr;
-    WCHAR hwndText[255];
+    LRESULT ret;
+    WCHAR *hwndText;
+    UINT len, size, cpt;
     RECT r;
-    BOOL displayall = FALSE;
-    int cpt, height, sel;
+    BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND);
+    int height, sel;
 
     if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
 
@@ -127,21 +129,20 @@ 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 */
                     if (This->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000))
                     {
                         WCHAR *buf;
-                        size_t len = strlenW(hwndText);
-                        size_t sz = strlenW(This->quickComplete) + 1;
-                        sz += max(len, 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 */
 
                         /* Replace the first %s directly without using snprintf, to avoid
                            exploits since the format string can be retrieved from the registry */
@@ -162,29 +163,31 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                             SendMessageW(hwnd, EM_SETSEL, 0, sz-1);
                             heap_free(buf);
                         }
+                        if (This->options & ACO_AUTOSUGGEST)
+                            ShowWindow(This->hwndListBox, SW_HIDE);
+                        heap_free(hwndText);
+                        return 0;
                     }
 
                     if (This->options & ACO_AUTOSUGGEST)
                         ShowWindow(This->hwndListBox, SW_HIDE);
-                    return 0;
-                case VK_LEFT:
-                case VK_RIGHT:
-                    return 0;
+                    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)) {
+                    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);
@@ -211,29 +214,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;
+                    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);
+            }
+
+            size = len+1;
+            if (!(hwndText = heap_alloc(size * sizeof(WCHAR))))
+                return ret;
+            len = SendMessageW(hwnd, WM_GETTEXT, size, (LPARAM)hwndText);
+            if (len+1 != size)
+                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);
             for(cpt = 0;;) {
@@ -245,7 +270,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                     break;
 
                 if (!strncmpiW(hwndText, strs, len)) {
-                    if (cpt == 0 && (This->options & ACO_AUTOAPPEND)) {
+                    if (cpt == 0 && noautoappend == FALSE) {
                         WCHAR buffW[255];
 
                         strcpyW(buffW, hwndText);
@@ -282,9 +307,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                     ShowWindow(This->hwndListBox, SW_HIDE);
                 }
             }
-
-            break;
-        }
+            return ret;
         case WM_DESTROY:
         {
             WNDPROC proc = This->wpOrigEditProc;
-- 
1.9.1

[PATCH v2 08/11] shell32/autocomplete: Redesign the window proc to trigger on key presses instead of key release

[Huw Davies]

On Thu, Sep 06, 2018 at 09:26:18PM +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.
>
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>
> v2: Most of it has been split into multiple patches, and I split it as much
> as I could without introducing extra code that would just get removed on
> the very next patch in the series.
>
> 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.
>
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>
> dlls/shell32/autocomplete.c | 111 ++++++++++++++++++++++++++------------------
>  1 file changed, 67 insertions(+), 44 deletions(-)
>
> diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
> index 4485167..fa965c0 100644
> --- a/dlls/shell32/autocomplete.c
> +++ b/dlls/shell32/autocomplete.c
> @@ -108,10 +108,12 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
>  {
>      IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW);
>      HRESULT hr;
> -    WCHAR hwndText[255];
> +    LRESULT ret;
> +    WCHAR *hwndText;

While changing hwndText from a static buffer to a dynamically
allocated array is a good plan, it doesn't belong in this patch.

This whole function is a mess anyway.  It would probably help
to have the WM_KEYUP (or the new WM_KEYDOWN/WM_CHAR) handling
done in helper functions.
> +    UINT len, size, cpt;
>      RECT r;
> -    BOOL displayall = FALSE;
> -    int cpt, height, sel;
> +    BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND);
> +    int height, sel;
>
> if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
>
> @@ -127,21 +129,20 @@ 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 */
>                      if (This->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000))
>                      {
>                          WCHAR *buf;
> -                        size_t len = strlenW(hwndText);
> -                        size_t sz = strlenW(This->quickComplete) + 1;
> -                        sz += max(len, 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 */
>
> /* Replace the first %s directly without using snprintf, to avoid
>                             exploits since the format string can be retrieved from the registry */
> @@ -162,29 +163,31 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
>                              SendMessageW(hwnd, EM_SETSEL, 0, sz-1);
>                              heap_free(buf);
>                          }
> +                        if (This->options & ACO_AUTOSUGGEST)
> +                            ShowWindow(This->hwndListBox, SW_HIDE);
> +                        heap_free(hwndText);
> +                        return 0;
>                      }
>
> if (This->options & ACO_AUTOSUGGEST)
>                          ShowWindow(This->hwndListBox, SW_HIDE);
> -                    return 0;
> -                case VK_LEFT:
> -                case VK_RIGHT:
> -                    return 0;
> +                    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)) {
> +                    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);
> @@ -211,29 +214,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;
> +                    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);
> +            }
> +
> +            size = len+1;
> +            if (!(hwndText = heap_alloc(size * sizeof(WCHAR))))
> +                return ret;
> +            len = SendMessageW(hwnd, WM_GETTEXT, size, (LPARAM)hwndText);
> +            if (len+1 != size)
> +                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);
>              for(cpt = 0;;) {
> @@ -245,7 +270,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
>                      break;
>
> if (!strncmpiW(hwndText, strs, len)) {
> -                    if (cpt == 0 && (This->options & ACO_AUTOAPPEND)) {
> +                    if (cpt == 0 && noautoappend == FALSE) {
>                          WCHAR buffW[255];
>
> strcpyW(buffW, hwndText);
> @@ -282,9 +307,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
>                      ShowWindow(This->hwndListBox, SW_HIDE);
>                  }
>              }
> -
> -            break;
> -        }
> +            return ret;
>          case WM_DESTROY:
>          {
>              WNDPROC proc = This->wpOrigEditProc;
> -- 
> 1.9.1

[PATCH v2 08/11] shell32/autocomplete: Redesign the window proc to trigger on key presses instead of key release

[Gabriel Ivăncescu]

On Fri, Sep 7, 2018 at 12:07 PM, Huw Davies <huw(a)codeweavers.com> wrote:
> While changing hwndText from a static buffer to a dynamically
> allocated array is a good plan, it doesn't belong in this patch.
>
> This whole function is a mess anyway.  It would probably help
> to have the WM_KEYUP (or the new WM_KEYDOWN/WM_CHAR) handling
> done in helper functions.

If I do it in a separate patch before this, I'll have to add code that
I'll remove in the very next patch in the series. Is that acceptable?

Helper functions might work, but the problem is I wouldn't be able to
share code paths as easily between WM_KEYDOWN and WM_CHAR, like it is
now. I think they may complicate some things in such areas.

Also, that would require even more changes to the code. I tried to
keep even the indentation the same so that the patch is smaller, since
it's already too big.

[PATCH v2 09/11] shell32/autocomplete: Avoid another buffer overflow and handle arbitrary sizes for the auto-append string

[Gabriel Ivăncescu]

The previous code caps the auto-append text at 255 characters, which can
be easily exploited.

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/shell32/autocomplete.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index fa965c0..af8c067 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -271,12 +271,23 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
 
                 if (!strncmpiW(hwndText, strs, len)) {
                     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;
-- 
1.9.1

[PATCH v2 10/11] shell32/autocomplete: Localize some variables and simplify some calculations

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/shell32/autocomplete.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
index af8c067..ea15d8d 100644
--- a/dlls/shell32/autocomplete.c
+++ b/dlls/shell32/autocomplete.c
@@ -111,9 +111,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
     LRESULT ret;
     WCHAR *hwndText;
     UINT len, size, cpt;
-    RECT r;
     BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND);
-    int height, sel;
+    INT sel;
 
     if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
 
@@ -188,9 +187,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
                                 goto autocomplete_text;
                             }
                         } else {
-                            int count;
+                            INT count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0);
 
-                            count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0);
                             /* Change the selection */
                             sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0);
                             if (wParam == VK_UP)
@@ -305,14 +303,15 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
 
             if (This->options & ACO_AUTOSUGGEST) {
                 if (cpt) {
-                    height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0);
+                    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);
@@ -323,8 +322,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
         {
             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);
         }
-- 
1.9.1

[PATCH v2 11/11] shell32/tests: Send WM_CHAR instead of WM_KEYUP

[Gabriel Ivăncescu]

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

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/shell32/tests/autocomplete.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c
index 3c8de88..b9c6374 100644
--- a/dlls/shell32/tests/autocomplete.c
+++ b/dlls/shell32/tests/autocomplete.c
@@ -386,8 +386,7 @@ static void test_custom_source(void)
     ok(hr == S_OK, "IAutoComplete_Init failed: %x\n", hr);
 
     SendMessageW(hwnd_edit, WM_CHAR, 'a', 1);
-    /* Send a keyup message since wine doesn't handle WM_CHAR yet */
-    SendMessageW(hwnd_edit, WM_KEYUP, 'u', 1);
+    SendMessageW(hwnd_edit, WM_CHAR, 'u', 1);
     Sleep(100);
     while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE))
     {
-- 
1.9.1

[Huw Davies]

Signed-off-by: Huw Davies <huw(a)codeweavers.com>