Re: kernel32 & winex11: WaitForMultipleObjectsEx should ignore NULL handles

[Robert Shearman]

Adam Strzelecki wrote:
> Hi,
>
> In MSI dialog.c: msi_dialog_check_messages function it often happens 
> that is sends:
> MsgWaitForMultipleObjects( 1, &handle, 0, INFINITE, QS_ALLINPUT );
> Where handle = NULL. I'm not sure if it is correct behavior. But 
> tracing it down I found out that kernel32 and winex11 sync functions 
> are not checking at all if the passes handles are not NULLs.
>
> So I'm not sure whether this patch is right or not, but I believe we 
> shouldn't bother wineserver with NULL handles!?
> Yet maybe I don't understand clearly the way interserver communication 
> works, and sending "WaitFor" with NULL handle makes sense. Does it?

No, but this is the wrong fix.

You're assuming that because MSI code does action A and that causes bad 
effect when it does action B that the correct action is to fix B so that 
it doesn't have a bad effect. However, the bug could also be that action 
A shouldn't be done.

In this case, MSI should not be calling MsgWaitForMultipleObjects with 
an invalid handle and in fact that barring memory corruption, this can't 
happen:
> while (1)
>     {
>         msi_process_pending_messages( NULL );
>
> if( !handle )
>             break;
>
> /*
>          * block here until somebody creates a new dialog or
>          * the handle we're waiting on becomes ready
>          */
>         r = MsgWaitForMultipleObjects( 1, &handle, 0, INFINITE, 
> QS_ALLINPUT );
>         if( r == WAIT_OBJECT_0 )
>             break;
>     }

-- 
Rob Shearman

kernel32 & winex11: WaitForMultipleObjectsEx should ignore NULL handles

[Adam Strzelecki]

> In this case, MSI should not be calling MsgWaitForMultipleObjects  
> with an invalid handle and in fact that barring memory corruption,  
> this can't happen:
> You're right, my patch was incorrect, I made bad assumption.

I'm still not sure why this X11DRV_MsgWaitForMultipleObjectsEx is  
testing for "(count || timeout)", rather than "(count && timeout)" or  
just "count" at:
> if (process_events( data->display, mask )) ret = count - 1;
>      else if (count || timeout)
>      {
>          ret = WaitForMultipleObjectsEx( count, handles, flags &  
> MWMO_WAITALL,
>                                          timeout, flags &  
> MWMO_ALERTABLE );
> >

I can see that msi_dialog_run_message_loop calls  
"MsgWaitForMultipleObjects( 0, NULL, 0, INFINITE, QS_ALLINPUT );"  
which implies that
"WaitForMultipleObjects( 0, NULL, 0, INFINITE, MWMO_ALERTABLE );" can  
be called if precess_events returns FALSE.

I'm pretty sure I saw in gdb some calls with NULL handles to  
WaitForMultipleObjects related to MSI.

Regards,
-- 
Adam Strzelecki |: nanoant.com :|