On 04/05/2011 02:46 PM, Tiago Vignatti wrote: > On 04/05/2011 03:54 PM, ext Simon Thum wrote: >> On 04/05/2011 01:27 PM, Tiago Vignatti wrote: >>> On 04/05/2011 03:14 PM, ext Simon Thum wrote: >>>> On 04/04/2011 07:54 PM, Tiago Vignatti wrote: >>>>> It will fix two possible cases of use after free in RemoveDevice. >>>>> >>>>> Signed-off-by: Tiago Vignatti<[email protected]> >>>>> --- >>>>> dix/devices.c | 1 + >>>>> 1 files changed, 1 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/dix/devices.c b/dix/devices.c >>>>> index 534931c..0288e15 100644 >>>>> --- a/dix/devices.c >>>>> +++ b/dix/devices.c >>>>> @@ -941,6 +941,7 @@ CloseDevice(DeviceIntPtr dev) >>>>> free(dev->config_info); /* Allocated in >>>>> xf86ActivateDevice. */ >>>>> dev->config_info = NULL; >>>>> dixFreeObjectWithPrivates(dev, PRIVATE_DEVICE); >>>>> + dev = NULL; >>>>> } >>>>> >>>>> /** >>>> OK, but _how_ does it do what you say it does? I'm just seeing a dead >>>> store to a local. >>> >>> if you check RemoveDevice, you will see two loops (for) with a >>> conditional (tmp = dev). If one of these conditional takes true path it >>> will call CloseDevice(tmp), which in turn will free(tmp) through >>> dixFreeObjectWithPrivates(dev, PRIVATE_DEVICE). Next, when it returns, >>> the conditional to stop the loop in RemoveDevice will test the value of >>> tmp, which cannot be done because was previously freed. >> That can be done, because they're comparing the pointer without >> dereferencing. Still, the loops in RD are strange. They're disguised >> ways to get the device out of the global lists, which should be done >> strictly before CloseDevice is called. But they seem correct (though >> weird) to me as long as the device is only ever in at most one of the >> lists. All dereferencing is done before CloseDevice(). >> >> Making RemoveDevice readable instead would be nice, but why? What >> actually leads you to think this dead store solves a problem? > > all right, nevermind and sorry about the confusion. > > I was caught about a false positive here in the statical analyzer which > lead me to miscook the patch: the problem started when I haven't paid > attention that tmp was passed by value instead reference; later there is > this thing of deference a value not allocated (prev = tmp) but which > won't be used. Well maybe it's not the best style, but clearly valid - if you are willing to assume that that the removal will not be preempted by a device addition which allocates just that precise address still stored in locals. Which is 99% safe and should be made 100% safe by ensuring RD is called with signals off.
Sounds more like an analyzer problem. > > Thanks for checking and ignore this patch. > > > PS: as a enhancement for RemoveDevice, can we use break on both loops > when a device is found given there will be only one device removed at > one time? It'd certainly be an improvement. Also check for signal safety when you're at it. Cheers, Simon _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
