[PATCH 07/17] shell32/autocomplete: Redesign the window proc to trigger on key presses, handle arbitrary sizes and avoid another buffer overflow
Huw Davies
huw at codeweavers.com
Thu Sep 6 04:55:31 CDT 2018
On Wed, Sep 05, 2018 at 07:13:09PM +0300, Gabriel Ivăncescu wrote:
> AutoComplete currently shows up when the user releases a key, which is
> wrong. Windows does it when the user presses a key, so use both WM_KEYDOWN
> and WM_CHAR and redesign it so that it matches Windows behavior.
>
> At the same time, dynamically allocate the internal text buffer itself so we
> can handle any length, while avoiding the buffer overflow. The previous code
> caps the text at 255 characters (and in such case gives the wrong results),
> and is prone to exploits, as it assumes the enumerated string fits in 255
> characters as well, which can be easily exploited.
>
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> ---
>
> This patch does many changes because they are either minor or intertwined
> and hard to separate without breaking the compilation or Wine.
Well the patch is too big, so start with moving the minor stuff to a
separate patch and let's see where we're at. [Hint: any patch that
has "At the same time" in its commit message should probably be
split].
> A difficulty
> here stems from the fact that we need to handle not just WM_KEYDOWN but
> also WM_CHAR, which is harder than WM_KEYUP especially when trying to
> share code paths because control characters have to be handled separately
> (for auto-append purposes).
>
> For example VK_BACK is not processed in WM_KEYDOWN because it's done during
> WM_CHAR as the escape '\b' along the rest. That's not the case with VK_DELETE
> which has no WM_CHAR escape. This is important since the edit control *must*
> receive the message before us, otherwise the behavior will be wrong, so
> they have to be deferred to WM_CHAR and we can't do it in WM_KEYDOWN. That's
> also why we always make sure to pass the corresponding message to the edit
> control first.
>
> dlls/shell32/autocomplete.c | 169 ++++++++++++++++++++++++++------------------
> 1 file changed, 101 insertions(+), 68 deletions(-)
>
> diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
> index e5712e6..74728e0 100644
> --- a/dlls/shell32/autocomplete.c
> +++ b/dlls/shell32/autocomplete.c
> @@ -108,17 +108,19 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> {
> IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW);
> HRESULT hr;
> - WCHAR hwndText[255];
> - RECT r;
> - BOOL control, filled, displayall = FALSE;
> - int cpt, height, sel;
> + LRESULT ret;
> + WCHAR *hwndText;
> + UINT len, old_len, cpt;
> + BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND);
> + INT sel;
>
> if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
>
> switch (uMsg)
> {
> case CB_SHOWDROPDOWN:
> - ShowWindow(This->hwndListBox, SW_HIDE);
> + if (This->options & ACO_AUTOSUGGEST)
> + ShowWindow(This->hwndListBox, SW_HIDE);
> break;
> case WM_KILLFOCUS:
> if ((This->options & ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox))
> @@ -126,21 +128,19 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> ShowWindow(This->hwndListBox, SW_HIDE);
> }
> return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
> - case WM_KEYUP:
> - {
> - int len;
> -
> - GetWindowTextW(hwnd, hwndText, ARRAY_SIZE(hwndText));
> -
> + case WM_KEYDOWN:
> switch(wParam) {
> case VK_RETURN:
> /* If quickComplete is set and control is pressed, replace the string */
> - control = GetKeyState(VK_CONTROL) & 0x8000;
> - if (control && This->quickComplete) {
> + if (This->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000)) {
> WCHAR *qc;
> - size_t sz = strlenW(This->quickComplete) + 1;
> - sz += max(strlenW(hwndText), 2); /* %s is 2 chars */
> + size_t sz;
> + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0) + 1; /* include NUL */
> + if (!(hwndText = heap_alloc(len * sizeof(WCHAR))))
> + return 0;
> + len = SendMessageW(hwnd, WM_GETTEXT, len, (LPARAM)hwndText);
>
> + sz = strlenW(This->quickComplete)+1 + max(len, 2); /* %s is 2 chars */
> while ((qc = heap_alloc(sz * sizeof(WCHAR)))) {
> /* Pass it 3 times to guard against crap like %*.*s, where the
> width and precision are taken from extra args preceeding it,
> @@ -158,31 +158,35 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> break;
> sz *= 2;
> }
> + if (This->options & ACO_AUTOSUGGEST)
> + ShowWindow(This->hwndListBox, SW_HIDE);
> + heap_free(hwndText);
> + return 0;
> }
>
> - ShowWindow(This->hwndListBox, SW_HIDE);
> - return 0;
> - case VK_LEFT:
> - case VK_RIGHT:
> - return 0;
> + if (This->options & ACO_AUTOSUGGEST)
> + ShowWindow(This->hwndListBox, SW_HIDE);
> + break;
> case VK_UP:
> case VK_DOWN:
> - /* Two cases here :
> - - if the listbox is not visible, displays it
> - with all the entries if the style ACO_UPDOWNKEYDROPSLIST
> - is present but does not select anything.
> + /* Two cases here:
> + - if the listbox is not visible and ACO_UPDOWNKEYDROPSLIST is
> + set, display it with all the entries, without selecting any
> - if the listbox is visible, change the selection
> */
> - if ( (This->options & (ACO_AUTOSUGGEST | ACO_UPDOWNKEYDROPSLIST))
> - && (!IsWindowVisible(This->hwndListBox) && (! *hwndText)) )
> - {
> - /* We must display all the entries */
> - displayall = TRUE;
> - } else {
> - if (IsWindowVisible(This->hwndListBox)) {
> - int count;
> + if (This->options & ACO_AUTOSUGGEST) {
> + if (!IsWindowVisible(This->hwndListBox)) {
> + if (This->options & ACO_UPDOWNKEYDROPSLIST) {
> + ret = 0;
> + displayall = TRUE;
> + noautoappend = TRUE;
> + goto autocomplete_text;
> + }
> + } else {
> + INT count;
>
> count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0);
> +
> /* Change the selection */
> sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0);
> if (wParam == VK_UP)
> @@ -206,34 +210,51 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> len = strlenW(This->txtbackup);
> SendMessageW(hwnd, EM_SETSEL, len, len);
> }
> + return 0;
> }
> - return 0;
> }
> break;
> - case VK_BACK:
> case VK_DELETE:
> - if ((! *hwndText) && (This->options & ACO_AUTOSUGGEST)) {
> - ShowWindow(This->hwndListBox, SW_HIDE);
> - return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
> - }
> - break;
> - default:
> - ;
> + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
> + goto handle_control_char;
> + }
> + return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
> + case WM_CHAR:
> + case WM_UNICHAR:
> + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
> + if (wParam < ' ')
> + {
> + handle_control_char:
> + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0);
> + if ((This->options & ACO_AUTOSUGGEST) && len == 0) {
> + ShowWindow(This->hwndListBox, SW_HIDE);
> + return ret;
> + }
> + if (wParam != 0x16 /* ^V (paste) */)
> + noautoappend = TRUE;
> }
> + else {
> + autocomplete_text:
> + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0);
> + }
> +
> + old_len = len+1; /* include NUL */
> + if (!(hwndText = heap_alloc(old_len * sizeof(WCHAR))))
> + return ret;
> + len = SendMessageW(hwnd, WM_GETTEXT, old_len, (LPARAM)hwndText);
> + if (len+1 != old_len)
> + hwndText = heap_realloc(hwndText, len+1);
>
> SendMessageW(This->hwndListBox, LB_RESETCONTENT, 0, 0);
>
> + /* Set txtbackup to point to hwndText itself (which must not be released) */
> heap_free(This->txtbackup);
> - len = strlenW(hwndText);
> - This->txtbackup = heap_alloc((len + 1)*sizeof(WCHAR));
> - lstrcpyW(This->txtbackup, hwndText);
> + This->txtbackup = hwndText;
>
> - /* Returns if there is no text to search and we doesn't want to display all the entries */
> - if ((!displayall) && (! *hwndText) )
> - break;
> + if (!displayall && !len)
> + return ret;
>
> IEnumString_Reset(This->enumstr);
> - filled = FALSE;
> for(cpt = 0;;) {
> LPOLESTR strs = NULL;
> ULONG fetched;
> @@ -243,62 +264,68 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> break;
>
> if (!strncmpiW(hwndText, strs, len)) {
> - if (!filled && (This->options & ACO_AUTOAPPEND) &&
> - wParam != VK_BACK && wParam != VK_DELETE)
> + if (cpt == 0 && noautoappend == FALSE)
> {
> - WCHAR buffW[255];
> + /* The character capitalization can be different,
> + so merge hwndText and strs into a new string */
> + WCHAR *tmp;
> + size_t strslen = len + strlenW(&strs[len]);
> +
> + if ((tmp = heap_alloc((strslen+1) * sizeof(WCHAR))))
> + {
> + memcpy(tmp, hwndText, len * sizeof(WCHAR));
> + memcpy(&tmp[len], &strs[len], (strslen-len+1) * sizeof(WCHAR));
> + }
> + else tmp = strs;
> +
> + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)tmp);
> + SendMessageW(hwnd, EM_SETSEL, len, strslen);
> + if (tmp != strs)
> + heap_free(tmp);
>
> - strcpyW(buffW, hwndText);
> - strcatW(buffW, &strs[len]);
> - SetWindowTextW(hwnd, buffW);
> - SendMessageW(hwnd, EM_SETSEL, len, strlenW(strs));
> if (!(This->options & ACO_AUTOSUGGEST)) {
> CoTaskMemFree(strs);
> break;
> }
> }
>
> - if (This->options & ACO_AUTOSUGGEST) {
> - SendMessageW(This->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs);
> - cpt++;
> - }
> + if (This->options & ACO_AUTOSUGGEST)
> + SendMessageW(This->hwndListBox, LB_INSERTSTRING, -1, (LPARAM)strs);
>
> - filled = TRUE;
> + cpt++;
> }
>
> CoTaskMemFree(strs);
> }
>
> if (This->options & ACO_AUTOSUGGEST) {
> - if (filled) {
> - height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0);
> + if (cpt) {
> + RECT r;
> + UINT height = SendMessageW(This->hwndListBox, LB_GETITEMHEIGHT, 0, 0);
> SendMessageW(This->hwndListBox, LB_CARETOFF, 0, 0);
> GetWindowRect(hwnd, &r);
> SetParent(This->hwndListBox, HWND_DESKTOP);
> /* It seems that Windows XP displays 7 lines at most
> and otherwise displays a vertical scroll bar */
> SetWindowPos(This->hwndListBox, HWND_TOP,
> - r.left, r.bottom + 1, r.right - r.left, min(height * 7, height*(cpt+1)),
> + r.left, r.bottom + 1, r.right - r.left, height*min(cpt+1, 7),
> SWP_SHOWWINDOW );
> } else {
> ShowWindow(This->hwndListBox, SW_HIDE);
> }
> }
> -
> - break;
> - }
> + return ret;
> case WM_DESTROY:
> {
> WNDPROC proc = This->wpOrigEditProc;
>
> - RemovePropW(hwnd, autocomplete_propertyW);
> SetWindowLongPtrW(hwnd, GWLP_WNDPROC, (LONG_PTR)proc);
> + RemovePropW(hwnd, autocomplete_propertyW);
> destroy_autocomplete_object(This);
> return CallWindowProcW(proc, hwnd, uMsg, wParam, lParam);
> }
> default:
> return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam);
> -
> }
>
> return 0;
> @@ -351,6 +378,8 @@ static void create_listbox(IAutoCompleteImpl *This)
> This->wpOrigLBoxProc = (WNDPROC) SetWindowLongPtrW( This->hwndListBox, GWLP_WNDPROC, (LONG_PTR) ACLBoxSubclassProc);
> SetWindowLongPtrW( This->hwndListBox, GWLP_USERDATA, (LONG_PTR)This);
> }
> + else
> + This->options &= ~ACO_AUTOSUGGEST;
> }
>
> /**************************************************************************
> @@ -470,6 +499,10 @@ static HRESULT WINAPI IAutoComplete2_fnInit(
> return This->hwndEdit ? E_FAIL : E_UNEXPECTED;
> }
>
> + /* Prevent txtbackup from ever being NULL to simplify the code */
> + if ((This->txtbackup = heap_alloc_zero(sizeof(WCHAR))) == NULL)
> + return E_OUTOFMEMORY;
> +
> if (FAILED (IUnknown_QueryInterface (punkACL, &IID_IEnumString, (LPVOID*)&This->enumstr))) {
> WARN("No IEnumString interface\n");
> return E_NOINTERFACE;
> --
> 1.9.1
>
>
>
More information about the wine-devel
mailing list