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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list