[PATCH 3/5] dix: set pointer to NULL after freeing at CloseDevice

[Simon Thum]

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<tiago.vignatti at nokia.com>
>>> ---
>>>   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?

> 
> hope you understood my explanation...
> 
>       Tiago
>