9:22 a.m.
*** BLURB HERE ***
Michal Privoznik (8):
virDomainDefFree: Don't leak initenv name and value
virDomainDefParseXML: Free @tmp
virNodeDevCapCCWParseXML: Free temporary variables
qemuDomainObjPrivateFree: Free @machineName
virVMXParseConfig: Don't leak def->videos
virTestCompareToFile: Don't access memory we don't own
qemuhotplugtest: Don't leak @vm
virdbustest: Don't leak @out_strv1
src/conf/domain_conf.c | 6 +++++-
src/conf/node_device_conf.c | 3 +++
src/qemu/qemu_domain.c | 1 +
src/vmx/vmx.c | 9 ++++++---
tests/qemuhotplugtest.c | 1 +
tests/testutils.c | 1 +
tests/virdbustest.c | 1 +
7 files changed, 18 insertions(+), 4 deletions(-)
--
2.13.0
9:22 a.m.
New subject: [libvirt] [PATCH 1/8] virDomainDefFree: Don't leak initenv name and value
When parsing boot options from domain XML in
virDomainDefParseBootOptions() initenv id stored to:
def->os.initenv[i]->name
def->os.initenv[i]->value
But these are never freed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 640f29d3e..8168dc52f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2931,8 +2931,11 @@ void virDomainDefFree(virDomainDefPtr def)
for (i = 0; def->os.initargv && def->os.initargv[i]; i++)
VIR_FREE(def->os.initargv[i]);
VIR_FREE(def->os.initargv);
- for (i = 0; def->os.initenv && def->os.initenv[i]; i++)
+ for (i = 0; def->os.initenv && def->os.initenv[i]; i++) {
+ VIR_FREE(def->os.initenv[i]->name);
+ VIR_FREE(def->os.initenv[i]->value);
VIR_FREE(def->os.initenv[i]);
+ }
VIR_FREE(def->os.initdir);
VIR_FREE(def->os.inituser);
VIR_FREE(def->os.initgroup);
--
2.13.0
9:53 a.m.
New subject: [libvirt] [PATCH 1/8] virDomainDefFree: Don't leak initenv name and value
On Fri, Aug 04, 2017 at 16:22:29 +0200, Michal Privoznik wrote:
When parsing boot options from domain XML in
virDomainDefParseBootOptions() initenv id stored to:
In this function there's a very shady approach to extract the contents
of the XML element.
def->os.initenv[i]->name
def->os.initenv[i]->value
But these are never freed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 640f29d3e..8168dc52f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2931,8 +2931,11 @@ void virDomainDefFree(virDomainDefPtr def)
for (i = 0; def->os.initargv && def->os.initargv[i]; i++)
VIR_FREE(def->os.initargv[i]);
VIR_FREE(def->os.initargv);
- for (i = 0; def->os.initenv && def->os.initenv[i]; i++)
+ for (i = 0; def->os.initenv && def->os.initenv[i]; i++) {
+ VIR_FREE(def->os.initenv[i]->name);
+ VIR_FREE(def->os.initenv[i]->value);
VIR_FREE(def->os.initenv[i]);
ACK to this though
9:22 a.m.
New subject: [libvirt] [PATCH 2/8] virDomainDefParseXML: Free @tmp
When parsing <ioapic> feature we're using @tmp to store some
temporary string but never free it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8168dc52f..3cdb5e348 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17892,6 +17892,7 @@ virDomainDefParseXML(xmlDocPtr xml,
}
def->ioapic = value;
def->features[val] = VIR_TRISTATE_SWITCH_ON;
+ VIR_FREE(tmp);
}
ctxt->node = node;
break;
--
2.13.0
9:42 a.m.
New subject: [libvirt] [PATCH 2/8] virDomainDefParseXML: Free @tmp
On Fri, Aug 04, 2017 at 16:22:30 +0200, Michal Privoznik wrote:
When parsing <ioapic> feature we're using @tmp to store
some
temporary string but never free it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 1 +
1 file changed, 1 insertion(+)
ACK
9:22 a.m.
New subject: [libvirt] [PATCH 3/8] virNodeDevCapCCWParseXML: Free temporary variables
Again, we are using @cssid, @ssid and @devno to store some
temporary strings, but never free it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/node_device_conf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 82f02fa6c..726bf042c 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -792,6 +792,9 @@ virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
out:
ctxt->node = orignode;
+ VIR_FREE(cssid);
+ VIR_FREE(ssid);
+ VIR_FREE(devno);
return ret;
}
--
2.13.0
9:44 a.m.
New subject: [libvirt] [PATCH 3/8] virNodeDevCapCCWParseXML: Free temporary variables
On Fri, Aug 04, 2017 at 16:22:31 +0200, Michal Privoznik wrote:
Again, we are using @cssid, @ssid and @devno to store some
temporary strings, but never free it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/node_device_conf.c | 3 +++
1 file changed, 3 insertions(+)
ACK
9:22 a.m.
New subject: [libvirt] [PATCH 4/8] qemuDomainObjPrivateFree: Free @machineName
We're storing the machine name in @priv but free it just in
qemuProcessStop, Therefore this may leak.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index fe27e1122..40608554c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1722,6 +1722,7 @@ qemuDomainObjPrivateFree(void *data)
virBitmapFree(priv->autoNodeset);
virBitmapFree(priv->autoCpuset);
+ VIR_FREE(priv->machineName);
VIR_FREE(priv->libDir);
VIR_FREE(priv->channelTargetDir);
--
2.13.0
9:43 a.m.
New subject: [libvirt] [PATCH 4/8] qemuDomainObjPrivateFree: Free @machineName
On Fri, Aug 04, 2017 at 16:22:32 +0200, Michal Privoznik wrote:
We're storing the machine name in @priv but free it just in
qemuProcessStop, Therefore this may leak.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 1 +
1 file changed, 1 insertion(+)
ACK
9:22 a.m.
New subject: [libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos
This function calls virDomainDefAddImplicitDevices() which adds
implicit video device to domain definition. However, later in the
process the function just ignores this and overwrites the @videos
array without prior free.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 96507f10f..858bc090d 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1305,6 +1305,7 @@ virVMXParseConfig(virVMXContext *ctx,
long long sharedFolder_maxNum = 0;
int cpumasklen;
char *namespaceData;
+ size_t i;
if (ctx->parseFileName == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1761,11 +1762,13 @@ virVMXParseConfig(virVMXContext *ctx,
/* FIXME */
/* def:videos */
- if (VIR_ALLOC_N(def->videos, 1) < 0)
- goto cleanup;
-
+ for (i = 0; i < def->nvideos; i++)
+ virDomainVideoDefFree(def->videos[i]);
def->nvideos = 0;
+ if (VIR_REALLOC_N(def->videos, 1) < 0)
+ goto cleanup;
+
if (virVMXParseSVGA(conf, &def->videos[def->nvideos]) < 0)
goto cleanup;
--
2.13.0
9:31 a.m.
New subject: [libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos
On 08/04/2017 04:22 PM, Michal Privoznik wrote:
This function calls virDomainDefAddImplicitDevices() which adds
implicit video device to domain definition. However, later in the
process the function just ignores this and overwrites the @videos
array without prior free.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 96507f10f..858bc090d 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1305,6 +1305,7 @@ virVMXParseConfig(virVMXContext *ctx,
long long sharedFolder_maxNum = 0;
int cpumasklen;
char *namespaceData;
+ size_t i;
if (ctx->parseFileName == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1761,11 +1762,13 @@ virVMXParseConfig(virVMXContext *ctx,
/* FIXME */
/* def:videos */
- if (VIR_ALLOC_N(def->videos, 1) < 0)
- goto cleanup;
-
+ for (i = 0; i < def->nvideos; i++)
+ virDomainVideoDefFree(def->videos[i]);
def->nvideos = 0;
+ if (VIR_REALLOC_N(def->videos, 1) < 0)
+ goto cleanup;
+
if (virVMXParseSVGA(conf, &def->videos[def->nvideos]) < 0)
goto cleanup;
Oh, forgot to push this into the commit:
diff --git i/src/vmx/vmx.c w/src/vmx/vmx.c
index 858bc090d..d111b1ef1 100644
--- i/src/vmx/vmx.c
+++ w/src/vmx/vmx.c
@@ -3035,7 +3035,7 @@ virVMXParseSVGA(virConfPtr conf, virDomainVideoDefPtr *def)
int result = -1;
long long svga_vramSize = 0;
- if (def == NULL || *def != NULL) {
+ if (def == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid
argument"));
return -1;
}
Consider it squashed in locally.
Michal
9:56 a.m.
New subject: [libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos
On Fri, Aug 04, 2017 at 04:22:33PM +0200, Michal Privoznik wrote:
This function calls virDomainDefAddImplicitDevices() which adds
implicit video device to domain definition. However, later in the
process the function just ignores this and overwrites the @videos
array without prior free.
We should not be calling virDomainDefAddImplicitDevices before
all the explicit devices are added to the domain definition.
What is the point of adding a device just to free it a few lines later?
Jan
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/vmx/vmx.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
4:21 a.m.
New subject: [libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos
On 08/04/2017 04:56 PM, Ján Tomko wrote:
On Fri, Aug 04, 2017 at 04:22:33PM +0200, Michal Privoznik wrote:
> This function calls virDomainDefAddImplicitDevices() which adds
> implicit video device to domain definition. However, later in the
> process the function just ignores this and overwrites the @videos
> array without prior free.
>
We should not be calling virDomainDefAddImplicitDevices before
all the explicit devices are added to the domain definition.
What is the point of adding a device just to free it a few lines later?
Because the parsing code expects some controllers to be in place before
it gets to video devices. Just look around the place where
virDomainDefAddImplicitDevices() is called. If you have a bright idea
how to fix it I'm all ears.
Michal
9:22 a.m.
New subject: [libvirt] [PATCH 6/8] virTestCompareToFile: Don't access memory we don't own
After reading the contents of a file some cleanup is performed.
However, the check for it might access a byte outside of the
string - if the file is empty in the first place. Then strlen()
is zero.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/testutils.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/testutils.c b/tests/testutils.c
index 71692f1fa..4bb6140ec 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -796,6 +796,7 @@ virTestCompareToFile(const char *strcontent,
goto failure;
if (filecontent &&
+ strlen(filecontent) > 0 &&
filecontent[strlen(filecontent) - 1] == '\n' &&
strcontent[strlen(strcontent) - 1] != '\n') {
if (virAsprintf(&fixedcontent, "%s\n", strcontent) < 0)
--
2.13.0
9:39 a.m.
New subject: [libvirt] [PATCH 6/8] virTestCompareToFile: Don't access memory we don't own
On Fri, Aug 04, 2017 at 16:22:34 +0200, Michal Privoznik wrote:
After reading the contents of a file some cleanup is performed.
However, the check for it might access a byte outside of the
string - if the file is empty in the first place. Then strlen()
is zero.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/testutils.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/testutils.c b/tests/testutils.c
index 71692f1fa..4bb6140ec 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -796,6 +796,7 @@ virTestCompareToFile(const char *strcontent,
goto failure;
if (filecontent &&
+ strlen(filecontent) > 0 &&
I'd store the length in a variable ...
filecontent[strlen(filecontent) - 1] == '\n'
&&
... so that it's not evaluated twice.
strcontent[strlen(strcontent) - 1] != '\n') {
if (virAsprintf(&fixedcontent, "%s\n", strcontent) < 0)
ACK with that.
9:22 a.m.
New subject: [libvirt] [PATCH 7/8] qemuhotplugtest: Don't leak @vm
Some tests take already prepared domain from previous tests. In
this case, the domain is freed by the first test that doesn't
keep the domain. However, if there's no such test case domain is
leaked.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/qemuhotplugtest.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 79f032690..23a498617 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -855,6 +855,7 @@ mymain(void)
DO_TEST_CPU_INDIVIDUAL("ppc64-modern-individual", "17", true,
true, true);
qemuTestDriverFree(&driver);
+ virObjectUnref(data.vm);
return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
2.13.0
9:48 a.m.
New subject: [libvirt] [PATCH 7/8] qemuhotplugtest: Don't leak @vm
On Fri, Aug 04, 2017 at 16:22:35 +0200, Michal Privoznik wrote:
Some tests take already prepared domain from previous tests. In
this case, the domain is freed by the first test that doesn't
keep the domain. However, if there's no such test case domain is
leaked.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/qemuhotplugtest.c | 1 +
1 file changed, 1 insertion(+)
ACK
9:22 a.m.
New subject: [libvirt] [PATCH 8/8] virdbustest: Don't leak @out_strv1
In testMessageSingleArrayRef the string is doubly referenced.
Therefore we have to free also the first pointer to the string.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virdbustest.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/virdbustest.c b/tests/virdbustest.c
index 3be9cf17e..b7ddd7791 100644
--- a/tests/virdbustest.c
+++ b/tests/virdbustest.c
@@ -323,6 +323,7 @@ static int testMessageSingleArrayRef(const void *args
ATTRIBUTE_UNUSED)
cleanup:
if (out_strv1)
VIR_FREE(out_strv1[0]);
+ VIR_FREE(out_strv1);
virDBusMessageUnref(msg);
return ret;
}
--
2.13.0
9:50 a.m.
New subject: [libvirt] [PATCH 8/8] virdbustest: Don't leak @out_strv1
On Fri, Aug 04, 2017 at 16:22:36 +0200, Michal Privoznik wrote:
In testMessageSingleArrayRef the string is doubly referenced.
Therefore we have to free also the first pointer to the string.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virdbustest.c | 1 +
1 file changed, 1 insertion(+)
ACK
2664
days inactive
2667
days old
18 comments
3 participants
participants (3)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa