In testDomainSetBlockIoTune() the info.group_name is strdup()-ed and just after the whole @info structure is passed to virDomainDiskSetBlockIOTune() the @group_name member is set to NULL. This creates a memleak, because virDomainDiskSetBlockIOTune() creates its own copy of the string. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/test/test_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1504334c30..9e1fc65972 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4027,7 +4027,6 @@ testDomainSetBlockIoTune(virDomainPtr dom, #undef TEST_BLOCK_IOTUNE_MAX_CHECK virDomainDiskSetBlockIOTune(conf_disk, &info); - info.group_name = NULL; ret = 0; cleanup: -- 2.34.1

There are a few places where a variable is VIR_FREE()-d and then explicitly set to NULL. This is not necessary since VIR_FREE() does that for us. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 1 - src/conf/nwfilter_conf.c | 1 - tests/virtypedparamtest.c | 1 - 3 files changed, 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 58e696416d..e931aef6c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12746,7 +12746,6 @@ virDomainGraphicsDefNew(virDomainXMLOption *xmlopt) if (xmlopt && xmlopt->privateData.graphicsNew && !(def->privateData = xmlopt->privateData.graphicsNew())) { VIR_FREE(def); - def = NULL; } return def; diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 3fedfdde56..c886218dde 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1813,7 +1813,6 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, if (match && STREQ(match, "no")) match_flag = NWFILTER_ENTRY_ITEM_FLAG_IS_NEG; VIR_FREE(match); - match = NULL; while (att[idx].name != NULL) { prop = virXMLPropString(node, att[idx].name); diff --git a/tests/virtypedparamtest.c b/tests/virtypedparamtest.c index 87b47ba86e..5ced453be5 100644 --- a/tests/virtypedparamtest.c +++ b/tests/virtypedparamtest.c @@ -106,7 +106,6 @@ testTypedParamsFilter(const void *opaque G_GNUC_UNUSED) goto cleanup; } VIR_FREE(filtered); - filtered = NULL; nfiltered = virTypedParamsFilter(params, G_N_ELEMENTS(params), "bar", &filtered); -- 2.34.1

Inside the testPCIVPDResourceCustomCompareIndex() function we have two variables @a and @b, both marked as g_autoptr(). Then, towards the end of the function b->value is freed and set to a->value. This is to make sure virPCIVPDResourceCustomCompareIndex() works correctly even if ->value member is the same for both arguments. Nevertheless, if the function returns anything else than 0 then the control executes subsequent return statement and since b->value points to the very same string as a->value a double free will occur. Avoid this by setting b->value to NULL explicitly, just like we are already doing for the successful path. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/virpcivpdtest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index a6bf46b103..a5f4abae6f 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -229,8 +229,10 @@ testPCIVPDResourceCustomCompareIndex(const void *data G_GNUC_UNUSED) /* Different index, same value pointers */ g_free(b->value); b->value = a->value; - if (virPCIVPDResourceCustomCompareIndex(b, a)) + if (virPCIVPDResourceCustomCompareIndex(b, a)) { + b->value = NULL; return -1; + } b->value = NULL; -- 2.34.1

There are few places where the g_steal_pointer() is open coded. Switch them to calling the g_steal_pointer() function instead. Generated by the following spatch: @ rule1 @ expression a, b; @@ <... - b = a; ... when != b - a = NULL; + b = g_steal_pointer(&a); ...> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/esx/esx_vi.c | 6 ++---- src/hyperv/hyperv_driver.c | 8 +++----- src/hyperv/hyperv_wmi.c | 3 +-- src/hypervisor/virhostdev.c | 3 +-- src/libxl/xen_common.c | 12 ++++-------- src/qemu/qemu_hotplug.c | 6 ++---- src/qemu/qemu_migration.c | 9 +++------ src/storage/storage_backend_gluster.c | 9 +++------ src/storage/storage_util.c | 6 ++---- src/util/virfile.c | 3 +-- src/util/virlease.c | 3 +-- src/util/virnuma.c | 9 +++------ 12 files changed, 26 insertions(+), 51 deletions(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 80ed6199e3..17401f0a36 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -1033,8 +1033,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) if (root != ctx->service->rootFolder) esxVI_ManagedObjectReference_Free(&root); - root = folder->_reference; - folder->_reference = NULL; + root = g_steal_pointer(&folder->_reference); } else { /* Try to lookup item as a datacenter */ if (esxVI_LookupDatacenter(ctx, item, root, NULL, &ctx->datacenter, @@ -1087,8 +1086,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) if (root != ctx->datacenter->hostFolder) esxVI_ManagedObjectReference_Free(&root); - root = folder->_reference; - folder->_reference = NULL; + root = g_steal_pointer(&folder->_reference); } else { /* Try to lookup item as a compute resource */ if (esxVI_LookupComputeResource(ctx, item, root, NULL, diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 7b684e04ba..52851ac507 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1780,8 +1780,7 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->version = g_strdup(os->data->Version); - conn->privateData = priv; - priv = NULL; + conn->privateData = g_steal_pointer(&priv); result = VIR_DRV_OPEN_SUCCESS; cleanup: @@ -3518,9 +3517,8 @@ hypervConnectListAllDomains(virConnectPtr conn, doms[count++] = domain; } - if (doms) - *domains = doms; - doms = NULL; + if (domains) + *domains = g_steal_pointer(&doms); ret = count; cleanup: diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 6712ef4835..83d28b22aa 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -994,8 +994,7 @@ hypervEnumAndPull(hypervPrivate *priv, hypervWqlQuery *wqlQuery, response = NULL; } - *list = head; - head = NULL; + *list = g_steal_pointer(&head); return 0; } diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index e44eb7f1d3..bcd5e96c2c 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -586,8 +586,7 @@ virHostdevRestoreNetConfig(virDomainHostdevDef *hostdev, */ if (MAC) { VIR_FREE(adminMAC); - adminMAC = MAC; - MAC = NULL; + adminMAC = g_steal_pointer(&MAC); } ignore_value(virNetDevSetNetConfig(linkdev, vf, diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index fc975f1299..b97ba0a199 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -682,9 +682,8 @@ xenParseVfb(virConf *conf, virDomainDef *def) if (xenConfigCopyStringOpt(conf, "keymap", &graphics->data.vnc.keymap) < 0) goto cleanup; def->graphics = g_new0(virDomainGraphicsDef *, 1); - def->graphics[0] = graphics; + def->graphics[0] = g_steal_pointer(&graphics); def->ngraphics = 1; - graphics = NULL; } else { if (xenConfigGetBool(conf, "sdl", &val, 0) < 0) goto cleanup; @@ -696,9 +695,8 @@ xenParseVfb(virConf *conf, virDomainDef *def) if (xenConfigCopyStringOpt(conf, "xauthority", &graphics->data.sdl.xauth) < 0) goto cleanup; def->graphics = g_new0(virDomainGraphicsDef *, 1); - def->graphics[0] = graphics; + def->graphics[0] = g_steal_pointer(&graphics); def->ngraphics = 1; - graphics = NULL; } } } @@ -774,9 +772,8 @@ xenParseVfb(virConf *conf, virDomainDef *def) VIR_FREE(listenAddr); } def->graphics = g_new0(virDomainGraphicsDef *, 1); - def->graphics[0] = graphics; + def->graphics[0] = g_steal_pointer(&graphics); def->ngraphics = 1; - graphics = NULL; } else { if (xenHandleConfGetValueStringListErrors(rc) < 0) goto cleanup; @@ -953,9 +950,8 @@ xenParseCharDev(virConf *conf, virDomainDef *def, const char *nativeFormat) chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; chr->target.port = 0; - def->parallels[0] = chr; + def->parallels[0] = g_steal_pointer(&chr); def->nparallels++; - chr = NULL; } /* Try to get the list of values to support multiple serial ports */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d801559057..c9548d7f35 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4297,8 +4297,7 @@ qemuDomainChangeGraphics(virQEMUDriver *driver, /* Steal the new dev's char * reference */ VIR_FREE(olddev->data.vnc.auth.passwd); - olddev->data.vnc.auth.passwd = dev->data.vnc.auth.passwd; - dev->data.vnc.auth.passwd = NULL; + olddev->data.vnc.auth.passwd = g_steal_pointer(&dev->data.vnc.auth.passwd); olddev->data.vnc.auth.validTo = dev->data.vnc.auth.validTo; olddev->data.vnc.auth.expires = dev->data.vnc.auth.expires; olddev->data.vnc.auth.connected = dev->data.vnc.auth.connected; @@ -4345,8 +4344,7 @@ qemuDomainChangeGraphics(virQEMUDriver *driver, /* Steal the new dev's char * reference */ VIR_FREE(olddev->data.spice.auth.passwd); - olddev->data.spice.auth.passwd = dev->data.spice.auth.passwd; - dev->data.spice.auth.passwd = NULL; + olddev->data.spice.auth.passwd = g_steal_pointer(&dev->data.spice.auth.passwd); olddev->data.spice.auth.validTo = dev->data.spice.auth.validTo; olddev->data.spice.auth.expires = dev->data.spice.auth.expires; olddev->data.spice.auth.connected = dev->data.spice.auth.connected; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2635ef1162..09b8c415f4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3029,8 +3029,7 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, if (mig->lockState) { VIR_DEBUG("Received lockstate %s", mig->lockState); VIR_FREE(priv->lockState); - priv->lockState = mig->lockState; - mig->lockState = NULL; + priv->lockState = g_steal_pointer(&mig->lockState); } else { VIR_DEBUG("Received no lockstate"); } @@ -5598,8 +5597,7 @@ qemuMigrationDstPersist(virQEMUDriver *driver, error: virDomainDefFree(vm->newDef); vm->persistent = oldPersist; - vm->newDef = oldDef; - oldDef = NULL; + vm->newDef = g_steal_pointer(&oldDef); return -1; } @@ -5778,8 +5776,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, } if (mig->jobInfo) { - jobInfo = mig->jobInfo; - mig->jobInfo = NULL; + jobInfo = g_steal_pointer(&mig->jobInfo); if (jobInfo->sent && timeReceived) { jobInfo->timeDelta = timeReceived - jobInfo->sent; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index aacc23663e..52007db2ba 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -290,13 +290,10 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterState *state, if (meta->capacity) vol->target.capacity = meta->capacity; if (meta->encryption) { - vol->target.encryption = meta->encryption; - meta->encryption = NULL; + vol->target.encryption = g_steal_pointer(&meta->encryption); } - vol->target.features = meta->features; - meta->features = NULL; - vol->target.compat = meta->compat; - meta->compat = NULL; + vol->target.features = g_steal_pointer(&meta->features); + vol->target.compat = g_steal_pointer(&meta->compat); *volptr = g_steal_pointer(&vol); ret = 0; diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a680f14afc..f88d225e96 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3443,8 +3443,7 @@ storageBackendProbeTarget(virStorageSource *target, target->backingStore = virStorageSourceNew(); target->backingStore->type = VIR_STORAGE_TYPE_NETWORK; - target->backingStore->path = meta->backingStoreRaw; - meta->backingStoreRaw = NULL; + target->backingStore->path = g_steal_pointer(&meta->backingStoreRaw); target->backingStore->format = VIR_STORAGE_FILE_RAW; } @@ -3479,8 +3478,7 @@ storageBackendProbeTarget(virStorageSource *target, if (meta->encryption->payload_offset != -1) target->capacity -= meta->encryption->payload_offset * 512; - *encryption = meta->encryption; - meta->encryption = NULL; + *encryption = g_steal_pointer(&meta->encryption); /* XXX ideally we'd fill in secret UUID here * but we cannot guarantee 'conn' is non-NULL diff --git a/src/util/virfile.c b/src/util/virfile.c index d6faf7e3d2..2d9c89ebbe 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -967,8 +967,7 @@ int virFileNBDDeviceAssociate(const char *file, VIR_DEBUG("Associated NBD device %s with file %s and format %s", nbddev, file, fmtstr); - *dev = nbddev; - nbddev = NULL; + *dev = g_steal_pointer(&nbddev); return 0; } diff --git a/src/util/virlease.c b/src/util/virlease.c index 0e28a47dd2..48b0f6fe57 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -254,7 +254,6 @@ virLeaseNew(virJSONValue **lease_ret, if (virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0) return -1; - *lease_ret = lease_new; - lease_new = NULL; + *lease_ret = g_steal_pointer(&lease_new); return 0; } diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 7c892d6267..2306ab0cb1 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -839,16 +839,13 @@ virNumaGetPages(int node, } while (exchange); if (pages_size) { - *pages_size = tmp_size; - tmp_size = NULL; + *pages_size = g_steal_pointer(&tmp_size); } if (pages_avail) { - *pages_avail = tmp_avail; - tmp_avail = NULL; + *pages_avail = g_steal_pointer(&tmp_avail); } if (pages_free) { - *pages_free = tmp_free; - tmp_free = NULL; + *pages_free = g_steal_pointer(&tmp_free); } *npages = ntmp; return 0; -- 2.34.1

On 2/1/22 17:18, Erik Skultety wrote: Yeah, hyperv code doesn't get as much love as other drivers, but basically, @domains is allocated iff @doms != NULL, there's the following code earlier in the function: if (domains) { doms = g_new0(virDomainPtr, 1); ndoms = 1; } I find the new version easier to read, since @domains is an argument of the function. Do you want me to post a separate patch that does s/doms/domains/? Michal

On 2/1/22 17:59, Erik Skultety wrote: It's not, but I think it's trivial enough to be contained in this commit. But alright, let me keep the original condition and push. This is unrelated. Worth pushing, but unrelated. Note, there are three variables: doms, domain, and domains. My patch touches only the first and the last one. Michal