[libvirt] [PATCH] tests: fix segault 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. Fix this by nullifying obj passed to virNodeDeviceObjRemove. --- src/conf/node_device_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a23d8ef..16b9d93 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -218,6 +218,7 @@ void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, if (devs->objs[i] == dev) { virNodeDeviceObjUnlock(dev); virNodeDeviceObjFree(devs->objs[i]); + *(void**)dev = NULL; VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; -- 2.7.4
On 24.08.2016 12:55, 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.
Fix this by nullifying obj passed to virNodeDeviceObjRemove. --- src/conf/node_device_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a23d8ef..16b9d93 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -218,6 +218,7 @@ void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, if (devs->objs[i] == dev) { virNodeDeviceObjUnlock(dev); virNodeDeviceObjFree(devs->objs[i]); + *(void**)dev = NULL;
VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break;
This is very hackish. It only clears first 8 bytes of the struct so next time somebody tries to access the lock in there it's all zeroes. Either we should make virNodeDeviceObjRemove() take a double pointer (and thus set the pointer to NULL properly), or fix every caller of virNodeDeviceObjRemove(). Michal
Michal Privoznik wrote:
On 24.08.2016 12:55, 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.
Fix this by nullifying obj passed to virNodeDeviceObjRemove. --- src/conf/node_device_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a23d8ef..16b9d93 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -218,6 +218,7 @@ void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, if (devs->objs[i] == dev) { virNodeDeviceObjUnlock(dev); virNodeDeviceObjFree(devs->objs[i]); + *(void**)dev = NULL;
VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break;
This is very hackish. It only clears first 8 bytes of the struct so next time somebody tries to access the lock in there it's all zeroes.
At this point there's no sense to access the lock because it's been destroyed with virMutexDestroy anyway. But yeah, it feels hackish...
Either we should make virNodeDeviceObjRemove() take a double pointer (and thus set the pointer to NULL properly), or fix every caller of virNodeDeviceObjRemove().
Uh, yeah. I'd prefer to use double pointer I guess. virNodeDeviceObjRemove() doesn't seem to be a part of public API and we're allowed to change it this way, right? If that's the case I like this approach better. Roman Bogorodskiy
participants (2)
-
Michal Privoznik -
Roman Bogorodskiy