Re: [libvirt] [PATCH 1/2] util: mdev: Use a local variable instead of an invalid pointer reference

On Fri, Apr 28, 2017 at 09:46:32AM +0200, Erik Skultety wrote: > virMediatedDeviceListAdd calls VIR_APPEND_ELEMENT which normally clears > the element to be added, thus the pointer must not be used afterwards, > but because the pointer here is passed by value, what gets cleared is > a copy of the original pointer that got created on the stack > automatically when calling the function. The fact that it works now is > just a coincidence and a bug in virMediatedDeviceListAdd that needs to > be fixed in a separate commit. Therefore, use a local variable to hold > data, rather than accessing the pointer later. I don't thing that this patch is required. What happens here is that in this function we get a pointer to an existing memory that stores virMediatedDevice, if the mdev is not used we mark it as used and store that pointer to *dst* list. That's what virMediatedDeviceListAdd does, it only stores the pointer to an allocated into the list so it doesn't clear the real data, it only operates with pointers. This means that the memory addressed by local *mdev* variable still points to a valid memory and everything is OK, there is no issue at all apart from a fact, that setting the pointer in virMediatedDeviceListAdd is pointless. Pavel

> Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; > --- > src/util/virmdev.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/util/virmdev.c b/src/util/virmdev.c > index 2a1ade739..c1499d238 100644 > --- a/src/util/virmdev.c > +++ b/src/util/virmdev.c > @@ -447,20 +447,21 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, > virObjectLock(dst); > for (i = 0; i < count; i++) { > virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i); > + const char *mdev_path = mdev->path; > > if (virMediatedDeviceIsUsed(mdev, dst) || > virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0) > goto cleanup; > > /* Copy mdev references to the driver list: > - * - caller is responsible for NOT freeing devices in @list on success > + * - caller is responsible for NOT freeing devices in @src on success > * - we're responsible for performing a rollback on failure > */ > if (virMediatedDeviceListAdd(dst, mdev) < 0) > goto rollback; > > VIR_DEBUG("'%s' added to list of active mediated devices used by '%s'", > - mdev->path, domname); > + mdev_path, domname); > } > > ret = 0; > -- > 2.12.2 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list