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;
>     }

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,