[PATCH v2 0/2] MR1270: comctl32/edit: Check for control destruction on notification return.
Based on a patch by Mark Jansen. Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> -- v2: user32/edit: Check for control destruction on notification return. comctl32/edit: Check for control destruction on notification return. https://gitlab.winehq.org/wine/wine/-/merge_requests/1270
From: Nikolay Sivov <nsivov(a)codeweavers.com> Based on a patch by Mark Jansen. Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/comctl32/edit.c | 55 ++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c index 3111cda2a4b..a9a469ffd47 100644 --- a/dlls/comctl32/edit.c +++ b/dlls/comctl32/edit.c @@ -154,15 +154,13 @@ typedef struct #define SWAP_UINT32(x,y) do { UINT temp = (UINT)(x); (x) = (UINT)(y); (y) = temp; } while(0) #define ORDER_UINT(x,y) do { if ((UINT)(y) < (UINT)(x)) SWAP_UINT32((x),(y)); } while(0) -/* used for disabled or read-only edit control */ -#define EDIT_NOTIFY_PARENT(es, wNotifyCode) \ - do \ - { /* Notify parent which has created this edit control */ \ - TRACE("notification " #wNotifyCode " sent to hwnd=%p\n", es->hwndParent); \ - SendMessageW(es->hwndParent, WM_COMMAND, \ - MAKEWPARAM(GetWindowLongPtrW((es->hwndSelf),GWLP_ID), wNotifyCode), \ - (LPARAM)(es->hwndSelf)); \ - } while(0) +static inline BOOL notify_parent(const EDITSTATE *es, INT code) +{ + HWND hwnd = es->hwndSelf; + TRACE("notification %d sent to %p.\n", code, es->hwndParent); + SendMessageW(es->hwndParent, WM_COMMAND, MAKEWPARAM(GetWindowLongPtrW(es->hwndSelf, GWLP_ID), code), (LPARAM)es->hwndSelf); + return IsWindow(hwnd); +} static LRESULT EDIT_EM_PosFromChar(EDITSTATE *es, INT index, BOOL after_wrap); @@ -1275,7 +1273,7 @@ static BOOL EDIT_MakeFit(EDITSTATE *es, UINT size) if (es->buffer_size < size) { WARN("FAILED ! We now have %d+1\n", es->buffer_size); - EDIT_NOTIFY_PARENT(es, EN_ERRSPACE); + notify_parent(es, EN_ERRSPACE); return FALSE; } else { TRACE("We now have %d+1\n", es->buffer_size); @@ -1322,7 +1320,7 @@ static void EDIT_UpdateTextRegion(EDITSTATE *es, HRGN hrgn, BOOL bErase) { if (es->flags & EF_UPDATE) { es->flags &= ~EF_UPDATE; - EDIT_NOTIFY_PARENT(es, EN_UPDATE); + if (!notify_parent(es, EN_UPDATE)) return; } InvalidateRgn(es->hwndSelf, hrgn, bErase); } @@ -1337,7 +1335,7 @@ static void EDIT_UpdateText(EDITSTATE *es, const RECT *rc, BOOL bErase) { if (es->flags & EF_UPDATE) { es->flags &= ~EF_UPDATE; - EDIT_NOTIFY_PARENT(es, EN_UPDATE); + if (!notify_parent(es, EN_UPDATE)) return; } InvalidateRect(es->hwndSelf, rc, bErase); } @@ -1622,9 +1620,9 @@ static BOOL EDIT_EM_LineScroll_internal(EDITSTATE *es, INT dx, INT dy) EDIT_UpdateScrollInfo(es); } if (dx && !(es->flags & EF_HSCROLL_TRACK)) - EDIT_NOTIFY_PARENT(es, EN_HSCROLL); + notify_parent(es, EN_HSCROLL); if (dy && !(es->flags & EF_VSCROLL_TRACK)) - EDIT_NOTIFY_PARENT(es, EN_VSCROLL); + notify_parent(es, EN_VSCROLL); return TRUE; } @@ -2456,8 +2454,9 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r /* Issue the EN_MAXTEXT notification and continue with replacing text * so that buffer limit is honored. */ - if ((honor_limit) && (size > es->buffer_limit)) { - EDIT_NOTIFY_PARENT(es, EN_MAXTEXT); + if ((honor_limit) && (size > es->buffer_limit)) + { + if (!notify_parent(es, EN_MAXTEXT)) return; /* Buffer limit can be smaller than the actual length of text in combobox */ if (es->buffer_limit < (tl - (e-s))) strl = 0; @@ -2515,7 +2514,7 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r strl = 0; e = s; SetRectRgn(hrgn, 0, 0, 0, 0); - EDIT_NOTIFY_PARENT(es, EN_MAXTEXT); + if (!notify_parent(es, EN_MAXTEXT)) return; } } else { @@ -2532,7 +2531,7 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r EDIT_CalcLineWidth_SL(es); } text_buffer_changed(es); - EDIT_NOTIFY_PARENT(es, EN_MAXTEXT); + if (!notify_parent(es, EN_MAXTEXT)) return; } } @@ -2623,7 +2622,7 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r if(send_update || (es->flags & EF_UPDATE)) { es->flags &= ~EF_UPDATE; - EDIT_NOTIFY_PARENT(es, EN_CHANGE); + if (!notify_parent(es, EN_CHANGE)) return; } EDIT_InvalidateUniscribeData(es); } @@ -2891,7 +2890,7 @@ static BOOL EDIT_EM_Undo(EDITSTATE *es) EDIT_EM_ReplaceSel(es, TRUE, utext, ulength, TRUE, TRUE); EDIT_EM_SetSel(es, es->undo_position, es->undo_position + es->undo_insert_count, FALSE); /* send the notification after the selection start and end are set */ - EDIT_NOTIFY_PARENT(es, EN_CHANGE); + if (!notify_parent(es, EN_CHANGE)) return TRUE; EDIT_EM_ScrollCaret(es); heap_free(utext); @@ -3385,8 +3384,8 @@ static LRESULT EDIT_WM_KeyDown(EDITSTATE *es, INT key) { if (EDIT_EM_SetSel(es, 0, get_text_length(es), FALSE)) { - EDIT_NOTIFY_PARENT(es, EN_UPDATE); - EDIT_NOTIFY_PARENT(es, EN_CHANGE); + if (!notify_parent(es, EN_UPDATE)) break; + notify_parent(es, EN_CHANGE); } } break; @@ -3408,7 +3407,7 @@ static LRESULT EDIT_WM_KillFocus(HTHEME theme, EDITSTATE *es) DestroyCaret(); if (!(es->style & ES_NOHIDESEL)) EDIT_InvalidateText(es, es->selection_start, es->selection_end); - EDIT_NOTIFY_PARENT(es, EN_KILLFOCUS); + if (!notify_parent(es, EN_KILLFOCUS)) return 0; /* Throw away left over scroll when we lose focus */ es->wheelDeltaRemainder = 0; @@ -3694,7 +3693,7 @@ static void EDIT_WM_SetFocus(HTHEME theme, EDITSTATE *es) CreateCaret(es->hwndSelf, 0, 1, es->line_height); EDIT_SetCaretPos(es, es->selection_end, es->flags & EF_AFTER_WRAP); ShowCaret(es->hwndSelf); - EDIT_NOTIFY_PARENT(es, EN_SETFOCUS); + if (!notify_parent(es, EN_SETFOCUS)) return; if (theme) flags |= RDW_FRAME | RDW_ERASE; @@ -3820,8 +3819,8 @@ static void EDIT_WM_SetText(EDITSTATE *es, LPCWSTR text) */ if( !((es->style & ES_MULTILINE) || es->hwndListBox)) { - EDIT_NOTIFY_PARENT(es, EN_UPDATE); - EDIT_NOTIFY_PARENT(es, EN_CHANGE); + if (!notify_parent(es, EN_UPDATE)) return; + if (!notify_parent(es, EN_CHANGE)) return; } EDIT_EM_ScrollCaret(es); EDIT_UpdateScrollInfo(es); @@ -4022,7 +4021,7 @@ static LRESULT EDIT_WM_HScroll(EDITSTATE *es, INT action, INT pos) if (!dx) { /* force scroll info update */ EDIT_UpdateScrollInfo(es); - EDIT_NOTIFY_PARENT(es, EN_HSCROLL); + notify_parent(es, EN_HSCROLL); } break; case SB_ENDSCROLL: @@ -4145,7 +4144,7 @@ static LRESULT EDIT_WM_VScroll(EDITSTATE *es, INT action, INT pos) { /* force scroll info update */ EDIT_UpdateScrollInfo(es); - EDIT_NOTIFY_PARENT(es, EN_VSCROLL); + notify_parent(es, EN_VSCROLL); } break; case SB_ENDSCROLL: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1270
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/user32/edit.c | 51 +++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/dlls/user32/edit.c b/dlls/user32/edit.c index e48d63f2870..d950cb89889 100644 --- a/dlls/user32/edit.c +++ b/dlls/user32/edit.c @@ -141,15 +141,13 @@ typedef struct #define SWAP_UINT32(x,y) do { UINT temp = (UINT)(x); (x) = (UINT)(y); (y) = temp; } while(0) #define ORDER_UINT(x,y) do { if ((UINT)(y) < (UINT)(x)) SWAP_UINT32((x),(y)); } while(0) -/* used for disabled or read-only edit control */ -#define EDIT_NOTIFY_PARENT(es, wNotifyCode) \ - do \ - { /* Notify parent which has created this edit control */ \ - TRACE("notification " #wNotifyCode " sent to hwnd=%p\n", es->hwndParent); \ - SendMessageW(es->hwndParent, WM_COMMAND, \ - MAKEWPARAM(GetWindowLongPtrW((es->hwndSelf),GWLP_ID), wNotifyCode), \ - (LPARAM)(es->hwndSelf)); \ - } while(0) +static inline BOOL notify_parent(const EDITSTATE *es, INT code) +{ + HWND hwnd = es->hwndSelf; + TRACE("notification %d sent to %p.\n", code, es->hwndParent); + SendMessageW(es->hwndParent, WM_COMMAND, MAKEWPARAM(GetWindowLongPtrW(es->hwndSelf, GWLP_ID), code), (LPARAM)es->hwndSelf); + return IsWindow(hwnd); +} static LRESULT EDIT_EM_PosFromChar(EDITSTATE *es, INT index, BOOL after_wrap); @@ -1364,7 +1362,7 @@ static BOOL EDIT_MakeFit(EDITSTATE *es, UINT size) if (es->buffer_size < size) { WARN("FAILED ! We now have %d+1\n", es->buffer_size); - EDIT_NOTIFY_PARENT(es, EN_ERRSPACE); + notify_parent(es, EN_ERRSPACE); return FALSE; } else { TRACE("We now have %d+1\n", es->buffer_size); @@ -1411,7 +1409,7 @@ static void EDIT_UpdateTextRegion(EDITSTATE *es, HRGN hrgn, BOOL bErase) { if (es->flags & EF_UPDATE) { es->flags &= ~EF_UPDATE; - EDIT_NOTIFY_PARENT(es, EN_UPDATE); + if (!notify_parent(es, EN_UPDATE)) return; } NtUserInvalidateRgn(es->hwndSelf, hrgn, bErase); } @@ -1426,7 +1424,7 @@ static void EDIT_UpdateText(EDITSTATE *es, const RECT *rc, BOOL bErase) { if (es->flags & EF_UPDATE) { es->flags &= ~EF_UPDATE; - EDIT_NOTIFY_PARENT(es, EN_UPDATE); + if (!notify_parent(es, EN_UPDATE)) return; } NtUserInvalidateRect(es->hwndSelf, rc, bErase); } @@ -1706,9 +1704,9 @@ static BOOL EDIT_EM_LineScroll_internal(EDITSTATE *es, INT dx, INT dy) EDIT_UpdateScrollInfo(es); } if (dx && !(es->flags & EF_HSCROLL_TRACK)) - EDIT_NOTIFY_PARENT(es, EN_HSCROLL); + notify_parent(es, EN_HSCROLL); if (dy && !(es->flags & EF_VSCROLL_TRACK)) - EDIT_NOTIFY_PARENT(es, EN_VSCROLL); + notify_parent(es, EN_VSCROLL); return TRUE; } @@ -2567,8 +2565,9 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r /* Issue the EN_MAXTEXT notification and continue with replacing text * so that buffer limit is honored. */ - if ((honor_limit) && (size > es->buffer_limit)) { - EDIT_NOTIFY_PARENT(es, EN_MAXTEXT); + if ((honor_limit) && (size > es->buffer_limit)) + { + if (!notify_parent(es, EN_MAXTEXT)) return; /* Buffer limit can be smaller than the actual length of text in combobox */ if (es->buffer_limit < (tl - (e-s))) strl = 0; @@ -2626,7 +2625,7 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r strl = 0; e = s; SetRectRgn(hrgn, 0, 0, 0, 0); - EDIT_NOTIFY_PARENT(es, EN_MAXTEXT); + if (!notify_parent(es, EN_MAXTEXT)) return; } } else { @@ -2643,7 +2642,7 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r EDIT_CalcLineWidth_SL(es); } text_buffer_changed(es); - EDIT_NOTIFY_PARENT(es, EN_MAXTEXT); + if (!notify_parent(es, EN_MAXTEXT)) return; } } @@ -2734,7 +2733,7 @@ static void EDIT_EM_ReplaceSel(EDITSTATE *es, BOOL can_undo, const WCHAR *lpsz_r if(send_update || (es->flags & EF_UPDATE)) { es->flags &= ~EF_UPDATE; - EDIT_NOTIFY_PARENT(es, EN_CHANGE); + if (!notify_parent(es, EN_CHANGE)) return; } EDIT_InvalidateUniscribeData(es); } @@ -3044,7 +3043,7 @@ static BOOL EDIT_EM_Undo(EDITSTATE *es) EDIT_EM_ReplaceSel(es, TRUE, utext, ulength, TRUE, TRUE); EDIT_EM_SetSel(es, es->undo_position, es->undo_position + es->undo_insert_count, FALSE); /* send the notification after the selection start and end are set */ - EDIT_NOTIFY_PARENT(es, EN_CHANGE); + if (!notify_parent(es, EN_CHANGE)) return TRUE; EDIT_EM_ScrollCaret(es); HeapFree(GetProcessHeap(), 0, utext); @@ -3565,7 +3564,7 @@ static LRESULT EDIT_WM_KillFocus(EDITSTATE *es) DestroyCaret(); if(!(es->style & ES_NOHIDESEL)) EDIT_InvalidateText(es, es->selection_start, es->selection_end); - EDIT_NOTIFY_PARENT(es, EN_KILLFOCUS); + if (!notify_parent(es, EN_KILLFOCUS)) return 0; /* throw away left over scroll when we lose focus */ es->wheelDeltaRemainder = 0; return 0; @@ -3808,7 +3807,7 @@ static void EDIT_WM_SetFocus(EDITSTATE *es) EDIT_SetCaretPos(es, es->selection_end, es->flags & EF_AFTER_WRAP); NtUserShowCaret( es->hwndSelf ); - EDIT_NOTIFY_PARENT(es, EN_SETFOCUS); + notify_parent(es, EN_SETFOCUS); } static DWORD get_font_margins(HDC hdc, const TEXTMETRICW *tm, BOOL unicode) @@ -3948,8 +3947,8 @@ static void EDIT_WM_SetText(EDITSTATE *es, LPCWSTR text, BOOL unicode) */ if( !((es->style & ES_MULTILINE) || es->hwndListBox)) { - EDIT_NOTIFY_PARENT(es, EN_UPDATE); - EDIT_NOTIFY_PARENT(es, EN_CHANGE); + if (!notify_parent(es, EN_UPDATE)) return; + if (!notify_parent(es, EN_CHANGE)) return; } EDIT_EM_ScrollCaret(es); EDIT_UpdateScrollInfo(es); @@ -4150,7 +4149,7 @@ static LRESULT EDIT_WM_HScroll(EDITSTATE *es, INT action, INT pos) if (!dx) { /* force scroll info update */ EDIT_UpdateScrollInfo(es); - EDIT_NOTIFY_PARENT(es, EN_HSCROLL); + notify_parent(es, EN_HSCROLL); } break; case SB_ENDSCROLL: @@ -4273,7 +4272,7 @@ static LRESULT EDIT_WM_VScroll(EDITSTATE *es, INT action, INT pos) { /* force scroll info update */ EDIT_UpdateScrollInfo(es); - EDIT_NOTIFY_PARENT(es, EN_VSCROLL); + notify_parent(es, EN_VSCROLL); } break; case SB_ENDSCROLL: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1270
Mark Jansen (@learn-more) commented about dlls/comctl32/edit.c:
EDIT_UpdateScrollInfo(es); } if (dx && !(es->flags & EF_HSCROLL_TRACK)) - EDIT_NOTIFY_PARENT(es, EN_HSCROLL); + notify_parent(es, EN_HSCROLL); if (dy && !(es->flags & EF_VSCROLL_TRACK))
Here you are reading `es` after returning from a notification, should this be guarded? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1270#note_13373
Mark Jansen (@learn-more) commented about dlls/user32/edit.c:
if (!dx) { /* force scroll info update */ EDIT_UpdateScrollInfo(es); - EDIT_NOTIFY_PARENT(es, EN_HSCROLL); + notify_parent(es, EN_HSCROLL);
The code below only uses `es` under the guard of `if (dx)`, but when code below is edited this case most likely won't be updated, so it would be safer to bail out here as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1270#note_13374
On Mon Nov 7 09:46:01 2022 +0000, Mark Jansen wrote:
Here you are reading `es` after returning from a notification, should this be guarded? Yes, I was going to send another separate fixup later, once reported crash is gone.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1270#note_13375
participants (3)
-
Mark Jansen (＠learn-more) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)