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

[Tiago Vignatti]

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.

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?

        Tiago