Re: server: Fix for possible desktop variable being NULL when set_process_default_desktop is called

[Alexandre Julliard]

Robert Shearman  writes:
> ChangeLog:
> The desktop variable could be NULL when set_process_default_desktop is
> called, so check for this. (Found by Coverity).

I don't see how this could happen. Did I miss something?

server: Fix for possible desktop variable being NULL when set_process_default_desktop is called

[Robert Shearman]

Alexandre Julliard wrote:
> Robert Shearman  writes:
> > ChangeLog:
> > The desktop variable could be NULL when set_process_default_desktop is
> > called, so check for this. (Found by Coverity).
>
> I don't see how this could happen. Did I miss something?

See below.
> void connect_process_winstation( *struct* process *process, *struct* thread *parent )
> {
>    *struct* winstation *winstation = NULL;
>    *struct* desktop *desktop = NULL;
>    obj_handle_t handle;
>
> //* check for an inherited winstation handle (don't ask...) *//
>    *if* ((handle = find_inherited_handle( process, &winstation_ops )))
>    {
>        winstation = (*struct* winstation *)get_handle_obj( process, handle, 0, &winstation_ops );
>    }
>    *else* *if* (parent && parent->process->winstation)
>    {
>        handle = duplicate_handle( parent->process, parent->process->winstation,
>                                   process, 0, 0, DUP_HANDLE_SAME_ACCESS );
>        winstation = (*struct* winstation *)get_handle_obj( process, handle, 0, &winstation_ops );
>    }
>    *if* (!winstation) *goto* done;
>    process->winstation = handle;

Handle is non-NULL here.
> *if* ((handle = find_inherited_handle( process, &desktop_ops )))
>    {
>        desktop = get_desktop_obj( process, handle, 0 );
>        *if* (!desktop || desktop->winstation != winstation) *goto* done;
>    }
>    *else* *if* (parent && parent->desktop)
>    {
>        desktop = get_desktop_obj( parent->process, parent->desktop, 0 );
>        *if* (!desktop || desktop->winstation != winstation) *goto* done;
>        handle = duplicate_handle( parent->process, parent->desktop,
>                                   process, 0, 0, DUP_HANDLE_SAME_ACCESS );
>    }

If the process neither has desktop nor its parent has a desktop then 
handle will be non-NULL here, yet desktop will be NULL.
> *if* (handle) set_process_default_desktop( process, desktop, handle );
>
> done:
>    *if* (desktop) release_object( desktop );
>    *if* (winstation) release_object( winstation );
>    clear_error();
> }

server: Fix for possible desktop variable being NULL when set_process_default_desktop is called

[Alexandre Julliard]

Robert Shearman  writes:
> Handle is non-NULL here.
> > *if* ((handle = find_inherited_handle( process, &desktop_ops )))

It's set to NULL here if we don't find an inherited handle.
> > {
> >        desktop = get_desktop_obj( process, handle, 0 );
> >        *if* (!desktop || desktop->winstation != winstation) *goto* done;
> >    }
> >    *else* *if* (parent && parent->desktop)
> >    {
> >        desktop = get_desktop_obj( parent->process, parent->desktop, 0 );
> >        *if* (!desktop || desktop->winstation != winstation) *goto* done;
> >        handle = duplicate_handle( parent->process, parent->desktop,
> >                                   process, 0, 0, DUP_HANDLE_SAME_ACCESS );
> >    }
>
> If the process neither has desktop nor its parent has a desktop then
> handle will be non-NULL here, yet desktop will be NULL.

I still don't see how. It wouldn't make sense since the handle and
desktop are supposed to be the same object.

server: Fix for possible desktop variable being NULL when set_process_default_desktop is called

[Robert Shearman]

Alexandre Julliard wrote:
> Robert Shearman  writes:
> > Handle is non-NULL here.
> > > *if* ((handle = find_inherited_handle( process, &desktop_ops )))
>
> It's set to NULL here if we don't find an inherited handle.

/* check for an inherited winstation handle (don't ask...) */
    if ((handle = find_inherited_handle( process, &winstation_ops )))
    {
        winstation = (struct winstation *)get_handle_obj( process, 
handle, 0, &winstation_ops );
    }
    else if (parent && parent->process->winstation)
    {
        handle = duplicate_handle( parent->process, 
parent->process->winstation,
                                   process, 0, 0, DUP_HANDLE_SAME_ACCESS );
        winstation = (struct winstation *)get_handle_obj( process, 
handle, 0, &winstation_ops );
    }
    if (!winstation) goto done;
    process->winstation = handle;

Handle is guaranteed to be non-NULL here, since winstation and handle 
are the same object (well, except if duplicate_handle fails).

server: Fix for possible desktop variable being NULL when set_process_default_desktop is called

[Alexandre Julliard]

Robert Shearman  writes:
> /* check for an inherited winstation handle (don't ask...) */
>    if ((handle = find_inherited_handle( process, &winstation_ops )))
>    {
>        winstation = (struct winstation *)get_handle_obj( process,
> handle, 0, &winstation_ops );
>    }
>    else if (parent && parent->process->winstation)
>    {
>        handle = duplicate_handle( parent->process,
> parent->process->winstation,
>                                   process, 0, 0, DUP_HANDLE_SAME_ACCESS );
>        winstation = (struct winstation *)get_handle_obj( process,
> handle, 0, &winstation_ops );
>    }
>    if (!winstation) goto done;
>    process->winstation = handle;
>
> Handle is guaranteed to be non-NULL here, since winstation and handle
> are the same object (well, except if duplicate_handle fails).

Yes, it's non-NULL here, but that's irrelevant since it's reset on the
next line.

server: Fix for possible desktop variable being NULL when set_process_default_desktop is called

[Robert Shearman]

Alexandre Julliard wrote:
> Robert Shearman  writes:
> > /* check for an inherited winstation handle (don't ask...) */
> >   if ((handle = find_inherited_handle( process, &winstation_ops )))
> >   {
> >       winstation = (struct winstation *)get_handle_obj( process,
> > handle, 0, &winstation_ops );
> >   }
> >   else if (parent && parent->process->winstation)
> >   {
> >       handle = duplicate_handle( parent->process,
> > parent->process->winstation,
> >                                  process, 0, 0, DUP_HANDLE_SAME_ACCESS );
> >       winstation = (struct winstation *)get_handle_obj( process,
> > handle, 0, &winstation_ops );
> >   }
> >   if (!winstation) goto done;
> >   process->winstation = handle;
> >
> > Handle is guaranteed to be non-NULL here, since winstation and handle
> > are the same object (well, except if duplicate_handle fails).
>
> Yes, it's non-NULL here, but that's irrelevant since it's reset on the
> next line.

Sorry, you are correct. It does indeed look like a flaw in the scanner:

At conditional (5): "handle = find_inherited_handle != 0" taking false path

298          if ((handle = find_inherited_handle( process, &desktop_ops )))
...
Event var_deref_model: Variable "desktop" tracked as NULL was passed to 
a function that dereferences it. [model]
Also see events: [assign_zero]
At conditional (8): "handle != 0" taking true path
311          if (handle) set_process_default_desktop( process, desktop, 
handle );

It doesn't seem to have tracked that handle was assigned zero on line 298.

server: Fix for possible desktop variable being NULL when set_process_default_desktop is called

[Mike Hearn]

On Fri, 07 Apr 2006 13:34:49 +0100, Robert Shearman wrote:
> Sorry, you are correct. It does indeed look like a flaw in the scanner:

I'm not sure whether this is a bug in the scanner or some other issue, but
cases where it flags things that cannot happen because of the
design of other parts of the code is what I meant by "logical" errors,
rather than actual boolean logic. I'm not really convinced a static
analysis can catch things that we "know" cannot happen (eg it'd be a
violation of the X protocol or something) and so rely on them without
explicit checks.

thanks -mike