7:07 a.m.
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
7:07 a.m.
New subject: [PATCH 1/9] util: Fix memory leak in virAuthGetCredential
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(a)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
7:07 a.m.
New subject: [PATCH 2/9] util: Fix memory leak in virAuthConfigLookup
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(a)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
7:07 a.m.
New subject: [PATCH 3/9] lxc: Fix memory leak in virLXCControllerPopulateDevices
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(a)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
7:07 a.m.
New subject: [PATCH 4/9] util: Fix memory leak in virPCIProbeStubDriver
Since 9ea90206, @drvpath could be overwritten if we jumped to recheck
Found by Coverity.
Signed-off-by: John Ferlan <jferlan(a)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
7:19 a.m.
New subject: [PATCH 4/9] util: Fix memory leak in virPCIProbeStubDriver
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(a)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
7:07 a.m.
New subject: [PATCH 5/9] test: Fix memory leak in testParseXMLDocFromFile
Since ceb3255c, @absFile could be leaked if we jumped to error.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan(a)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
7:07 a.m.
New subject: [PATCH 6/9] conf: Fix memory leak in openvzWriteConfigParam
Since 60623a7c, @temp_file was not properly free'd on the non error path.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan(a)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
7:07 a.m.
New subject: [PATCH 7/9] conf: Fix memory leak in openvzReadFSConf
Since 1f5deed9, @veid_str has been leaked in the error path.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan(a)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
7:07 a.m.
New subject: [PATCH 8/9] conf: Fix memory leak in virCPUDefParseXML
Since a08669c31, @tsc is not automatically free'd by any g_auto* method.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan(a)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
7:07 a.m.
New subject: [PATCH 9/9] util: Avoid using wrong free function
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(a)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
7:14 a.m.
New subject: [PATCH 9/9] util: Avoid using wrong free function
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.
7:16 a.m.
New subject: [PATCH 9/9] util: Avoid using wrong free function
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 :|
8:25 a.m.
New subject: [PATCH 9/9] util: Avoid using wrong free function
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
7:25 a.m.
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(a)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
8:38 a.m.
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(a)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
>
1620
days inactive
1620
days old
16 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Peter Krempa