[PATCH v3 0/5] MR1554: win32u: Reset focused item when removing menu item
Fixes an out of bounds access in `show_popup`. ```c if (menu->FocusedItem != NO_SELECTED_ITEM) { menu->items[menu->FocusedItem].fState &= ~(MF_HILITE|MF_MOUSESELECT); // <- can crash here menu->FocusedItem = NO_SELECTED_ITEM; } ``` Menu resets focused item on show, not on close. If items were later deleted, next time user opens the menu, menu can crash on out of bounds access and won't show up again, as menu thinks it's still on screen. In other words: - open split button dropdown - click on any item - clear dropdown items - open dropdown again Menu borked and won't open again. Source for the testing app for reproduction. [test.c](/uploads/2f703f63891b33e1a0eb0fcd27c912b7/test.c) I guess, the alternative is to reset focused item when menu is closed, but not sure where's the best place to do that, haven't dug deep enough. -- v3: win32u: Update focused item when inserting a menu item win32u: Reset focused item if it was removed when removing a menu item https://gitlab.winehq.org/wine/wine/-/merge_requests/1554
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/user32/tests/menu.c | 3 --- dlls/win32u/menu.c | 6 ++++++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c index 6862f13c6b1..b00c403940f 100644 --- a/dlls/user32/tests/menu.c +++ b/dlls/user32/tests/menu.c @@ -2531,11 +2531,8 @@ static void test_menu_hilitemenuitem( void ) "HiliteMenuItem: Item 2 is hilited\n"); SetLastError(0xdeadbeef); - todo_wine - { ok(!HiliteMenuItem(NULL, hPopupMenu, 1, MF_HILITE | MF_BYPOSITION), "HiliteMenuItem: call should have failed.\n"); - } ok(GetLastError() == 0xdeadbeef || /* 9x */ GetLastError() == ERROR_INVALID_WINDOW_HANDLE /* NT */, "HiliteMenuItem: expected error ERROR_INVALID_WINDOW_HANDLE, got: %ld\n", GetLastError()); diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c index 98456446e11..3e42547650a 100644 --- a/dlls/win32u/menu.c +++ b/dlls/win32u/menu.c @@ -4545,6 +4545,12 @@ BOOL WINAPI NtUserHiliteMenuItem( HWND hwnd, HMENU handle, UINT item, UINT hilit TRACE( "(%p, %p, %04x, %04x);\n", hwnd, handle, item, hilite ); + if (!hwnd) + { + RtlSetLastWin32Error(ERROR_INVALID_WINDOW_HANDLE); + return FALSE; + } + if (!(menu = find_menu_item(handle, item, hilite, &pos))) return FALSE; handle_menu = menu->obj.handle; focused_item = menu->FocusedItem; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1554
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/win32u/menu.c | 47 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c index 3e42547650a..397fda2420d 100644 --- a/dlls/win32u/menu.c +++ b/dlls/win32u/menu.c @@ -3152,6 +3152,35 @@ static void ensure_menu_item_visible( struct menu *menu, UINT index, HDC hdc ) } } +static void deselect_item(HWND owner, HMENU hmenu) +{ + struct menu *menu; + HDC hdc; + + TRACE( "owner %p menu %p\n", owner, hmenu); + + menu = unsafe_menu_ptr( hmenu ); + if (!menu || !menu->nItems || !menu->hWnd) + return; + + if (menu->wFlags & MF_POPUP) + hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE ); + else + hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW ); + + NtGdiSelectFont( hdc, get_menu_font( FALSE ) ); + + if (menu->FocusedItem != NO_SELECTED_ITEM) + { + menu->items[menu->FocusedItem].fState &= ~(MF_HILITE | MF_MOUSESELECT); + draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem], + !(menu->wFlags & MF_POPUP), ODA_SELECT ); + menu->FocusedItem = NO_SELECTED_ITEM; + } + + NtUserReleaseDC( menu->hWnd, hdc ); +} + static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select, HMENU topmenu ) { struct menu *menu; @@ -3174,12 +3203,7 @@ static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select, NtGdiSelectFont( hdc, get_menu_font( FALSE )); /* Clear previous highlighted item */ - if (menu->FocusedItem != NO_SELECTED_ITEM) - { - menu->items[menu->FocusedItem].fState &= ~(MF_HILITE|MF_MOUSESELECT); - draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem], - !(menu->wFlags & MF_POPUP), ODA_SELECT ); - } + deselect_item(owner, hmenu); /* Highlight new item (if any) */ menu->FocusedItem = index; @@ -4556,11 +4580,16 @@ BOOL WINAPI NtUserHiliteMenuItem( HWND hwnd, HMENU handle, UINT item, UINT hilit focused_item = menu->FocusedItem; release_menu_ptr(menu); - if (focused_item != pos) + if ((hilite & MF_HILITE)) { - hide_sub_popups( hwnd, handle_menu, FALSE, 0 ); - select_item( hwnd, handle_menu, pos, TRUE, 0 ); + if (focused_item != pos) + { + hide_sub_popups( hwnd, handle_menu, FALSE, 0 ); + select_item( hwnd, handle_menu, pos, TRUE, 0 ); + } } + else /* MF_UNHILITE */ + deselect_item( hwnd, handle_menu ); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1554
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/user32/tests/menu.c | 6 ----- dlls/win32u/menu.c | 58 +++++++++++++++++++++++++--------------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c index b00c403940f..9cd24141ad5 100644 --- a/dlls/user32/tests/menu.c +++ b/dlls/user32/tests/menu.c @@ -2578,11 +2578,8 @@ static void test_menu_hilitemenuitem( void ) ok(GetLastError() == 0xdeadbeef, "HiliteMenuItem: expected error 0xdeadbeef, got: %ld\n", GetLastError()); - todo_wine - { ok(GetMenuState(hPopupMenu, 1, MF_BYPOSITION) & MF_HILITE, "HiliteMenuItem: Item 2 is not hilited\n"); - } /* unhilite a menu item (by position) */ @@ -2603,11 +2600,8 @@ static void test_menu_hilitemenuitem( void ) ok(GetLastError() == 0xdeadbeef, "HiliteMenuItem: expected error 0xdeadbeef, got: %ld\n", GetLastError()); - todo_wine - { ok(GetMenuState(hPopupMenu, 2, MF_BYPOSITION) & MF_HILITE, "HiliteMenuItem: Item 3 is not hilited\n"); - } /* unhilite a menu item (by command) */ diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c index 397fda2420d..a0fb851bb87 100644 --- a/dlls/win32u/menu.c +++ b/dlls/win32u/menu.c @@ -3155,66 +3155,80 @@ static void ensure_menu_item_visible( struct menu *menu, UINT index, HDC hdc ) static void deselect_item(HWND owner, HMENU hmenu) { struct menu *menu; - HDC hdc; + HDC hdc = NULL; TRACE( "owner %p menu %p\n", owner, hmenu); menu = unsafe_menu_ptr( hmenu ); - if (!menu || !menu->nItems || !menu->hWnd) + if (!menu || !menu->nItems) return; - if (menu->wFlags & MF_POPUP) - hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE ); - else - hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW ); + if (menu->hWnd) + { + if (menu->wFlags & MF_POPUP) + hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE ); + else + hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW ); - NtGdiSelectFont( hdc, get_menu_font( FALSE ) ); + NtGdiSelectFont( hdc, get_menu_font( FALSE ) ); + } if (menu->FocusedItem != NO_SELECTED_ITEM) { menu->items[menu->FocusedItem].fState &= ~(MF_HILITE | MF_MOUSESELECT); - draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem], - !(menu->wFlags & MF_POPUP), ODA_SELECT ); + if (menu->hWnd) + draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem], + !(menu->wFlags & MF_POPUP), ODA_SELECT ); menu->FocusedItem = NO_SELECTED_ITEM; } - NtUserReleaseDC( menu->hWnd, hdc ); + if (hdc) + NtUserReleaseDC( menu->hWnd, hdc ); } static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select, HMENU topmenu ) { struct menu *menu; - HDC hdc; + HDC hdc = NULL; TRACE( "owner %p menu %p index 0x%04x select 0x%04x\n", owner, hmenu, index, send_select ); menu = unsafe_menu_ptr( hmenu ); - if (!menu || !menu->nItems || !menu->hWnd) return; + if (!menu || !menu->nItems) return; if (menu->FocusedItem == index) return; - if (menu->wFlags & MF_POPUP) hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE ); - else hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW); - if (!top_popup) + if (menu->hWnd) + { + if (menu->wFlags & MF_POPUP) + hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE ); + else + hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW); + + NtGdiSelectFont( hdc, get_menu_font( FALSE )); + } + if (!top_popup && menu->hWnd) { top_popup = menu->hWnd; top_popup_hmenu = hmenu; } - NtGdiSelectFont( hdc, get_menu_font( FALSE )); - /* Clear previous highlighted item */ deselect_item(owner, hmenu); /* Highlight new item (if any) */ menu->FocusedItem = index; + send_select = send_select && menu->hWnd; if (menu->FocusedItem != NO_SELECTED_ITEM) { if (!(menu->items[index].fType & MF_SEPARATOR)) { menu->items[index].fState |= MF_HILITE; - ensure_menu_item_visible( menu, index, hdc ); - draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[index], - !(menu->wFlags & MF_POPUP), ODA_SELECT ); + if (menu->hWnd) + { + ensure_menu_item_visible( menu, index, hdc ); + draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[index], + !(menu->wFlags & MF_POPUP), ODA_SELECT ); + } } if (send_select) { @@ -3240,7 +3254,9 @@ static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select, } } } - NtUserReleaseDC( menu->hWnd, hdc ); + + if (hdc) + NtUserReleaseDC( menu->hWnd, hdc ); } /*********************************************************************** -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1554
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/user32/tests/menu.c | 17 +++++++++++++++++ dlls/win32u/menu.c | 4 ++++ 2 files changed, 21 insertions(+) diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c index 9cd24141ad5..f6b37365209 100644 --- a/dlls/user32/tests/menu.c +++ b/dlls/user32/tests/menu.c @@ -2614,6 +2614,23 @@ static void test_menu_hilitemenuitem( void ) ok(!(GetMenuState(hPopupMenu, 2, MF_BYPOSITION) & MF_HILITE), "HiliteMenuItem: Item 3 is hilited\n"); + /* deleting an off-screen menu item doesn't reset hilite */ + + SetLastError(0xdeadbeef); + ok(HiliteMenuItem(hWnd, hPopupMenu, 0, MF_HILITE | MF_BYPOSITION), + "HiliteMenuItem: call should not have failed.\n"); + ok(GetLastError() == 0xdeadbeef, + "HiliteMenuItem: expected error 0xdeadbeef, got: %ld\n", GetLastError()); + ok(GetMenuState(hPopupMenu, 0, MF_BYPOSITION) & MF_HILITE, + "HiliteMenuItem: Item 1 is not hilited\n"); + + ok(DeleteMenu(hPopupMenu, 2, MF_BYPOSITION), + "DeleteMenu: call should have succeeded.\n"); + ok(GetMenuState(hPopupMenu, 0, MF_BYPOSITION) & MF_HILITE, + "HiliteMenuItem: Item 1 is not hilited\n"); + ok(!(GetMenuState(hPopupMenu, 1, MF_BYPOSITION) & MF_HILITE), + "HiliteMenuItem: Item 2 is hilited\n"); + DestroyWindow(hWnd); } diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c index a0fb851bb87..706c63b39c4 100644 --- a/dlls/win32u/menu.c +++ b/dlls/win32u/menu.c @@ -1370,11 +1370,15 @@ BOOL WINAPI NtUserRemoveMenu( HMENU handle, UINT id, UINT flags ) { free( menu->items ); menu->items = NULL; + menu->FocusedItem = NO_SELECTED_ITEM; } else { struct menu_item *new_items, *item = &menu->items[pos]; + if (menu->FocusedItem >= menu->nItems) + menu->FocusedItem = NO_SELECTED_ITEM; + while (pos < menu->nItems) { *item = item[1]; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1554
From: Vladislav Timonin <timoninvlad(a)yandex.ru> --- dlls/user32/tests/menu.c | 27 +++++++++++++++++++++++++++ dlls/win32u/menu.c | 3 +++ 2 files changed, 30 insertions(+) diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c index f6b37365209..9cbd513232d 100644 --- a/dlls/user32/tests/menu.c +++ b/dlls/user32/tests/menu.c @@ -2631,6 +2631,33 @@ static void test_menu_hilitemenuitem( void ) ok(!(GetMenuState(hPopupMenu, 1, MF_BYPOSITION) & MF_HILITE), "HiliteMenuItem: Item 2 is hilited\n"); + ok(HiliteMenuItem(hWnd, hPopupMenu, 0, MF_UNHILITE | MF_BYPOSITION), + "HiliteMenuItem: call should not have failed.\n"); + ok(!(GetMenuState(hPopupMenu, 0, MF_BYPOSITION) & MF_HILITE), + "HiliteMenuItem: Item 1 is hilited\n"); + AppendMenuA(hPopupMenu, MF_STRING, 103, "Item 3"); + + /* inserting into off-screen menu doesn't move hilite */ + + SetLastError(0xdeadbeef); + ok(HiliteMenuItem(hWnd, hPopupMenu, 1, MF_HILITE | MF_BYPOSITION), + "HiliteMenuItem: call should not have failed.\n"); + ok(GetLastError() == 0xdeadbeef, + "HiliteMenuItem: expected error 0xdeadbeef, got: %ld\n", GetLastError()); + ok(GetMenuState(hPopupMenu, 1, MF_BYPOSITION) & MF_HILITE, + "HiliteMenuItem: Item 2 is not hilited\n"); + + ok(InsertMenuA(hPopupMenu, 1, MF_STRING, 104, "Item 4"), + "InsertMenuA: call should have succeeded.\n"); + ok(!(GetMenuState(hPopupMenu, 0, MF_BYPOSITION) & MF_HILITE), + "HiliteMenuItem: Item 1 is hilited\n"); + ok(GetMenuState(hPopupMenu, 1, MF_BYPOSITION) & MF_HILITE, + "HiliteMenuItem: Item 4 is not hilited\n"); + ok(!(GetMenuState(hPopupMenu, 2, MF_BYPOSITION) & MF_HILITE), + "HiliteMenuItem: Item 2 is hilited\n"); + ok(!(GetMenuState(hPopupMenu, 3, MF_BYPOSITION) & MF_HILITE), + "HiliteMenuItem: Item 3 is hilited\n"); + DestroyWindow(hWnd); } diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c index 706c63b39c4..80d514f7b55 100644 --- a/dlls/win32u/menu.c +++ b/dlls/win32u/menu.c @@ -467,6 +467,9 @@ static struct menu *insert_menu_item( HMENU handle, UINT id, UINT flags, UINT *r memset( &new_items[pos], 0, sizeof(*new_items) ); menu->Height = 0; /* force size recalculate */ + if (menu->FocusedItem != NO_SELECTED_ITEM && menu->FocusedItem >= pos) + menu->FocusedItem++; + *ret_pos = pos; return menu; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1554
participants (2)
-
Vladislav Timonin -
Vladislav Timonin (＠vt)