Re: [v4 PATCH] user32/menu: Return locked menu data when looking for menu items.

[Huw Davies]

On Wed, May 09, 2018 at 12:04:28PM +0300, Nikolay Sivov wrote:
> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
> ---
>
> v4: changed helper interface, simplified insertion point logic a bit.
>
> dlls/user32/menu.c | 556 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 348 insertions(+), 208 deletions(-)
>
> diff --git a/dlls/user32/menu.c b/dlls/user32/menu.c
> index 537c2ebe1a..98f1fce506 100644
> --- a/dlls/user32/menu.c
> +++ b/dlls/user32/menu.c
> @@ -584,26 +584,25 @@ static UINT  MENU_GetStartOfPrevColumn(
>      return i;
>  }
>
> -
> -
> -/***********************************************************************
> - *           MENU_FindItem
> - *
> - * Find a menu item. Return a pointer on the item, and modifies *hmenu
> - * in case the item was in a sub-menu.
> - */
> -static MENUITEM *MENU_FindItem( HMENU *hmenu, UINT *nPos, UINT wFlags )
> +static POPUPMENU *find_menu_item(HMENU hmenu, UINT id, UINT flags, UINT *pos)

I think this is more natural than the previous version.
> BOOL WINAPI RemoveMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>  {
> -    LPPOPUPMENU	menu;
> -    MENUITEM *item;
> +    POPUPMENU *menu;
> +    UINT pos;
>
> TRACE("(menu=%p pos=%04x flags=%04x)\n",hMenu, nPos, wFlags);
> -    if (!(item = MENU_FindItem( &hMenu, &nPos, wFlags ))) return FALSE;
> -    if (!(menu = MENU_GetMenu(hMenu))) return FALSE;
>
> -      /* Remove item */
> +    if (!(menu = find_menu_item(hMenu, nPos, wFlags, &pos)))
> +        return FALSE;
>
> -    MENU_FreeItemData( item );
> +    /* Remove item */
> +    MENU_FreeItemData( &menu->items[pos] );
>
> if (--menu->nItems == 0)
>      {
> @@ -4024,17 +4093,19 @@ BOOL WINAPI RemoveMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>      }
>      else
>      {
> -        MENUITEM *new_items;
> -	while(nPos < menu->nItems)
> +        MENUITEM *new_items, *item = &menu->items[pos];
> +
> +	while (nPos < menu->nItems)
>   {
>       *item = *(item+1);
>       item++;
>       nPos++;
>   }

These should be pos not nPos.  I'd suggest changing the function's parameter name
from nPos to id.
> -        new_items = HeapReAlloc( GetProcessHeap(), 0, menu->items,
> -                                 menu->nItems * sizeof(MENUITEM) );
> +        new_items = HeapReAlloc( GetProcessHeap(), 0, menu->items, menu->nItems * sizeof(MENUITEM) );
>          if (new_items) menu->items = new_items;
>      }
> +    release_menu_ptr(menu);
> +
>      return TRUE;
>  }
>
> @@ -4044,11 +4115,17 @@ BOOL WINAPI RemoveMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>   */
>  BOOL WINAPI DeleteMenu( HMENU hMenu, UINT nPos, UINT wFlags )
>  {

Probably change nPos to id here as well.

Huw.