[PATCH v3 01/10] shell32/autocomplete: Fix a vulnerability by avoiding the use of snprintf
The quickComplete format can have more than one % argument, or stuff like %*.* or %1234s, which can be exploited since the format string can be read from the registry, so handle it manually instead of using sprintf. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Supersedes series starting with 150711. v3: Use a helper function and handle %% and also split it to multiple patches and fix the embarrassing mistake. And for the entire patch series, I've moved the "txtbackup never NULL" patch to the IACList::Expand patch which will come later (because then it will hopefully make more sense!). Lastly, please see comments on patch 7/10 in the series for more information, hopefully that's okay now. dlls/shell32/autocomplete.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 4ec7387..39df45e 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -93,6 +93,34 @@ static inline IAutoCompleteImpl *impl_from_IAutoCompleteDropDown(IAutoCompleteDr return CONTAINING_RECORD(iface, IAutoCompleteImpl, IAutoCompleteDropDown_iface); } +static void format_quick_complete(WCHAR *dst, const WCHAR *qc, const WCHAR *str, size_t str_len) +{ + /* Replace the first %s directly without using snprintf, to avoid + exploits since the format string can be retrieved from the registry */ + while (*qc != '\0') + { + if (qc[0] == '%') + { + if (qc[1] == 's') + { + memcpy(dst, str, str_len * sizeof(WCHAR)); + dst += str_len; + qc += 2; + while (*qc != '\0') + { + if (qc[0] == '%' && qc[1] == '%') + qc++; + *dst++ = *qc++; + } + break; + } + qc += (qc[1] == '%'); + } + *dst++ = *qc++; + } + *dst = '\0'; +} + static void destroy_autocomplete_object(IAutoCompleteImpl *ac) { ac->hwndEdit = NULL; @@ -109,7 +137,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 +165,16 @@ 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 + len; + if ((buf = heap_alloc(sz * sizeof(WCHAR)))) + { + format_quick_complete(buf, This->quickComplete, hwndText, len); + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)buf); + SendMessageW(hwnd, EM_SETSEL, 0, sz-1); + heap_free(buf); + } } ShowWindow(This->hwndListBox, SW_HIDE); -- 1.9.1
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 39df45e..2f9d16f 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -209,10 +209,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)); @@ -319,7 +319,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: @@ -331,7 +332,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
On Sat, Sep 08, 2018 at 02:50:48PM +0300, Gabriel Ivăncescu wrote:
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 39df45e..2f9d16f 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -209,10 +209,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);
Changing the type of len and making moving the declaration into an assignment don't belong in this patch. What you really want here is a check against LB_ERR, but that again is a different patch.
- 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)); @@ -319,7 +319,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;
Same goes for these changes.
switch (uMsg) { case WM_MOUSEMOVE: @@ -331,7 +332,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
On Mon, Sep 10, 2018 at 11:40 AM, Huw Davies <huw(a)codeweavers.com> wrote:
Changing the type of len and making moving the declaration into an assignment don't belong in this patch. What you really want here is a check against LB_ERR, but that again is a different patch.
Oh I thought they were way too trivial changes to make in a separate patch, sorry. Though, indeed I'll add a check for LB_ERR in an extra patch then. BTW (unrelated to this patch), I know you're reviewing the larger patches at the end of the patchset and want the function split into helper functions. I did a bit more this weekend and I want to emphasize again that the WM_CHAR will have to be rewritten at some point, so I think that it is the perfect opportunity to do it later rather than now. So IMO, if there aren't other issues, for now let's just try to get these committed as they are, so we can move to round 2 (and round 3 later, when the move to helper functions and the rewrite will happen). I mean, I already have those patches written, since most of the WM_CHAR part (especially string enumeration) will have to be changed. But those patches will come after IACList::Expand (so probably on round 3, assuming this is round 1) when I will implement ResetEnumerator and the cached sorted string list (two of the TODO at the top of the file). I'd prefer if we do it then also because it would make around 30+ patches easier to rebase for me, sitting on way too many right now. :-)
On Mon, Sep 10, 2018 at 2:54 PM, Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
On Mon, Sep 10, 2018 at 11:40 AM, Huw Davies <huw(a)codeweavers.com> wrote:
Changing the type of len and making moving the declaration into an assignment don't belong in this patch. What you really want here is a check against LB_ERR, but that again is a different patch.
Oh I thought they were way too trivial changes to make in a separate patch, sorry. Though, indeed I'll add a check for LB_ERR in an extra patch then.
On second thought, it looks like all the LB_GETTEXTLEN are done after LB_GETCURSEL, so they should never be able to fail (an invalid selection is already checked before that). Should I still check for LB_ERR? Or just send an extra patch that converts int to UINT for len without the checks?
On Mon, Sep 10, 2018 at 05:29:51PM +0300, Gabriel Ivăncescu wrote:
On Mon, Sep 10, 2018 at 2:54 PM, Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
On Mon, Sep 10, 2018 at 11:40 AM, Huw Davies <huw(a)codeweavers.com> wrote:
Changing the type of len and making moving the declaration into an assignment don't belong in this patch. What you really want here is a check against LB_ERR, but that again is a different patch.
Oh I thought they were way too trivial changes to make in a separate patch, sorry. Though, indeed I'll add a check for LB_ERR in an extra patch then.
On second thought, it looks like all the LB_GETTEXTLEN are done after LB_GETCURSEL, so they should never be able to fail (an invalid selection is already checked before that).
Should I still check for LB_ERR? Or just send an extra patch that converts int to UINT for len without the checks?
Just leave them as ints. Huw.
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 2f9d16f..7319d84 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -213,13 +213,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; @@ -334,9 +336,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
On Sat, Sep 08, 2018 at 02:50:49PM +0300, Gabriel Ivăncescu wrote:
We can retrieve the length of the string from the SendMessage calls already.
Looks good.
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 2f9d16f..7319d84 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -213,13 +213,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; @@ -334,9 +336,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
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 7319d84..93be8e3 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -146,7 +146,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)) @@ -177,7 +178,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: @@ -311,7 +313,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, } default: return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - } return 0; @@ -364,6 +365,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
On Sat, Sep 08, 2018 at 02:50:50PM +0300, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
Looks good.
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 7319d84..93be8e3 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -146,7 +146,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)) @@ -177,7 +178,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: @@ -311,7 +313,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, } default: return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); - }
return 0; @@ -364,6 +365,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
There's no need to have filled, since cpt can already 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 93be8e3..3fed020 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -138,7 +138,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); @@ -164,8 +164,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 + len; @@ -250,7 +250,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; @@ -260,7 +259,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); @@ -273,19 +272,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
On Sat, Sep 08, 2018 at 02:50:51PM +0300, Gabriel Ivăncescu wrote:
There's no need to have filled, since cpt can already provide the same information.
Looks good.
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 93be8e3..3fed020 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -138,7 +138,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); @@ -164,8 +164,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 + len; @@ -250,7 +250,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; @@ -260,7 +259,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); @@ -273,19 +272,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
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v3: Split from the next patch in the series, even if some of the added code will be removed, hopefully that's ok. dlls/shell32/autocomplete.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 3fed020..5038bb5 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -136,10 +136,11 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW); HRESULT hr; - WCHAR hwndText[255]; + WCHAR *hwndText; + UINT len, size, cpt; RECT r; BOOL displayall = FALSE; - int cpt, height, sel; + int height, sel; if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); @@ -156,10 +157,11 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, } return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); case WM_KEYUP: - { - int len; - - GetWindowTextW(hwnd, hwndText, ARRAY_SIZE(hwndText)); + len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + size = len+1; + if (!(hwndText = heap_alloc(size * sizeof(WCHAR)))) + return 0; + len = SendMessageW(hwnd, WM_GETTEXT, size, (LPARAM)hwndText); switch(wParam) { case VK_RETURN: @@ -167,7 +169,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (This->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000)) { WCHAR *buf; - size_t len = strlenW(hwndText); size_t sz = strlenW(This->quickComplete) + 1 + len; if ((buf = heap_alloc(sz * sizeof(WCHAR)))) { @@ -180,9 +181,11 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (This->options & ACO_AUTOSUGGEST) ShowWindow(This->hwndListBox, SW_HIDE); + heap_free(hwndText); return 0; case VK_LEFT: case VK_RIGHT: + heap_free(hwndText); return 0; case VK_UP: case VK_DOWN: @@ -198,6 +201,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, /* We must display all the entries */ displayall = TRUE; } else { + heap_free(hwndText); if (IsWindowVisible(This->hwndListBox)) { int count; @@ -232,21 +236,23 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, case VK_BACK: case VK_DELETE: if ((! *hwndText) && (This->options & ACO_AUTOSUGGEST)) { + heap_free(hwndText); ShowWindow(This->hwndListBox, SW_HIDE); return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); } break; } + 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) ) + if (!displayall && !len) break; IEnumString_Reset(This->enumstr); @@ -298,7 +304,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, } break; - } case WM_DESTROY: { WNDPROC proc = This->wpOrigEditProc; -- 1.9.1
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> --- v3: I've split as much as possible without a complete rewrite of the function. Most of the other patches in the series (that will follow) will still apply now with just a small rebase, so please try to consider it in this shape (i.e. without rewriting it completely with helper functions which would also make code sharing harder in this particular situation) since I have more patches pending that will fix AutoComplete for good. Of course, they will introduce some helper functions on their own without rewriting the function. The function can always be split into helper functions later, once all of these patch series are settled (and ones I haven't sent yet, including implementing ResetEnumerator), because it will already change over the course of the series and some parts might be reduced (like the auto-append) depending if they are accepted. IMO it's better to wait and see how it shapes up first before refactoring it, if that's truly needed. Besides, it's not worse than the current code mess, and honestly I wouldn't want the current code to still be there for another Wine release, it's just too broken and unusable for anything that relies on AutoComplete (it's actually frustrating for users, even releasing the Shift key is annoying). dlls/shell32/autocomplete.c | 97 +++++++++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 39 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 5038bb5..c46b897 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -136,10 +136,11 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW); HRESULT hr; + LRESULT ret; WCHAR *hwndText; UINT len, size, cpt; RECT r; - BOOL displayall = FALSE; + BOOLEAN displayall = FALSE, noautoappend = !(This->options & ACO_AUTOAPPEND); int height, sel; if (!This->enabled) return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); @@ -156,20 +157,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: - len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); - size = len+1; - if (!(hwndText = heap_alloc(size * sizeof(WCHAR)))) - return 0; - len = SendMessageW(hwnd, WM_GETTEXT, size, (LPARAM)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 sz = strlenW(This->quickComplete) + 1 + len; + 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 + len; + if ((buf = heap_alloc(sz * sizeof(WCHAR)))) { format_quick_complete(buf, This->quickComplete, hwndText, len); @@ -177,32 +178,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); - heap_free(hwndText); - return 0; - case VK_LEFT: - case VK_RIGHT: - heap_free(hwndText); - 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 { - heap_free(hwndText); - 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 handle_char; + } + } else { int count; count = SendMessageW(This->hwndListBox, LB_GETCOUNT, 0, 0); @@ -229,20 +229,40 @@ 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)) { - heap_free(hwndText); - 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 + { + handle_char: + 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); @@ -253,7 +273,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, This->txtbackup = hwndText; if (!displayall && !len) - break; + return ret; IEnumString_Reset(This->enumstr); for(cpt = 0;;) { @@ -265,7 +285,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); @@ -302,8 +322,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
The previous code caps the auto-append text at 255 characters, which can be easily exploited. It's also less efficient as it scans the string multiple times. 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 c46b897..fc4c595 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -286,12 +286,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
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 fc4c595..77e6945 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -139,9 +139,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); @@ -203,9 +202,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, goto handle_char; } } 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) @@ -320,14 +318,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); @@ -338,8 +337,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
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
On Sat, Sep 08, 2018 at 02:50:47PM +0300, Gabriel Ivăncescu wrote:
The quickComplete format can have more than one % argument, or stuff like %*.* or %1234s, which can be exploited since the format string can be read from the registry, so handle it manually instead of using sprintf.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
Supersedes series starting with 150711.
v3: Use a helper function and handle %% and also split it to multiple patches and fix the embarrassing mistake. And for the entire patch series, I've moved the "txtbackup never NULL" patch to the IACList::Expand patch which will come later (because then it will hopefully make more sense!).
Lastly, please see comments on patch 7/10 in the series for more information, hopefully that's okay now.
dlls/shell32/autocomplete.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 4ec7387..39df45e 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -93,6 +93,34 @@ static inline IAutoCompleteImpl *impl_from_IAutoCompleteDropDown(IAutoCompleteDr return CONTAINING_RECORD(iface, IAutoCompleteImpl, IAutoCompleteDropDown_iface); }
+static void format_quick_complete(WCHAR *dst, const WCHAR *qc, const WCHAR *str, size_t str_len) +{ + /* Replace the first %s directly without using snprintf, to avoid + exploits since the format string can be retrieved from the registry */ + while (*qc != '\0') + { + if (qc[0] == '%') + { + if (qc[1] == 's') + { + memcpy(dst, str, str_len * sizeof(WCHAR)); + dst += str_len; + qc += 2; + while (*qc != '\0') + { + if (qc[0] == '%' && qc[1] == '%') + qc++; + *dst++ = *qc++; + }
This inner loop to process %% is ugly. Just do the processing of %s in this block. If you want to make sure you only do it once then set a flag.
+ break; + } + qc += (qc[1] == '%'); + } + *dst++ = *qc++; + } + *dst = '\0'; +} + static void destroy_autocomplete_object(IAutoCompleteImpl *ac) { ac->hwndEdit = NULL; @@ -109,7 +137,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 +165,16 @@ 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 + len; + if ((buf = heap_alloc(sz * sizeof(WCHAR)))) + { + format_quick_complete(buf, This->quickComplete, hwndText, len); + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)buf); + SendMessageW(hwnd, EM_SETSEL, 0, sz-1);
Again, let's put spaces around binary operators (here the minus op) in new code. This applies other patches in this series too. However it would be nicer to have format_quick_complete return the length so that you can call EM_SETSEL with the correct length rather than an over-estimate.
+ heap_free(buf); + } }
ShowWindow(This->hwndListBox, SW_HIDE); -- 1.9.1
On Mon, Sep 10, 2018 at 11:05 AM, Huw Davies <huw(a)codeweavers.com> wrote:
This inner loop to process %% is ugly. Just do the processing of %s in this block. If you want to make sure you only do it once then set a flag.
Alright, I honestly think it's ok since it's a short block, but I'll count the number of args then, I think it's a better approach than a flag (even though in this case it's the same thing since only one %s arg is allowed).
Again, let's put spaces around binary operators (here the minus op) in new code. This applies other patches in this series too.
However it would be nicer to have format_quick_complete return the length so that you can call EM_SETSEL with the correct length rather than an over-estimate.
Will do. Note that I might miss some spaces between operators especially in future patches (I have quite some patches lined up for AutoComplete locally right now) but I hope that's not such a big deal even if I miss the odd one, I'll try to put spaces where I find them though.
On Mon, Sep 10, 2018 at 02:48:02PM +0300, Gabriel Ivăncescu wrote:
On Mon, Sep 10, 2018 at 11:05 AM, Huw Davies <huw(a)codeweavers.com> wrote:
This inner loop to process %% is ugly. Just do the processing of %s in this block. If you want to make sure you only do it once then set a flag.
Alright, I honestly think it's ok since it's a short block, but I'll count the number of args then, I think it's a better approach than a flag (even though in this case it's the same thing since only one %s arg is allowed).
It took me significantly longer to figure out what was going on than it should have done. What you should be trying to do is to keep the code as simple as possible so that the reviewer doesn't have to figure these things out. A count instead of a flag will be fine if prefer that. Huw.
participants (2)
-
Gabriel Ivăncescu -
Huw Davies