[PATCH 0/3] Some random fixes
I've tried to run libvirt under valgrind and found two issues. Here are the fixes. Michal Prívozník (3): qemu_interface: Fix @cfg refcounting in qemuInterfacePrepareSlirp() lib: Prefer g_autoptr() declaration of virQEMUDriverConfigPtr qemu_namespace: Don't leak mknod items that are being skipped over src/qemu/qemu_conf.h | 6 +---- src/qemu/qemu_hostdev.c | 4 +-- src/qemu/qemu_interface.c | 11 +++------ src/qemu/qemu_migration.c | 8 ++---- src/qemu/qemu_migration_cookie.c | 4 +-- src/qemu/qemu_namespace.c | 42 +++++++++++++++++++++++--------- tests/domaincapstest.c | 4 +-- tests/qemuxml2xmltest.c | 2 +- 8 files changed, 42 insertions(+), 39 deletions(-) -- 2.26.2
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@redhat.com> --- 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
On 9/4/20 4:24 AM, Michal Privoznik wrote:
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@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
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@redhat.com> --- 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
On 9/4/20 4:24 AM, Michal Privoznik wrote:
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@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
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@redhat.com> --- 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 9/4/20 4:24 AM, Michal Privoznik wrote:
When building and populating domain NS a couple of functions is called
s/is/are/
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@redhat.com> ---
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);
I recall once being reprimanded for calling g_free on a g_autofree() pointer (by someone who said that *they* had been reprimanded for same). I will say Reviewed-by: Laine Stump <laine@redhat.com> but don't want to later be fingered as "the person" who allowed this into the code, so I guess maybe look for a way around that (or a signoff from whoever it was that made the initial "it's bad to manually free a g_autofree pointer" statement). (I don't think it's particularly ugly BTW. Sure, it could probably be done in a cleaner manner somehow, but sometimes things are just messy; you can move the mess, but you can't eliminate it)
+ next = g_strdup(item.target); + + if (!added) + qemuNamespaceMknodItemClear(&item); }
return 0;
On a Friday in 2020, Michal Privoznik wrote:
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@redhat.com> ---
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);
@next gets consumed here. That seems to be the cause of all grief.
- 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;
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.
+ } /* Some other (critical) error. */ return -1; }
+ isLink = S_ISLNK(item.sb.st_mode); +
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.
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;
This change makes sense on its own and can be separated.
}
- next = item.target; + g_free(next);
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.
+ next = g_strdup(item.target); +
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
+ if (!added) + qemuNamespaceMknodItemClear(&item); }
return 0; -- 2.26.2
participants (3)
-
Ján Tomko -
Laine Stump -
Michal Privoznik