Bug 12830 - Xkb cleanup deferences released pointer
Summary: Xkb cleanup deferences released pointer
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/DDX/Xorg (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: xorg-7.4
  Show dependency treegraph
 
Reported: 2007-10-16 12:20 UTC by Paulo César Pereira de Andrade
Modified: 2008-02-28 02:16 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
proposed fix (682 bytes, text/x-patch)
2007-10-16 12:22 UTC, Paulo César Pereira de Andrade
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paulo César Pereira de Andrade 2007-10-16 12:20:45 UTC
While experimenting with an attempt to create a "Xserver crash recovery" environment/patch I found this clear bug. The problem is that xkb/xkbEvents.c:XkbRemoveResourceClient() deferences the dev->key->xkbInfo field,
but this field has been already released earlier in dix/devices.c:CloseDevice()
Comment 1 Paulo César Pereira de Andrade 2007-10-16 12:22:08 UTC
Created attachment 12079 [details]
proposed fix
Comment 2 Daniel Stone 2007-10-16 17:26:57 UTC
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.
Comment 3 Paulo César Pereira de Andrade 2007-10-16 18:04:12 UTC
  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).
Comment 4 Peter Hutterer 2008-02-26 23:54:08 UTC
(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

Comment 5 Paulo César Pereira de Andrade 2008-02-27 10:15:47 UTC
> 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.
Comment 6 Peter Hutterer 2008-02-27 20:51:16 UTC
(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?

Comment 7 Daniel Stone 2008-02-28 02:16:09 UTC
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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.