[libvirt] [PATCH v2] tests: fix segfault in objecteventtest

Test 12 from objecteventtest (createXML add event) segaults on FreeBSD with bus error. At some point it calls testNodeDeviceDestroy() from the test driver. And it fails when it tries to unlock the device in the "out:" label of this function. Unlocking fails because the previous step was a call to virNodeDeviceObjRemove from conf/node_device_conf.c. This function removes the given device from the device list and cleans up the object, including destroying of its mutex. However, it does not nullify the pointer that was given to it. As a result, we end up in testNodeDeviceDestroy() here: out: if (obj) virNodeDeviceObjUnlock(obj); And instead of skipping this, we try to do Unlock and fail because of malformed mutex. Change virNodeDeviceObjRemove to use double pointer and set pointer to NULL. --- src/conf/node_device_conf.c | 13 +++++++------ src/conf/node_device_conf.h | 2 +- src/node_device/node_device_hal.c | 4 ++-- src/test/test_driver.c | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a23d8ef..719abcd 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -207,22 +207,23 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, } void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr dev) + virNodeDeviceObjPtr *dev) { size_t i; - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(*dev); for (i = 0; i < devs->count; i++) { - virNodeDeviceObjLock(dev); - if (devs->objs[i] == dev) { - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjLock(*dev); + if (devs->objs[i] == *dev) { + virNodeDeviceObjUnlock(*dev); virNodeDeviceObjFree(devs->objs[i]); + *dev = NULL; VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; } - virNodeDeviceObjUnlock(dev); + virNodeDeviceObjUnlock(*dev); } } diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 8f23a98..9f00500 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -249,7 +249,7 @@ virNodeDeviceObjPtr virNodeDeviceAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def); void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, - virNodeDeviceObjPtr dev); + virNodeDeviceObjPtr *dev); char *virNodeDeviceDefFormat(const virNodeDeviceDef *def); diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index ef0cd9c..fb7bf25 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -530,7 +530,7 @@ dev_refresh(const char *udi) /* Simply "rediscover" device -- incrementally handling changes * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ - virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjRemove(&driver->devs, &dev); } else { VIR_DEBUG("no device named %s", name); } @@ -560,7 +560,7 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, dev = virNodeDeviceFindByName(&driver->devs, name); VIR_DEBUG("%s", name); if (dev) - virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjRemove(&driver->devs, &dev); else VIR_DEBUG("no device named %s", name); nodeDeviceUnlock(); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index bc1f93d..53cfa3c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5534,7 +5534,7 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) 0); virNodeDeviceObjLock(obj); - virNodeDeviceObjRemove(&driver->devs, obj); + virNodeDeviceObjRemove(&driver->devs, &obj); out: if (obj) -- 2.7.4

On 25.08.2016 07:50, Roman Bogorodskiy wrote:
Test 12 from objecteventtest (createXML add event) segaults on FreeBSD with bus error.
At some point it calls testNodeDeviceDestroy() from the test driver. And it fails when it tries to unlock the device in the "out:" label of this function.
Unlocking fails because the previous step was a call to virNodeDeviceObjRemove from conf/node_device_conf.c. This function removes the given device from the device list and cleans up the object, including destroying of its mutex. However, it does not nullify the pointer that was given to it.
As a result, we end up in testNodeDeviceDestroy() here:
out: if (obj) virNodeDeviceObjUnlock(obj);
And instead of skipping this, we try to do Unlock and fail because of malformed mutex.
Change virNodeDeviceObjRemove to use double pointer and set pointer to NULL. --- src/conf/node_device_conf.c | 13 +++++++------ src/conf/node_device_conf.h | 2 +- src/node_device/node_device_hal.c | 4 ++-- src/test/test_driver.c | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-)
Almost. You've forgotten about udev (perhaps due to BSD? :-P) ACK if you squash this in: diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ddf3d88..520269f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1044,7 +1044,7 @@ static int udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", dev->def->name, name); - virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjRemove(&driver->devs, &dev); ret = 0; cleanup: Safe for freeze. Michal

Michal Privoznik wrote:
On 25.08.2016 07:50, Roman Bogorodskiy wrote:
Test 12 from objecteventtest (createXML add event) segaults on FreeBSD with bus error.
At some point it calls testNodeDeviceDestroy() from the test driver. And it fails when it tries to unlock the device in the "out:" label of this function.
Unlocking fails because the previous step was a call to virNodeDeviceObjRemove from conf/node_device_conf.c. This function removes the given device from the device list and cleans up the object, including destroying of its mutex. However, it does not nullify the pointer that was given to it.
As a result, we end up in testNodeDeviceDestroy() here:
out: if (obj) virNodeDeviceObjUnlock(obj);
And instead of skipping this, we try to do Unlock and fail because of malformed mutex.
Change virNodeDeviceObjRemove to use double pointer and set pointer to NULL. --- src/conf/node_device_conf.c | 13 +++++++------ src/conf/node_device_conf.h | 2 +- src/node_device/node_device_hal.c | 4 ++-- src/test/test_driver.c | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-)
Almost. You've forgotten about udev (perhaps due to BSD? :-P)
Sorry, should have tested this on Linux as well. :( Thanks for catching that!
ACK if you squash this in:
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ddf3d88..520269f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1044,7 +1044,7 @@ static int udevRemoveOneDevice(struct udev_device *device)
VIR_DEBUG("Removing device '%s' with sysfs path '%s'", dev->def->name, name); - virNodeDeviceObjRemove(&driver->devs, dev); + virNodeDeviceObjRemove(&driver->devs, &dev);
ret = 0; cleanup:
Safe for freeze.
Pushed with the udev fix squashed in, thanks! Roman Bogorodskiy
participants (2)
-
Michal Privoznik
-
Roman Bogorodskiy