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

[Simon Thum]

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<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?
> 
> 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