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.
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
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
