[PATCH 0/9] Some Coverity memory leak patches
Even though I changed roles, the cron job still runs. I recently updated to a newer coverity version and it found some new stuff and even more false positives. It's still very unhappy with the usage of GLIB_DEPRECATED_ENUMERATOR_IN_* macros for *glib/gspawn.h and *gobject/gparam.h, but I can work around that. I had to disable the use_after_free type checking because the virObject to GObject change just caused a large number of possible false positives for virObjectUnref usages (coverity cannot keep track of all the counts). Anyway, I had a few spare cycles, so I figured I'd pass along at least the memory leak and usage things that were found. I haven't posted in a while so hopefully I haven't missed some subtlety. FWIW: I no longer have commit access so I'm at the mercy of someone else pushing my patches now. John Ferlan (9): util: Fix memory leak in virAuthGetCredential util: Fix memory leak in virAuthConfigLookup lxc: Fix memory leak in virLXCControllerPopulateDevices util: Fix memory leak in virPCIProbeStubDriver test: Fix memory leak in testParseXMLDocFromFile conf: Fix memory leak in openvzWriteConfigParam conf: Fix memory leak in openvzReadFSConf conf: Fix memory leak in virCPUDefParseXML util: Avoid using wrong free function src/conf/cpu_conf.c | 2 +- src/lxc/lxc_controller.c | 3 ++- src/openvz/openvz_conf.c | 7 ++----- src/remote/remote_driver.c | 2 +- src/test/test_driver.c | 2 +- src/util/iohelper.c | 7 +++++++ src/util/virauth.c | 5 +---- src/util/virauthconfig.c | 4 ++-- src/util/virauthconfig.h | 2 +- src/util/virpci.c | 1 + tests/virauthconfigtest.c | 2 +- 11 files changed, 20 insertions(+), 17 deletions(-) -- 2.25.4
Since 5084091a, @tmp is filled by a g_key_file_get_string which is now an allocated string as opposed to some hash table lookup value, so we need to treat it as so. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 2 +- src/util/virauth.c | 5 +---- src/util/virauthconfig.c | 2 +- src/util/virauthconfig.h | 2 +- tests/virauthconfigtest.c | 2 +- 5 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 0aeab9db27..653c68472a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4097,7 +4097,7 @@ static int remoteAuthFillFromConfig(virConnectPtr conn, } for (ninteract = 0; state->interact[ninteract].id != 0; ninteract++) { - const char *value = NULL; + char *value = NULL; switch (state->interact[ninteract].id) { case SASL_CB_USER: diff --git a/src/util/virauth.c b/src/util/virauth.c index f75e674586..105fca16eb 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -107,7 +107,6 @@ virAuthGetCredential(const char *servicename, char **value) { g_autoptr(virAuthConfig) config = NULL; - const char *tmp; *value = NULL; @@ -121,11 +120,9 @@ virAuthGetCredential(const char *servicename, servicename, hostname, credname, - &tmp) < 0) + value) < 0) return -1; - *value = g_strdup(tmp); - return 0; } diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c index 1c007757c7..8289b28d34 100644 --- a/src/util/virauthconfig.c +++ b/src/util/virauthconfig.c @@ -99,7 +99,7 @@ int virAuthConfigLookup(virAuthConfigPtr auth, const char *service, const char *hostname, const char *credname, - const char **value) + char **value) { g_autofree char *authgroup = NULL; g_autofree char *credgroup = NULL; diff --git a/src/util/virauthconfig.h b/src/util/virauthconfig.h index de28b1ff28..b6f5b5c110 100644 --- a/src/util/virauthconfig.h +++ b/src/util/virauthconfig.h @@ -37,6 +37,6 @@ int virAuthConfigLookup(virAuthConfigPtr auth, const char *service, const char *hostname, const char *credname, - const char **value); + char **value); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virAuthConfig, virAuthConfigFree); diff --git a/tests/virauthconfigtest.c b/tests/virauthconfigtest.c index 20855f004e..a88b453543 100644 --- a/tests/virauthconfigtest.c +++ b/tests/virauthconfigtest.c @@ -42,7 +42,7 @@ struct ConfigLookupData { static int testAuthLookup(const void *args) { const struct ConfigLookupData *data = args; - const char *actual = NULL; + g_autofree char *actual = NULL; int rv; rv = virAuthConfigLookup(data->config, -- 2.25.4
Since 5084091a, @authcred is filled by a g_key_file_get_string which is now an allocated string as opposed to some hash table lookup value, so we need to treat it as so. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virauthconfig.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c index 8289b28d34..2e50609531 100644 --- a/src/util/virauthconfig.c +++ b/src/util/virauthconfig.c @@ -103,7 +103,7 @@ int virAuthConfigLookup(virAuthConfigPtr auth, { g_autofree char *authgroup = NULL; g_autofree char *credgroup = NULL; - const char *authcred; + g_autofree char *authcred = NULL; *value = NULL; -- 2.25.4
Since 5b82f7f3, @path should have been placed inside the for loop since it'd need to be free'd for each pass through the loop; otherwise, we'd leak like a sieve. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_controller.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 4672920574..734ac73210 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1474,7 +1474,6 @@ static int virLXCControllerSetupDev(virLXCControllerPtr ctrl) static int virLXCControllerPopulateDevices(virLXCControllerPtr ctrl) { size_t i; - g_autofree char *path = NULL; const struct { int maj; int min; @@ -1494,6 +1493,8 @@ static int virLXCControllerPopulateDevices(virLXCControllerPtr ctrl) /* Populate /dev/ with a few important bits */ for (i = 0; i < G_N_ELEMENTS(devs); i++) { + g_autofree char *path = NULL; + path = g_strdup_printf("/%s/%s.dev/%s", LXC_STATE_DIR, ctrl->def->name, devs[i].path); -- 2.25.4
Since 9ea90206, @drvpath could be overwritten if we jumped to recheck Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virpci.c b/src/util/virpci.c index 6c7e6bbcab..16936c4be9 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1018,6 +1018,7 @@ virPCIProbeStubDriver(virPCIStubDriver driver) goto cleanup; } + g_free(drvpath); goto recheck; } -- 2.25.4
On Tue, Jun 16, 2020 at 08:07:05 -0400, John Ferlan wrote:
Since 9ea90206, @drvpath could be overwritten if we jumped to recheck
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 6c7e6bbcab..16936c4be9 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1018,6 +1018,7 @@ virPCIProbeStubDriver(virPCIStubDriver driver) goto cleanup; }
+ g_free(drvpath);
The variable is reused so please use VIR_FREE as g_free doesn't clear the pointer.
goto recheck; }
-- 2.25.4
Since ceb3255c, @absFile could be leaked if we jumped to error. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7a1db21718..993f405f3c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -781,7 +781,7 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file, const char *type) { xmlNodePtr ret = NULL; xmlDocPtr doc = NULL; - char *absFile = NULL; + g_autofree char *absFile = NULL; g_autofree char *relFile = NULL; if ((relFile = virXMLPropString(node, "file"))) { -- 2.25.4
Since 60623a7c, @temp_file was not properly free'd on the non error path. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 78547b8b28..c06bfa13e3 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -616,7 +616,7 @@ int openvzLoadDomains(struct openvz_driver *driver) static int openvzWriteConfigParam(const char * conf_file, const char *param, const char *value) { - char * temp_file = NULL; + g_autofree char *temp_file = NULL; int temp_fd = -1; FILE *fp; char *line = NULL; @@ -666,7 +666,6 @@ openvzWriteConfigParam(const char * conf_file, const char *param, const char *va VIR_FORCE_CLOSE(temp_fd); if (temp_file) unlink(temp_file); - VIR_FREE(temp_file); return -1; } -- 2.25.4
Since 1f5deed9, @veid_str has been leaked in the error path. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index c06bfa13e3..190c57b622 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -330,7 +330,7 @@ openvzReadFSConf(virDomainDefPtr def, { int ret; virDomainFSDefPtr fs = NULL; - char *veid_str = NULL; + g_autofree char *veid_str = NULL; char *temp = NULL; const char *param; unsigned long long barrier, limit; @@ -365,8 +365,6 @@ openvzReadFSConf(virDomainDefPtr def, fs->type = VIR_DOMAIN_FS_TYPE_MOUNT; if (!(fs->src->path = virStringReplace(temp, "$VEID", veid_str))) goto error; - - VIR_FREE(veid_str); } fs->dst = g_strdup("/"); -- 2.25.4
Since a08669c31, @tsc is not automatically free'd by any g_auto* method. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/cpu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b40737e407..e1b0a5653f 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -335,7 +335,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, g_autofree char *vendor_id = NULL; g_autofree char *tscScaling = NULL; g_autofree char *migratable = NULL; - virHostCPUTscInfoPtr tsc = NULL; + g_autofree virHostCPUTscInfoPtr tsc = NULL; *cpu = NULL; -- 2.25.4
Since 1e2ae2e31, changes to use the automagic free logic didn't take into account that one path uses posix_memalign and the other uses VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free() to free the memory. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/iohelper.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 342bae229b..64b7a13f61 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -45,7 +45,11 @@ static int runIO(const char *path, int fd, int oflags) { +#if HAVE_POSIX_MEMALIGN + void *base = NULL; /* Location to be freed */ +#else g_autofree void *base = NULL; /* Location to be freed */ +#endif char *buf = NULL; /* Aligned location within base */ size_t buflen = 1024*1024; intptr_t alignMask = 64*1024 - 1; @@ -168,6 +172,9 @@ runIO(const char *path, int fd, int oflags) ret = 0; cleanup: +#if HAVE_POSIX_MEMALIGN + VIR_FREE(base); +#endif if (VIR_CLOSE(fd) < 0 && ret == 0) { virReportSystemError(errno, _("Unable to close %s"), path); -- 2.25.4
On Tue, Jun 16, 2020 at 08:07:10 -0400, John Ferlan wrote:
Since 1e2ae2e31, changes to use the automagic free logic didn't take into account that one path uses posix_memalign and the other uses VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free() to free the memory.
I don't really think this makes sense. VIR_FREE is now using g_clear_pointer(&PTR, g_free) internally, so g_free will be used regardless. Is the problem really in g_free or rather some kind of problem with the __attribute__(cleanup)) feature? Additionally this fix looks weird. I'd prefer if autofree is dropped here and VIR_FREE is used directly in both cases if this in fact fixes the problem as there is no reason to keep g_autofree just in one case.
On Tue, Jun 16, 2020 at 08:07:10AM -0400, John Ferlan wrote:
Since 1e2ae2e31, changes to use the automagic free logic didn't take into account that one path uses posix_memalign and the other uses VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free() to free the memory.
VIR_FREE calls g_free, so they're semantically identical Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/16/20 8:16 AM, Daniel P. Berrangé wrote:
On Tue, Jun 16, 2020 at 08:07:10AM -0400, John Ferlan wrote:
Since 1e2ae2e31, changes to use the automagic free logic didn't take into account that one path uses posix_memalign and the other uses VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free() to free the memory.
VIR_FREE calls g_free, so they're semantically identical
Hmm... this one was something that had been on my list of false positives (~60 patches) and wait for enough other changes before posting list since March, but I see that reverting it doesn't cause the error any more. So sure dropping is understandable... Tks - John
On Tue, Jun 16, 2020 at 08:07:01 -0400, John Ferlan wrote:
Even though I changed roles, the cron job still runs. I recently updated to a newer coverity version and it found some new stuff and even more false positives. It's still very unhappy with the usage of GLIB_DEPRECATED_ENUMERATOR_IN_* macros for *glib/gspawn.h and *gobject/gparam.h, but I can work around that. I had to disable the use_after_free type checking because the virObject to GObject change just caused a large number of possible false positives for virObjectUnref usages (coverity cannot keep track of all the counts).
Anyway, I had a few spare cycles, so I figured I'd pass along at least the memory leak and usage things that were found. I haven't posted in a while so hopefully I haven't missed some subtlety.
FWIW: I no longer have commit access so I'm at the mercy of someone else pushing my patches now.
John Ferlan (9): util: Fix memory leak in virAuthGetCredential util: Fix memory leak in virAuthConfigLookup lxc: Fix memory leak in virLXCControllerPopulateDevices util: Fix memory leak in virPCIProbeStubDriver test: Fix memory leak in testParseXMLDocFromFile conf: Fix memory leak in openvzWriteConfigParam conf: Fix memory leak in openvzReadFSConf conf: Fix memory leak in virCPUDefParseXML
Patches 1-8: Reviewed-by: Peter Krempa <pkrempa@redhat.com> And I'll push them shortly with the fix mentioned in one of them.
util: Avoid using wrong free function
src/conf/cpu_conf.c | 2 +- src/lxc/lxc_controller.c | 3 ++- src/openvz/openvz_conf.c | 7 ++----- src/remote/remote_driver.c | 2 +- src/test/test_driver.c | 2 +- src/util/iohelper.c | 7 +++++++ src/util/virauth.c | 5 +---- src/util/virauthconfig.c | 4 ++-- src/util/virauthconfig.h | 2 +- src/util/virpci.c | 1 + tests/virauthconfigtest.c | 2 +- 11 files changed, 20 insertions(+), 17 deletions(-)
-- 2.25.4
On 6/16/20 8:25 AM, Peter Krempa wrote:
On Tue, Jun 16, 2020 at 08:07:01 -0400, John Ferlan wrote:
Even though I changed roles, the cron job still runs. I recently updated to a newer coverity version and it found some new stuff and even more false positives. It's still very unhappy with the usage of GLIB_DEPRECATED_ENUMERATOR_IN_* macros for *glib/gspawn.h and *gobject/gparam.h, but I can work around that. I had to disable the use_after_free type checking because the virObject to GObject change just caused a large number of possible false positives for virObjectUnref usages (coverity cannot keep track of all the counts).
Anyway, I had a few spare cycles, so I figured I'd pass along at least the memory leak and usage things that were found. I haven't posted in a while so hopefully I haven't missed some subtlety.
FWIW: I no longer have commit access so I'm at the mercy of someone else pushing my patches now.
John Ferlan (9): util: Fix memory leak in virAuthGetCredential util: Fix memory leak in virAuthConfigLookup lxc: Fix memory leak in virLXCControllerPopulateDevices util: Fix memory leak in virPCIProbeStubDriver test: Fix memory leak in testParseXMLDocFromFile conf: Fix memory leak in openvzWriteConfigParam conf: Fix memory leak in openvzReadFSConf conf: Fix memory leak in virCPUDefParseXML
Patches 1-8:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
And I'll push them shortly with the fix mentioned in one of them.
OK - thanks! John I see why I commented the way I did for @absFile <sigh>, but yeah it's a problem either way... Probably was more focused on the fact it was my original blunder...
util: Avoid using wrong free function
src/conf/cpu_conf.c | 2 +- src/lxc/lxc_controller.c | 3 ++- src/openvz/openvz_conf.c | 7 ++----- src/remote/remote_driver.c | 2 +- src/test/test_driver.c | 2 +- src/util/iohelper.c | 7 +++++++ src/util/virauth.c | 5 +---- src/util/virauthconfig.c | 4 ++-- src/util/virauthconfig.h | 2 +- src/util/virpci.c | 1 + tests/virauthconfigtest.c | 2 +- 11 files changed, 20 insertions(+), 17 deletions(-)
-- 2.25.4
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Peter Krempa