On May 6, 2014, at 9:28 AM, Andrew Eikum wrote:
+static HMENU MENU_GetHandle(POPUPMENU *menu) +{
- return menu ? menu->obj.handle : NULL;
+}
Now that DestroyMenu() really destroys the handle, any use of the handle and thus this function has to be examined very carefully. The menu pointer may be valid and menu->obj.handle may have a value, but that value may no longer be a valid handle.
menuchar = SendMessageW( hwndOwner, WM_MENUCHAR,
MAKEWPARAM( key, menu->wFlags ), (LPARAM)hmenu );
MAKEWPARAM( key, menu->wFlags ), (LPARAM)MENU_GetHandle(menu) );
So, here we're passing the menu handle in the WM_MENUCHAR message. If the menu has been destroyed, that's an invalid handle. We probably need tests to verify that that's what happens on Windows. That is, maybe it passes NULL. Or maybe the handle is still valid somehow. (The existing destroyed-submenu tests suggest that the handle isn't valid anymore, but maybe magic happens.)
drawItem.hwndItem = (HWND)hmenu;
drawItem.hwndItem = (HWND)MENU_GetHandle(menu);
dis.hwndItem = (HWND)hmenu;
dis.hwndItem = (HWND)MENU_GetHandle(menu);
Same for the WM_DRAWITEM message.
@@ -1845,7 +1840,7 @@ static BOOL MENU_InitPopup( HWND hwndOwner, HMENU hmenu, UINT flags ) menu->hWnd = CreateWindowExW( ex_style, (LPCWSTR)POPUPMENU_CLASS_ATOM, NULL, WS_POPUP, 0, 0, 0, 0, hwndOwner, 0, (HINSTANCE)GetWindowLongPtrW(hwndOwner, GWLP_HINSTANCE),
(LPVOID)hmenu );
(LPVOID)MENU_GetHandle(menu) );
This one is interesting. This is a value that's just forwarded through to the WM_CREATE message via the lpCreateParams field of the CREATESTRUCTW pointed to by the lparam. It's then stored in the window extra space using SetWindowLongPtrW(). It's then looked up elsewhere in the window proc. Some cases in the window proc try to use the handle, which will fail if it's been destroyed.
I think you can pass the menu pointer here instead of the menu handle. Likewise, I think you can store the menu pointer into the window extra space. You'd have to change the size of the extra space defined in the MENU_builtin_class struct. And you'd need to change all uses of Set/GetWindowLongPtrW() on the menu's window with 0 index.
MENU_DrawPopupMenu() would need to be changed to take a menu pointer instead of a handle.
A couple of those uses are for the MM_SETMENUHANDLE and MM_GETMENUHANDLE messages. I believe those are obsolete Wine-internal messages. They can probably be completely removed.
For the MN_GETHMENU message, you can return MENU_GetHandle(GetWindowLongPtrW(hwnd, 0)), although, again, it would be good to test what Windows does for a destroyed-but-still-alive menu.
The stored value is also used in GetMenuBarInfo(), so you'd want to rework that.
@@ -2023,18 +2018,17 @@ static void MENU_SelectItem( HWND hwndOwner, HMENU hmenu, UINT wIndex,
SendMessageW( hwndOwner, WM_MENUSELECT, MAKEWPARAM(pos, ip->fType | ip->fState |
(ptm->wFlags & MF_SYSMENU)), (LPARAM)topmenu);
(topmenu->wFlags & MF_SYSMENU)), (LPARAM)topmenu);
Here you missed a case. The lparam should be the menu handle, but topmenu is now a menu pointer.
It might be good to also review all remaining uses of MENU_GetMenu(). Again, I think it should be possible to confine uses of menu handles just to the interfacing with client code. So, uses of MENU_GetMenu() at other places indicates that you're relying on the handle being valid. For example, the call to GetSubMenu() in MENU_FindItemByKey() or the calls to EnableMenuItem() in MENU_InitSysMenuPopup(). Unfortunately, that may involve creating a lot of helper functions.
I didn't review further than this.
Sorry that my suggestions are creating so much work. If you haven't already done so, you should probably get input from others (i.e. Alexandre) to see if my suggestions make sense.
-Ken
On Wed, May 07, 2014 at 06:09:44AM -0500, Ken Thomases wrote:
On May 6, 2014, at 9:28 AM, Andrew Eikum wrote:
+static HMENU MENU_GetHandle(POPUPMENU *menu) +{
- return menu ? menu->obj.handle : NULL;
+}
Now that DestroyMenu() really destroys the handle, any use of the handle and thus this function has to be examined very carefully. The menu pointer may be valid and menu->obj.handle may have a value, but that value may no longer be a valid handle.
Unless I'm mistaken, these issues existed before this patch as well. DestroyMenu has always closed the handle, and additionally destroyed the struct. Now we preserve the struct if it's referenced by another menu.
menuchar = SendMessageW( hwndOwner, WM_MENUCHAR,
MAKEWPARAM( key, menu->wFlags ), (LPARAM)hmenu );
MAKEWPARAM( key, menu->wFlags ), (LPARAM)MENU_GetHandle(menu) );So, here we're passing the menu handle in the WM_MENUCHAR message. If the menu has been destroyed, that's an invalid handle. We probably need tests to verify that that's what happens on Windows. That is, maybe it passes NULL. Or maybe the handle is still valid somehow. (The existing destroyed-submenu tests suggest that the handle isn't valid anymore, but maybe magic happens.)
drawItem.hwndItem = (HWND)hmenu;
drawItem.hwndItem = (HWND)MENU_GetHandle(menu);
dis.hwndItem = (HWND)hmenu;
dis.hwndItem = (HWND)MENU_GetHandle(menu);Same for the WM_DRAWITEM message.
There is some precedent for Windows returning an invalid HMENU. See the second GetMenuItemInfoA call in test_subpopup_locked_by_menu which returns the handle of the submenu that was just destroyed.
@@ -1845,7 +1840,7 @@ static BOOL MENU_InitPopup( HWND hwndOwner, HMENU hmenu, UINT flags ) menu->hWnd = CreateWindowExW( ex_style, (LPCWSTR)POPUPMENU_CLASS_ATOM, NULL, WS_POPUP, 0, 0, 0, 0, hwndOwner, 0, (HINSTANCE)GetWindowLongPtrW(hwndOwner, GWLP_HINSTANCE),
(LPVOID)hmenu );
(LPVOID)MENU_GetHandle(menu) );This one is interesting. This is a value that's just forwarded through to the WM_CREATE message via the lpCreateParams field of the CREATESTRUCTW pointed to by the lparam. It's then stored in the window extra space using SetWindowLongPtrW(). It's then looked up elsewhere in the window proc. Some cases in the window proc try to use the handle, which will fail if it's been destroyed.
I think you can pass the menu pointer here instead of the menu handle. Likewise, I think you can store the menu pointer into the window extra space. You'd have to change the size of the extra space defined in the MENU_builtin_class struct. And you'd need to change all uses of Set/GetWindowLongPtrW() on the menu's window with 0 index.
MENU_DrawPopupMenu() would need to be changed to take a menu pointer instead of a handle.
A couple of those uses are for the MM_SETMENUHANDLE and MM_GETMENUHANDLE messages. I believe those are obsolete Wine-internal messages. They can probably be completely removed.
For the MN_GETHMENU message, you can return MENU_GetHandle(GetWindowLongPtrW(hwnd, 0)), although, again, it would be good to test what Windows does for a destroyed-but-still-alive menu.
The stored value is also used in GetMenuBarInfo(), so you'd want to rework that.
This is a pretty straightforward change, so I wrote a new patch to clean it up.
It might be good to also review all remaining uses of MENU_GetMenu(). Again, I think it should be possible to confine uses of menu handles just to the interfacing with client code. So, uses of MENU_GetMenu() at other places indicates that you're relying on the handle being valid. For example, the call to GetSubMenu() in MENU_FindItemByKey() or the calls to EnableMenuItem() in MENU_InitSysMenuPopup(). Unfortunately, that may involve creating a lot of helper functions.
Yes, we should head in this direction. It would be really nice to clean up the use-after-free FIXME in MENU_GetMenu. I think that's outside the scope of this particular patch, though.
I didn't review further than this.
Sorry that my suggestions are creating so much work. If you haven't already done so, you should probably get input from others (i.e. Alexandre) to see if my suggestions make sense.
It feels right to me, at least. Thanks for reviewing!
Andrew
On May 7, 2014, at 9:57 AM, Andrew Eikum wrote:
On Wed, May 07, 2014 at 06:09:44AM -0500, Ken Thomases wrote:
On May 6, 2014, at 9:28 AM, Andrew Eikum wrote:
+static HMENU MENU_GetHandle(POPUPMENU *menu) +{
- return menu ? menu->obj.handle : NULL;
+}
Now that DestroyMenu() really destroys the handle, any use of the handle and thus this function has to be examined very carefully. The menu pointer may be valid and menu->obj.handle may have a value, but that value may no longer be a valid handle.
Unless I'm mistaken, these issues existed before this patch as well. DestroyMenu has always closed the handle, and additionally destroyed the struct. Now we preserve the struct if it's referenced by another menu.
Well, first of all, the situation before this patch has problems because of that, which is what you're trying to solve.
Also, the specific issue of having the struct and going from that to the handle, which might not be valid, and then implicitly expecting to be able to get back to the struct is new.
There is some precedent for Windows returning an invalid HMENU. See the second GetMenuItemInfoA call in test_subpopup_locked_by_menu which returns the handle of the submenu that was just destroyed.
Yes. I'm just not sure if that's what happens in these particular circumstances. And I'm not so terribly concerned about the handle being passed out to other code being invalid, although it would be good to know we're doing the right thing. It's mostly about places where the code has the menu pointer, obtains the menu handle from it, passes that to other code in the menu module, and then tries to get the menu pointer again.
-Ken
On Wed, May 07, 2014 at 03:59:07PM -0500, Ken Thomases wrote:
On May 7, 2014, at 9:57 AM, Andrew Eikum wrote:
Unless I'm mistaken, these issues existed before this patch as well. DestroyMenu has always closed the handle, and additionally destroyed the struct. Now we preserve the struct if it's referenced by another menu.
Well, first of all, the situation before this patch has problems because of that, which is what you're trying to solve.
Also, the specific issue of having the struct and going from that to the handle, which might not be valid, and then implicitly expecting to be able to get back to the struct is new.
I don't think it is new. Before the patch, we still relied on the HMENU being valid, it's just stored in a different place now. I don't think I've introduced any dependencies on the handle being valid that weren't already there, I've only removed them. Yes, there are some remaining, but those can be addressed in future patches as applications need it.
The only new problem I can think of is where before we would have just passed an invalid HMENU, now the struct may have been destroyed and we can no longer get the HMENU. But this issue is precisely why I had to add the struct refcounting to this large patch instead of breaking it out, so this should not actually be a problem.
There is some precedent for Windows returning an invalid HMENU. See the second GetMenuItemInfoA call in test_subpopup_locked_by_menu which returns the handle of the submenu that was just destroyed.
Yes. I'm just not sure if that's what happens in these particular circumstances. And I'm not so terribly concerned about the handle being passed out to other code being invalid, although it would be good to know we're doing the right thing. It's mostly about places where the code has the menu pointer, obtains the menu handle from it, passes that to other code in the menu module, and then tries to get the menu pointer again.
You're right that those should be cleaned up, but this patch is already huge at over 700 changed lines. Future patches could address those issues as needed. This patch shouldn't make them any worse than they already are.
Andrew
-
Andrew Eikum -
Ken Thomases