[PATCH v2 0/4] 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. -- v2: win32u: Reset focused item when removing menu item win32u: Allow (de)select_item to work with off-screen menus win32u: Add deselect to HiliteMenuItem win32u: Return FALSE in HiliteMenuItem when window handle is NULL 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 | 16 ++++++++++++++++ dlls/win32u/menu.c | 5 +++++ 2 files changed, 21 insertions(+) diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c index 9cd24141ad5..53bff17e1e6 100644 --- a/dlls/user32/tests/menu.c +++ b/dlls/user32/tests/menu.c @@ -2614,6 +2614,22 @@ static void test_menu_hilitemenuitem( void ) ok(!(GetMenuState(hPopupMenu, 2, MF_BYPOSITION) & MF_HILITE), "HiliteMenuItem: Item 3 is hilited\n"); + /* deleting an item resets 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 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..ac9f31fe545 100644 --- a/dlls/win32u/menu.c +++ b/dlls/win32u/menu.c @@ -1375,6 +1375,9 @@ BOOL WINAPI NtUserRemoveMenu( HMENU handle, UINT id, UINT flags ) { struct menu_item *new_items, *item = &menu->items[pos]; + if (menu->FocusedItem != NO_SELECTED_ITEM) + menu->items[menu->FocusedItem].fState &= ~(MF_HILITE | MF_MOUSESELECT); + while (pos < menu->nItems) { *item = item[1]; @@ -1385,6 +1388,8 @@ BOOL WINAPI NtUserRemoveMenu( HMENU handle, UINT id, UINT flags ) if (new_items) menu->items = new_items; } + menu->FocusedItem = NO_SELECTED_ITEM; + release_menu_ptr(menu); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1554
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=126831 Your paranoid android. === w7u_2qxl (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w7u_adm (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w7u_el (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w8 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w8adm (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w864 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1507 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1809 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_tsign (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w864 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1507 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1809 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_2qxl (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_adm (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_tsign (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_en_AE_u8 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_ar (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_ja (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_zh_CN (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
Added a test, but for it be actually useful it required changing `HiliteMenuItem` to be able to work with off-screen menus. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1554#note_17605
On Sun Nov 27 16:58:55 2022 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=126831 Your paranoid android. === w7u_2qxl (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w7u_adm (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w7u_el (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w8 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w8adm (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w864 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1507 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1809 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_tsign (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64 (32 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w864 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1507 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064v1809 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_2qxl (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_adm (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w1064_tsign (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_en_AE_u8 (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_ar (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_ja (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited === w10pro64_zh_CN (64 bit report) === user32: menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited ``` Awkward, thought I checked this on Windows before submitting.
Then I'm guessing the reset happens when popup is opened or closed, seeing how the test GUI app refreshes the menu after an item was deleted. Wine currently doesn't do that so the item will be stuck on screen, non-interactable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1554#note_17607
participants (3)
-
Marvin -
Vladislav Timonin -
Vladislav Timonin (＠vt)