[PATCH v5 0/1] MR1554: Draft: win32u: Update focused item when removing menu item

[Vladislav Timonin (＠vt)]

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.

--
  v5: win32u: Reset focused item if it was removed when removing a menu item
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1554

[PATCH v5 1/1] win32u: Reset focused item if it was removed when removing a menu item

[Vladislav Timonin]

From: Vladislav Timonin <timoninvlad(a)yandex.ru>

---
 dlls/user32/tests/menu.c | 76 ++++++++++++++++++++++++++++++++++++++++
 dlls/win32u/menu.c       |  4 +++
 2 files changed, 80 insertions(+)

diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c
index 6862f13c6b1..fbe3b3641a1 100644
--- a/dlls/user32/tests/menu.c
+++ b/dlls/user32/tests/menu.c
@@ -2623,6 +2623,29 @@ 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());
+    todo_wine
+    {
+    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");
+    todo_wine
+    {
+    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);
 }
 
@@ -3721,8 +3744,27 @@ static LRESULT WINAPI menu_fill_in_init(HWND hwnd, UINT msg,
     return DefWindowProcA(hwnd, msg, wparam, lparam);
 }
 
+static LRESULT WINAPI menu_select_second_item(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam)
+{
+    HWND hwndmenu;
+
+    switch (msg)
+    {
+    case WM_ENTERIDLE:
+        hwndmenu = GetCapture();
+        if (hwndmenu) {
+            PostMessageA(hwndmenu, WM_KEYDOWN, VK_DOWN, 0);
+            PostMessageA(hwndmenu, WM_KEYDOWN, VK_DOWN, 0);
+            PostMessageA(hwndmenu, WM_KEYDOWN, VK_RETURN, 0);
+        }
+    }
+
+    return DefWindowProcA(hwnd, msg, wparam, lparam);
+}
+
 static void test_emptypopup(void)
 {
+    const int itemid = 0x1234567;
     BOOL ret;
     HMENU hmenu;
 
@@ -3786,6 +3828,40 @@ static void test_emptypopup(void)
 
     ret = DestroyMenu(hmenu);
     ok(ret, "DestroyMenu failed with error %ld\n", GetLastError());
+
+    /* check that selecting an item, clearing the menu, and then opening an empty menu doesn't crash it */
+    hwnd = CreateWindowExA(0, (LPCSTR)MAKEINTATOM(atomMenuCheckClass), NULL,
+            WS_SYSMENU | WS_VISIBLE, CW_USEDEFAULT, CW_USEDEFAULT, 100, 100,
+            NULL, NULL, NULL, NULL);
+    ok(hwnd != NULL, "CreateWindowEx failed with error %ld\n", GetLastError());
+    SetWindowLongPtrA( hwnd, GWLP_WNDPROC, (LONG_PTR)menu_select_second_item);
+    hmenu = CreatePopupMenu();
+
+    AppendMenuA(hmenu, MF_STRING, itemid, "Item 1");
+    ret = TrackPopupMenu(hmenu, TPM_RETURNCMD, 100, 100, 0, hwnd, NULL);
+    ok(ret == itemid, "TrackPopupMenu returned %d error is %ld\n", ret, GetLastError());
+
+    RemoveMenu(hmenu, 0, MF_BYPOSITION);
+    ret = TrackPopupMenu(hmenu, TPM_RETURNCMD, 100, 100, 0, hwnd, NULL);
+    ok(ret == 0, "TrackPopupMenu returned %d error is %ld\n", ret, GetLastError());
+
+    ret = DestroyMenu(hmenu);
+    ok(ret, "DestroyMenu failed with error %ld\n", GetLastError());
+
+    /* check that selecting an item, deleting the selected item, and then opening an empty menu doesn't crash it */
+    hmenu = CreatePopupMenu();
+
+    AppendMenuA(hmenu, MF_STRING, 0, "Item 1");
+    AppendMenuA(hmenu, MF_STRING, itemid, "Item 2");
+    ret = TrackPopupMenu(hmenu, TPM_RETURNCMD, 100, 100, 0, hwnd, NULL);
+    ok(ret == itemid, "TrackPopupMenu returned %d error is %ld\n", ret, GetLastError());
+
+    RemoveMenu(hmenu, 1, MF_BYPOSITION);
+    ret = TrackPopupMenu(hmenu, TPM_RETURNCMD, 100, 100, 0, hwnd, NULL);
+    ok(ret == 0, "TrackPopupMenu returned %d error is %ld\n", ret, GetLastError());
+
+    ret = DestroyMenu(hmenu);
+    ok(ret, "DestroyMenu failed with error %ld\n", GetLastError());
 }
 
 static HMENU get_bad_hmenu( UINT_PTR id )
diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c
index a6b7ec38003..1e6a4760c46 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

[PATCH v5 0/1] MR1554: win32u: Reset focused item if it was removed when removing a menu item

[Vladislav Timonin (＠vt)]

Added a proper test that reproduces the issue I ran into with split button. Without having to bring comctl 6.0 into test suite and break half of it.

Dropped the off-screen support for `select_item`. It also happens to fix the same issue (by allowing `if (is_menu( mt.hTopMenu ))` check in `track_menu` proceed on menu close and reset selection there), but it should probably go into a separate MR.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1554#note_18104