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