Bug 12830

Created attachment 12079 [details]
proposed fix

er, surely the better fix would be to just set dev->key to NULL; no point changing the controls on something we're going to immediately free anyway.

  Setting it to NULL is probably a better fix. I just adapted the patch to execute the code originally intended to run.
  XkbRemoveResourceClient() code is also a bit "bloated", it should be like:
--
XkbInterestPtr ptr, prev;

for (prev = ptr = dev->xkb_interest; ptr; prev = ptr, ptr = ptr->next) {
    if (ptr->resource == id) {
        ** set local variables **
        if (prev == ptr)
            dev->xkb_interest = ptr->next;
        else
            prev->next = ptr->next;
        XkbFree(ptr);
        break;
    }
}
--
Or some variant without the need of too many local variables.
Anyway, dix/devices.c:CloseDevice() calls it with the "resource" value of the first element in the list...

But setting a released pointer to NULL is good practice.

And this bug may be the cause of random server crashes at exit, as it may happen or not, based on memory layout and "malloc" implementation (there are no allocations during the "free" calls, but contents may still be changed by the "malloc" implementation).

(In reply to comment #3)
>   Setting it to NULL is probably a better fix. I just adapted the patch to
> execute the code originally intended to run.
>   XkbRemoveResourceClient() code is also a bit "bloated", it should be like:
> --
> XkbInterestPtr ptr, prev;
> 
> for (prev = ptr = dev->xkb_interest; ptr; prev = ptr, ptr = ptr->next) {
>     if (ptr->resource == id) {
>         ** set local variables **
>         if (prev == ptr)
>             dev->xkb_interest = ptr->next;
>         else
>             prev->next = ptr->next;
>         XkbFree(ptr);
>         break;
>     }
> }

can you please provide a full patch and submit it to xorg@?

(In reply to comment #2)
> er, surely the better fix would be to just set dev->key to NULL; no point
> changing the controls on something we're going to immediately free anyway.
> 

pushed to master as 2257e20900460d85254734b595238e7ad5ee55c8

> can you please provide a full patch and submit it to xorg@?
> 
> (In reply to comment #2)
> > er, surely the better fix would be to just set dev->key to NULL; no point

  Attachment 1 [details] is an old patch, not in git format but should better
describe what the initial proposed patch does, i.e. it moves the cleanup
routines calls around to not deference the released pointer.

> pushed to master as 2257e20900460d85254734b595238e7ad5ee55c8

  Just setting dev->key to NULL should be enough, but maybe in some obscure
case (for sure not when the X Server is shuting down :-) it should really
also change/update controls, and in that case, attachment 1 [details] [review] is still a valid
patch.

(In reply to comment #5) 
>   Just setting dev->key to NULL should be enough, but maybe in some obscure
> case (for sure not when the X Server is shuting down :-) it should really
> also change/update controls, and in that case, attachment 1 [details] [review] is still a valid
> patch.

In all cases we close the device, free the memory and it's gone. So - yes - calling XkbRemove... before freeing xkbInfo may be the right thing to do since it would send an XkbControlsNotifyEvent to the client. But - if the device is gone anyway - why should we send this event? can you come up with a case where this is necessary?

On Wed, Feb 27, 2008 at 08:51:16PM -0800, bugzilla-daemon@freedesktop.org wrote:
> --- Comment #6 from Peter Hutterer <peter@cs.unisa.edu.au>  2008-02-27 20:51:16 PST ---
> (In reply to comment #5) 
> >   Just setting dev->key to NULL should be enough, but maybe in some obscure
> > case (for sure not when the X Server is shuting down :-) it should really
> > also change/update controls, and in that case, attachment 1 [details] [review] is still a valid
> > patch.
> 
> In all cases we close the device, free the memory and it's gone. So - yes -
> calling XkbRemove... before freeing xkbInfo may be the right thing to do since
> it would send an XkbControlsNotifyEvent to the client. But - if the device is
> gone anyway - why should we send this event? can you come up with a case where
> this is necessary?

Well, you have three cases here:
  * Regeneration: You have no clients left.
  * Termination: You probably have no clients left by this stage, but if
                 you do, they're about to disappear anyway.
  * Hotplug: You're about to send a DevicePresenceNotify anyway.