Re: [PATCH] [Server]: when getting the status of an exception, ensure the debugger continue information is not erased when releasing objects

[Alexandre Julliard]

Eric Pouech  writes:
> diff --git a/server/debugger.c b/server/debugger.c
> index 795a24a..4458a3b 100644
> --- a/server/debugger.c
> +++ b/server/debugger.c
> @@ -677,6 +677,8 @@ DECL_HANDLER(get_exception_status)
>      if ((event = (struct debug_event *)get_handle_obj( current->process, req->handle,
>                                                         0, &debug_event_ops )))
>      {
> -
>    NTSTATUS status = STATUS_PENDING;
> -
>    close_handle( current->process, req->handle );
>    if (event->state == EVENT_CONTINUED)
>    {
>
> @@ -686,10 +688,10 @@ DECL_HANDLER(get_exception_status)
>                  set_reply_data( &event->context, size );
>                  current->context = NULL;
>              }
> -
>        set_error( event->status );
> -
>        status = event->status;
>    }
> -
>    else set_error( STATUS_PENDING );
>    release_object( event );
> -
>    set_error( status );

Releasing an object should never change the error code. Where do you see
that happening?

[PATCH] [Server]: when getting the status of an exception, ensure the debugger continue information is not erased when releasing objects

[Eric Pouech]

Alexandre Julliard a écrit :
> Eric Pouech  writes:
> > diff --git a/server/debugger.c b/server/debugger.c
> > index 795a24a..4458a3b 100644
> > --- a/server/debugger.c
> > +++ b/server/debugger.c
> > @@ -677,6 +677,8 @@ DECL_HANDLER(get_exception_status)
> >      if ((event = (struct debug_event *)get_handle_obj( current->process, req->handle,
> >                                                         0, &debug_event_ops )))
> >      {
> > -
> >    NTSTATUS status = STATUS_PENDING;
> > -
> >    close_handle( current->process, req->handle );
> >    if (event->state == EVENT_CONTINUED)
> >    {
> >
> > @@ -686,10 +688,10 @@ DECL_HANDLER(get_exception_status)
> >                  set_reply_data( &event->context, size );
> >                  current->context = NULL;
> >              }
> > -
> >        set_error( event->status );
> > -
> >        status = event->status;
> >    }
> > -
> >    else set_error( STATUS_PENDING );
> >    release_object( event );
> > -
> >    set_error( status );
>
> Releasing an object should never change the error code. Where do you see
> that happening?

what happens is something like:
process P being debugged by D
- while processing a debug event (fetched in P, P calls 
ntdll@send_debug_event, and waits for the debugger to process the event)
- D gets the event, and the user asks for quit in D. D sends back to 
wineserver a DBG_CONTINUE response to the event
- D then dies before P runs again
- P then calls server@get_exception_status, where when getting the 
status from the debugger, (which should be DBG_CONTINUE, and it is 
before releasing the debug event). but while releasing the debug event, 
someone clears the status, and we get 0 from the get_exception_status event

I didn't check in details which clear_error() is to blame in server, but 
close_thread_desktop (from a cursory look) could be to blame

A+

[PATCH] [Server]: when getting the status of an exception, ensure the debugger continue information is not erased when releasing objects

[Alexandre Julliard]

Eric Pouech  writes:
> what happens is something like:
> process P being debugged by D
> - while processing a debug event (fetched in P, P calls
>
> ntdll@send_debug_event, and waits for the debugger to process the
> event)
> - D gets the event, and the user asks for quit in D. D sends back to
>
> wineserver a DBG_CONTINUE response to the event
> - D then dies before P runs again
> - P then calls server@get_exception_status, where when getting the
>
> status from the debugger, (which should be DBG_CONTINUE, and it is
> before releasing the debug event). but while releasing the debug
> event, someone clears the status, and we get 0 from the
> get_exception_status event
>
> I didn't check in details which clear_error() is to blame in server,
> but close_thread_desktop (from a cursory look) could be to blame

That one probably shouldn't clear the error, but I don't see how you'd
get there by releasing an event. Do you have a backtrace?