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

[Huw Davies]

On Mon, Apr 30, 2018 at 03:11:25PM +0300, Nikolay Sivov wrote:
> +struct menu_item_desc
> +{
> +    MENUITEM *item;
> +    POPUPMENU *menu;
> +    UINT pos;
> +};
>  
>  /***********************************************************************
>   *           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 BOOL MENU_FindItem(HMENU hmenu, UINT id, UINT flags, struct menu_item_desc *desc)

I'd have probably used the chance to rename this as find_item() ;-)

I also wondered about having this just return the menu and pos (in
which case it's find_item_pos()) and then either letting the callers
index directly into to the returned menu's items array, or adding a
get_item() that would do that.  I suppose though there are enough
callers that want the item, that this combined approach is a good
compromise.

>  {
> +    UINT fallback_pos = ~0u, i;
>      POPUPMENU *menu;
> -    MENUITEM *fallback = NULL;
> -    UINT fallback_pos = 0;
> -    UINT i;
>  
> -    if ((*hmenu == (HMENU)0xffff) || (!(menu = MENU_GetMenu(*hmenu)))) return NULL;
> -    if (wFlags & MF_BYPOSITION)
> +    memset(desc, 0, sizeof(*desc));
> +
> +    menu = grab_menu_ptr(hmenu);
> +    if (!menu)
> +        return FALSE;
> +
> +    if (flags & MF_BYPOSITION)
>      {
> -	if (*nPos >= menu->nItems) return NULL;
> -	return &menu->items[*nPos];
> +        if (id >= menu->nItems)
> +        {
> +            release_menu_ptr(menu);
> +            return FALSE;
> +        }
> +
> +        desc->menu = menu;
> +        desc->item = &menu->items[id];
> +        desc->pos = id;
> +        return TRUE;
>      }
>      else
>      {
> @@ -612,32 +625,37 @@ static MENUITEM *MENU_FindItem( HMENU *hmenu, UINT *nPos, UINT wFlags )
>  	{
>  	    if (item->fType & MF_POPUP)
>  	    {
> -		HMENU hsubmenu = item->hSubMenu;
> -		MENUITEM *subitem = MENU_FindItem( &hsubmenu, nPos, wFlags );
> -		if (subitem)
> -		{
> -		    *hmenu = hsubmenu;
> -		    return subitem;
> -		}
> -		else if (item->wID == *nPos)
> +                if (MENU_FindItem(item->hSubMenu, id, flags, desc))
> +                {
> +                    release_menu_ptr(menu);
> +                    return TRUE;
> +                }
> +                else if (item->wID == id)
>  		{
>  		    /* fallback to this item if nothing else found */
>  		    fallback_pos = i;
> -		    fallback = item;
>  		}
>  	    }
> -	    else if (item->wID == *nPos)
> +	    else if (item->wID == id)
>  	    {
> -		*nPos = i;
> -		return item;
> +                desc->menu = menu;
> +                desc->item = item;
> +                desc->pos = i;
> +                return TRUE;
>  	    }
>  	}
>      }
>  
> -    if (fallback)
> -        *nPos = fallback_pos;
> +    if (fallback_pos != ~0u)
> +    {
> +        desc->menu = menu;
> +        desc->item = &menu->items[fallback_pos];
> +        desc->pos = fallback_pos;
> +    }
> +    else
> +        release_menu_ptr(menu);
>  
> -    return fallback;
> +    return fallback_pos != ~0u;
>  }
>  
>  /***********************************************************************
> @@ -2135,28 +2153,34 @@ static void MENU_MoveSelection( HWND hwndOwner, HMENU hmenu, INT offset )
>   *
>   * Insert (allocate) a new item into a menu.
>   */
> -static MENUITEM *MENU_InsertItem( HMENU hMenu, UINT pos, UINT flags )
> +static BOOL MENU_InsertItem(HMENU hMenu, UINT id, UINT flags, struct menu_item_desc *desc)
>  {
> +    UINT pos = id;
>      MENUITEM *newItems;
>      POPUPMENU *menu;
>  
> -    if (!(menu = MENU_GetMenu(hMenu)))
> -        return NULL;
> +    memset(desc, 0, sizeof(*desc));
>  
>      /* Find where to insert new item */
> -
>      if (flags & MF_BYPOSITION) {
> -        if (pos > menu->nItems)
> +        menu = grab_menu_ptr(hMenu);
> +        if (id > menu->nItems)
>              pos = menu->nItems;
>      } else {
> -        if (!MENU_FindItem( &hMenu, &pos, flags ))
> +        struct menu_item_desc item_desc;
> +
> +        if (!MENU_FindItem(hMenu, id, flags, &item_desc))
> +        {
> +            menu = grab_menu_ptr(hMenu);
>              pos = menu->nItems;
> -        else {
> -            if (!(menu = MENU_GetMenu( hMenu )))
> -                return NULL;
>          }
> +        else
> +            menu = item_desc.menu;

Shouldn't you use the returned item_desc.pos here?

Also, to simplify things, couldn't the MF_BYPOSITION case just call
MENU_FindItem() too?

Huw.