In the qemuInterfacePrepareSlirp() function, the qemu driver config is obtained (via virQEMUDriverGetConfig()), but it is never unrefed leading to mangled refcounter. Fixes: 9145b3f1cc334e946b3f9ea45d6c24c868301e6f Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index 10a87a2528..ddd4ee2731 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -643,7 +643,7 @@ qemuSlirpPtr qemuInterfacePrepareSlirp(virQEMUDriverPtr driver, virDomainNetDefPtr net) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(qemuSlirp) slirp = NULL; size_t i; -- 2.26.2

In the past we had to declare @cfg and then explicitly unref it. But now, with glib we can use g_autoptr() which will do the unref automatically and thus is more bulletproof. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_conf.h | 6 +----- src/qemu/qemu_hostdev.c | 4 +--- src/qemu/qemu_interface.c | 9 +++------ src/qemu/qemu_migration.c | 8 ++------ src/qemu/qemu_migration_cookie.c | 4 +--- tests/domaincapstest.c | 4 +--- tests/qemuxml2xmltest.c | 2 +- 7 files changed, 10 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 687829123c..1d7afa8738 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -64,13 +64,9 @@ typedef virQEMUDriverConfig *virQEMUDriverConfigPtr; * being released while they use it. * * eg - * qemuDriverLock(driver); - * virQEMUDriverConfigPtr cfg = virObjectRef(driver->config); - * qemuDriverUnlock(driver); + * g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); * * ...do stuff with 'cfg'.. - * - * virObjectUnref(cfg); */ struct _virQEMUDriverConfig { virObject parent; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index d39f9d7584..721fe5da82 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -405,14 +405,12 @@ qemuHostdevReAttachPCIDevices(virQEMUDriverPtr driver, virDomainHostdevDefPtr *hostdevs, int nhostdevs) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); const char *oldStateDir = cfg->stateDir; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; virHostdevReAttachPCIDevices(hostdev_mgr, QEMU_DRIVER_NAME, name, hostdevs, nhostdevs, oldStateDir); - - virObjectUnref(cfg); } void diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index ddd4ee2731..cbf3d99981 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -261,7 +261,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def, { int ret = -1; char *res_ifname = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP; if (qemuInterfaceIsVnetCompatModel(net)) @@ -290,7 +290,6 @@ qemuInterfaceDirectConnect(virDomainDefPtr def, while (tapfdSize--) VIR_FORCE_CLOSE(tapfd[tapfdSize]); } - virObjectUnref(cfg); return ret; } @@ -413,7 +412,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, int ret = -1; unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; bool template_ifname = false; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); const char *tunpath = "/dev/net/tun"; const char *auditdev = tunpath; @@ -514,7 +513,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, if (template_ifname) VIR_FREE(net->ifname); } - virObjectUnref(cfg); return ret; } @@ -542,7 +540,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def, int ret = -1; unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; bool template_ifname = false; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); const char *tunpath = "/dev/net/tun"; if (net->backend.tap) { @@ -633,7 +631,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def, if (template_ifname) VIR_FREE(net->ifname); } - virObjectUnref(cfg); return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 142faa2cf9..1e80a22db6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3101,11 +3101,9 @@ qemuMigrationSrcConfirm(virQEMUDriverPtr driver, int cancelled) { qemuMigrationJobPhase phase; - virQEMUDriverConfigPtr cfg = NULL; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int ret = -1; - cfg = virQEMUDriverGetConfig(driver); - if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_OUT)) goto cleanup; @@ -3133,7 +3131,6 @@ qemuMigrationSrcConfirm(virQEMUDriverPtr driver, cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(cfg); return ret; } @@ -3928,7 +3925,7 @@ qemuMigrationSrcPerformTunnel(virQEMUDriverPtr driver, { int ret = -1; qemuMigrationSpec spec; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int fds[2] = { -1, -1 }; VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " @@ -3969,7 +3966,6 @@ qemuMigrationSrcPerformTunnel(virQEMUDriverPtr driver, VIR_FORCE_CLOSE(spec.dest.fd.qemu); VIR_FORCE_CLOSE(spec.dest.fd.local); - virObjectUnref(cfg); return ret; } diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index cef2555988..3ea46e1527 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -194,7 +194,7 @@ qemuMigrationCookieGraphicsSpiceAlloc(virQEMUDriverPtr driver, { qemuMigrationCookieGraphicsPtr mig = NULL; const char *listenAddr; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); if (VIR_ALLOC(mig) < 0) goto error; @@ -215,12 +215,10 @@ qemuMigrationCookieGraphicsSpiceAlloc(virQEMUDriverPtr driver, mig->listen = g_strdup(listenAddr); - virObjectUnref(cfg); return mig; error: qemuMigrationCookieGraphicsFree(mig); - virObjectUnref(cfg); return NULL; } diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index b060f47f7f..d2ed820cf9 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -371,7 +371,7 @@ mymain(void) #endif #if WITH_QEMU - virQEMUDriverConfigPtr cfg = virQEMUDriverConfigNew(false, NULL); + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverConfigNew(false, NULL); if (!cfg) return EXIT_FAILURE; @@ -453,8 +453,6 @@ mymain(void) * file has been added, run "VIR_TEST_REGENERATE_OUTPUT=1 ninja test". */ - virObjectUnref(cfg); - virFileWrapperClearPrefixes(); #endif /* WITH_QEMU */ diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6eb008c8d2..39a9da874f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -132,7 +132,7 @@ mymain(void) { int ret = 0; g_autofree char *fakerootdir = NULL; - virQEMUDriverConfigPtr cfg = NULL; + g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(virHashTable) capslatest = NULL; g_autoptr(virConnect) conn = NULL; -- 2.26.2

When building and populating domain NS a couple of functions is called that append paths to a string list. This string list is then inspected, one item at the time by qemuNamespacePrepareOneItem() which gathers all the info for given path (stat buffer, possible link target, ACLs, SELinux label) using qemuNamespaceMknodItemInit(). If the path needs to be created in the domain's private /dev then it's added onto this qemuNamespaceMknodData list which is freed later in the process. But, if the path does not need to be created in the domain's private /dev, then the memory allocated by qemuNamespaceMknodItemInit() is not freed anywhere leading to a leak. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- Honestly, I think this patch looks ugly. Ideas are welcome. src/qemu/qemu_namespace.c | 42 +++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 87f4fd8d58..917e140f6a 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1166,22 +1166,29 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr data, size_t ndevMountsPath) { long ttl = sysconf(_SC_SYMLOOP_MAX); - const char *next = file; + g_autofree char *next = g_strdup(file); size_t i; while (1) { qemuNamespaceMknodItem item = { 0 }; + bool added = false; + bool isLink; int rc; rc = qemuNamespaceMknodItemInit(&item, cfg, vm, next); - if (rc == -2) { - /* @file doesn't exist. We can break here. */ - break; - } else if (rc < 0) { + if (rc < 0) { + qemuNamespaceMknodItemClear(&item); + + if (rc == -2) { + /* @file doesn't exist. We can break here. */ + break; + } /* Some other (critical) error. */ return -1; } + isLink = S_ISLNK(item.sb.st_mode); + if (STRPREFIX(next, QEMU_DEVPREFIX)) { for (i = 0; i < ndevMountsPath; i++) { if (STREQ(devMountsPath[i], "/dev")) @@ -1190,22 +1197,35 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr data, break; } - if (i == ndevMountsPath && - VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 0) - return -1; + if (i == ndevMountsPath) { + if (VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 0) { + qemuNamespaceMknodItemClear(&item); + return -1; + } + added = true; + } } - if (!S_ISLNK(item.sb.st_mode)) + if (!isLink) { + if (!added) + qemuNamespaceMknodItemClear(&item); break; + } if (ttl-- == 0) { + if (!added) + qemuNamespaceMknodItemClear(&item); virReportSystemError(ELOOP, _("Too many levels of symbolic links: %s"), - next); + file); return -1; } - next = item.target; + g_free(next); + next = g_strdup(item.target); + + if (!added) + qemuNamespaceMknodItemClear(&item); } return 0; -- 2.26.2

On a Friday in 2020, Michal Privoznik wrote: @next gets consumed here. That seems to be the cause of all grief. Strictly speaking, there is nothing to free yet because -2 is returned pretty early. And ItemInit could be changed to clean up after itself if it returns -1. If you move the next assignment here, nothing below the APPEND_ELEMENT_COPY needs to access item anymore, so you can switch it to the non-copy version and clear it unconditionally and use g_auto to clear it. This change makes sense on its own and can be separated. Apart from the g_free/g_autofree mixup, this frees previous iteration's item->file, possibly after it has been added to data->items. So we do need to g_strdup it in ItemInit. If you add a second array to hold the thrown away items during this function, you could leave the iterator const char*. Not sure whether that is nicer or not. Jano