11:57 p.m.
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity.
Alex Jia (6):
src/conf/domain_conf.c: Plug memory leak on virDomainDefParseXML() error path
src/qemu/qemu_process.c: Plug memory leak on qemuProcessWaitForMonitor() error path
src/rpc/virnetclient.c: Plug memory leak on virNetClientSendInternal() error path
src/uml/uml_driver.c: Plug memory leak on umlStartVMDaemon() error path
src/util/virnetdevbridge.c: Plug memory leak on virNetDevBridgeGet() sucessful path
src/util/virnetdevmacvlan.c: Plug memory leak on
virNetDevMacVLanCreateWithVPortProfile() error path
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/conf/domain_conf.c | 1 +
src/qemu/qemu_process.c | 1 +
src/rpc/virnetclient.c | 1 +
src/uml/uml_driver.c | 1 +
src/util/virnetdevbridge.c | 1 +
src/util/virnetdevmacvlan.c | 1 +
6 files changed, 6 insertions(+), 0 deletions(-)
11:57 p.m.
New subject: [libvirt] [PATCH 1/6] conf: Plug memory leak on virDomainDefParseXML() error path
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 0873b68.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/conf/domain_conf.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d50a5c7..1559de2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7468,6 +7468,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
if (i != 0) {
virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Only the first console can be a serial
port"));
+ virDomainChrDefFree(chr);
goto error;
}
--
1.7.1
4:06 p.m.
New subject: [libvirt] [PATCH 1/6] conf: Plug memory leak on virDomainDefParseXML() error path
On 11/29/2011 10:57 PM, ajia(a)redhat.com wrote:
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 0873b68.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/conf/domain_conf.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d50a5c7..1559de2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7468,6 +7468,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
if (i != 0) {
virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Only the first console can be a serial
port"));
+ virDomainChrDefFree(chr);
goto error;
ACK and pushed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:57 p.m.
New subject: [libvirt] [PATCH 2/6] qemu: Plug memory leak on qemuProcessWaitForMonitor() error path
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 109efd7.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/qemu/qemu_process.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2563f97..f3f44ca 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1224,6 +1224,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver,
if (VIR_ALLOC_N(buf, buf_size) < 0) {
virReportOOMError();
+ VIR_FORCE_CLOSE(logfd);
return -1;
}
--
1.7.1
12:20 a.m.
New subject: [libvirt] [PATCH 2/6] qemu: Plug memory leak on qemuProcessWaitForMonitor() error path
At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 109efd7.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/qemu/qemu_process.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2563f97..f3f44ca 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1224,6 +1224,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver,
if (VIR_ALLOC_N(buf, buf_size) < 0) {
virReportOOMError();
+ VIR_FORCE_CLOSE(logfd);
return -1;
I think it is better to goto closelog
Thanks
Wen Congyang
}
12:32 a.m.
New subject: [libvirt] [PATCH 2/6] qemu: Plug memory leak on qemuProcessWaitForMonitor() error path
On 11/30/2011 02:20 PM, Wen Congyang wrote:
At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
> From: Alex Jia<ajia(a)redhat.com>
>
> Detected by Coverity. Leak introduced in commit 109efd7.
>
> Signed-off-by: Alex Jia<ajia(a)redhat.com>
> ---
> src/qemu/qemu_process.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 2563f97..f3f44ca 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1224,6 +1224,7 @@ qemuProcessWaitForMonitor(struct qemud_driver* driver,
>
> if (VIR_ALLOC_N(buf, buf_size)< 0) {
> virReportOOMError();
> + VIR_FORCE_CLOSE(logfd);
> return -1;
I think it is better to goto closelog
Yeah, I think so before, but
'closelog' label will free 'buf', in fact,
we haven't successfully allocate
memory to 'buf' variable, I'm not sure whether it is a issue. maybe, the
above is a simple way, otherwise, it should be better if we add a check
for 'buf' variable in 'closelog' label, it looks like this:
...
if (buf)
VIR_FREE(buf);
...
Chongyang, please correct me if I'm wrong :)
Thanks,
Alex
Thanks
Wen Congyang
> }
12:41 a.m.
New subject: [libvirt] [PATCH 2/6] qemu: Plug memory leak on qemuProcessWaitForMonitor() error path
At 11/30/2011 02:32 PM, Alex Jia Write:
On 11/30/2011 02:20 PM, Wen Congyang wrote:
> At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
>> From: Alex Jia<ajia(a)redhat.com>
>>
>> Detected by Coverity. Leak introduced in commit 109efd7.
>>
>> Signed-off-by: Alex Jia<ajia(a)redhat.com>
>> ---
>> src/qemu/qemu_process.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 2563f97..f3f44ca 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -1224,6 +1224,7 @@ qemuProcessWaitForMonitor(struct qemud_driver*
>> driver,
>>
>> if (VIR_ALLOC_N(buf, buf_size)< 0) {
>> virReportOOMError();
>> + VIR_FORCE_CLOSE(logfd);
>> return -1;
> I think it is better to goto closelog
Yeah, I think so before, but 'closelog' label will free 'buf', in fact,
we haven't successfully allocate
buf is inited to NULL, so it is safe to use VIR_FREE(buf)
memory to 'buf' variable, I'm not sure whether it is a
issue. maybe, the
above is a simple way, otherwise, it should be better if we add a check
for 'buf' variable in 'closelog' label, it looks like this:
...
if (buf)
VIR_FREE(buf);
If buf is NULL, VIR_FREE(bus) does nothing and is safe.
If you add a check for 'buf', make syntax-check will fail.
Thansk
Wen Congyang
...
Chongyang, please correct me if I'm wrong :)
Thanks,
Alex
> Thanks
> Wen Congyang
>
>> }
>
12:43 a.m.
New subject: [libvirt] [PATCH 2/6] qemu: Plug memory leak on qemuProcessWaitForMonitor() error path
On 11/30/2011 02:41 PM, Wen Congyang wrote:
At 11/30/2011 02:32 PM, Alex Jia Write:
> On 11/30/2011 02:20 PM, Wen Congyang wrote:
>> At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
>>> From: Alex Jia<ajia(a)redhat.com>
>>>
>>> Detected by Coverity. Leak introduced in commit 109efd7.
>>>
>>> Signed-off-by: Alex Jia<ajia(a)redhat.com>
>>> ---
>>> src/qemu/qemu_process.c | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 2563f97..f3f44ca 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -1224,6 +1224,7 @@ qemuProcessWaitForMonitor(struct qemud_driver*
>>> driver,
>>>
>>> if (VIR_ALLOC_N(buf, buf_size)< 0) {
>>> virReportOOMError();
>>> + VIR_FORCE_CLOSE(logfd);
>>> return -1;
>> I think it is better to goto closelog
> Yeah, I think so before, but 'closelog' label will free 'buf', in
fact,
> we haven't successfully allocate
buf is inited to NULL, so it is safe to use VIR_FREE(buf)
Agree, will send v2 patch
based on your advice.
> memory to 'buf' variable, I'm not sure whether it is
a issue. maybe, the
> above is a simple way, otherwise, it should be better if we add a check
> for 'buf' variable in 'closelog' label, it looks like this:
> ...
> if (buf)
> VIR_FREE(buf);
If buf is NULL, VIR_FREE(bus) does nothing and is safe.
If you add a check for 'buf', make syntax-check will fail.
Thanks.
Thansk
Wen Congyang
> ...
>
> Chongyang, please correct me if I'm wrong :)
>
> Thanks,
> Alex
>> Thanks
>> Wen Congyang
>>
>>> }
>
11:57 p.m.
New subject: [libvirt] [PATCH 3/6] rpc: Plug memory leak on virNetClientSendInternal() error path
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 673adba.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/rpc/virnetclient.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index a738129..9ed03a5 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1740,6 +1740,7 @@ cleanup:
unlock:
virNetClientUnlock(client);
+ VIR_FREE(call);
return ret;
}
--
1.7.1
12:26 a.m.
New subject: [libvirt] [PATCH 3/6] rpc: Plug memory leak on virNetClientSendInternal() error path
At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 673adba.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/rpc/virnetclient.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index a738129..9ed03a5 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1740,6 +1740,7 @@ cleanup:
unlock:
virNetClientUnlock(client);
+ VIR_FREE(call);
According to the comment:
/* If partially sent, then the call is still on the dispatch queue */
So I think we should not free call if it is still on the queue.
Thanks
Wen Congyang
return ret;
}
12:40 a.m.
New subject: [libvirt] [PATCH 3/6] rpc: Plug memory leak on virNetClientSendInternal() error path
On 11/30/2011 02:26 PM, Wen Congyang wrote:
At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
> From: Alex Jia<ajia(a)redhat.com>
>
> Detected by Coverity. Leak introduced in commit 673adba.
>
> Signed-off-by: Alex Jia<ajia(a)redhat.com>
> ---
> src/rpc/virnetclient.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index a738129..9ed03a5 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -1740,6 +1740,7 @@ cleanup:
>
> unlock:
> virNetClientUnlock(client);
> + VIR_FREE(call);
According to the comment:
/* If partially sent, then the call is still on the dispatch queue */
So I think we should not free call if it is still on the queue.
Yeah, it's a
inappropriate place, in addition, in 'cleanup' label, if
ret !=1, also will free 'all', then 'unlock' label still do this, the
following should be a right place:
1708 if (!client->sock || client->wantClose) {
1709 virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
1710 _("client socket is closed"));
*1711 VIR_FREE(call);*
1712 goto unlock;
1713 }
Thanks,
Alex
Thanks
Wen Congyang
> return ret;
> }
>
5:11 p.m.
New subject: [libvirt] [PATCH 3/6] rpc: Plug memory leak on virNetClientSendInternal() error path
On 11/29/2011 11:40 PM, Alex Jia wrote:
On 11/30/2011 02:26 PM, Wen Congyang wrote:
> At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
>> From: Alex Jia<ajia(a)redhat.com>
>>
>> Detected by Coverity. Leak introduced in commit 673adba.
>>
> So I think we should not free call if it is still on the queue.
Yeah, it's a inappropriate place, in addition, in 'cleanup' label, if
ret !=1, also will free 'all', then 'unlock' label still do this, the
following should be a right place:
1708 if (!client->sock || client->wantClose) {
1709 virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
1710 _("client socket is closed"));
*1711 VIR_FREE(call);*
1712 goto unlock;
1713 }
Closer, but still not quite right either. You solved the failure if
!client->sock, but still left in the bug that a failed virCondInit did
goto cleanup, which then did a call to virCondDestroy on uninitialized
data, which is undefined by POSIX.
Rearranging some code and consolidating to one label will solve both
bugs (the leak if !client->sock, and the incorrect virCondDestroy call
on virCondInit failure), while still avoiding to free call on a partial
send. Here's what I'm pushing under your name.
diff --git c/src/rpc/virnetclient.c w/src/rpc/virnetclient.c
index a738129..5165c8d 100644
--- c/src/rpc/virnetclient.c
+++ w/src/rpc/virnetclient.c
@@ -1705,13 +1705,13 @@ static int
virNetClientSendInternal(virNetClientPtr client,
virNetClientLock(client);
if (!client->sock || client->wantClose) {
virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
_("client socket is closed"));
- goto unlock;
+ goto cleanup;
}
if (virCondInit(&call->cond) < 0) {
virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot initialize condition variable"));
goto cleanup;
@@ -1726,22 +1726,22 @@ static int
virNetClientSendInternal(virNetClientPtr client,
call->expectReply = expectReply;
call->nonBlock = nonBlock;
call->haveThread = true;
ret = virNetClientIO(client, call);
-cleanup:
/* If partially sent, then the call is still on the dispatch queue */
if (ret == 1) {
call->haveThread = false;
} else {
ignore_value(virCondDestroy(&call->cond));
- VIR_FREE(call);
}
-unlock:
+cleanup:
+ if (ret != 1)
+ VIR_FREE(call);
virNetClientUnlock(client);
return ret;
}
/*
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:01 p.m.
New subject: [libvirt] [PATCH 3/6] rpc: Plug memory leak on virNetClientSendInternal() error path
On 12/01/2011 07:11 AM, Eric Blake wrote:
On 11/29/2011 11:40 PM, Alex Jia wrote:
> On 11/30/2011 02:26 PM, Wen Congyang wrote:
>> At 11/30/2011 01:57 PM, ajia(a)redhat.com Write:
>>> From: Alex Jia<ajia(a)redhat.com>
>>>
>>> Detected by Coverity. Leak introduced in commit 673adba.
>>>
>> So I think we should not free call if it is still on the queue.
> Yeah, it's a inappropriate place, in addition, in 'cleanup' label, if
> ret !=1, also will free 'all', then 'unlock' label still do this,
the
> following should be a right place:
>
> 1708 if (!client->sock || client->wantClose) {
> 1709 virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
> 1710 _("client socket is closed"));
> *1711 VIR_FREE(call);*
> 1712 goto unlock;
> 1713 }
Closer, but still not quite right either. You solved the failure if
!client->sock, but still left in the bug that a failed virCondInit did
goto cleanup, which then did a call to virCondDestroy on uninitialized
data, which is undefined by POSIX.
Rearranging some code and consolidating to one label will solve both
bugs (the leak if !client->sock, and the incorrect virCondDestroy call
on virCondInit failure), while still avoiding to free call on a partial
send. Here's what I'm pushing under your name.
Yeah, I just saw codes
again. Eric, thanks.
diff --git c/src/rpc/virnetclient.c w/src/rpc/virnetclient.c
index a738129..5165c8d 100644
--- c/src/rpc/virnetclient.c
+++ w/src/rpc/virnetclient.c
@@ -1705,13 +1705,13 @@ static int
virNetClientSendInternal(virNetClientPtr client,
virNetClientLock(client);
if (!client->sock || client->wantClose) {
virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
_("client socket is closed"));
- goto unlock;
+ goto cleanup;
}
if (virCondInit(&call->cond)< 0) {
virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot initialize condition variable"));
goto cleanup;
@@ -1726,22 +1726,22 @@ static int
virNetClientSendInternal(virNetClientPtr client,
call->expectReply = expectReply;
call->nonBlock = nonBlock;
call->haveThread = true;
ret = virNetClientIO(client, call);
-cleanup:
/* If partially sent, then the call is still on the dispatch queue */
if (ret == 1) {
call->haveThread = false;
} else {
ignore_value(virCondDestroy(&call->cond));
- VIR_FREE(call);
}
-unlock:
+cleanup:
+ if (ret != 1)
+ VIR_FREE(call);
virNetClientUnlock(client);
return ret;
}
/*
11:57 p.m.
New subject: [libvirt] [PATCH 4/6] uml: Plug memory leak on umlStartVMDaemon() error path
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 8866eed.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/uml/uml_driver.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 073f362..fe6d5ba 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1051,6 +1051,7 @@ static int umlStartVMDaemon(virConnectPtr conn,
VIR_FREE(vm->def->consoles[i]->info.alias);
if (virAsprintf(&vm->def->consoles[i]->info.alias,
"console%zu", i) < 0) {
virReportOOMError();
+ VIR_FORCE_CLOSE(logfd);
goto cleanup;
}
}
--
1.7.1
5:39 p.m.
New subject: [libvirt] [PATCH 4/6] uml: Plug memory leak on umlStartVMDaemon() error path
On 11/29/2011 10:57 PM, ajia(a)redhat.com wrote:
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 8866eed.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/uml/uml_driver.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 073f362..fe6d5ba 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1051,6 +1051,7 @@ static int umlStartVMDaemon(virConnectPtr conn,
VIR_FREE(vm->def->consoles[i]->info.alias);
if (virAsprintf(&vm->def->consoles[i]->info.alias,
"console%zu", i) < 0) {
virReportOOMError();
+ VIR_FORCE_CLOSE(logfd);
goto cleanup;
Good catch, but while reading the function, I found another bug - if we
failed to add the domain to autodestroy, we still returned success but
without setting the domain to transient. I'm pushing this more
comprehensive fix:
diff --git i/src/uml/uml_driver.c w/src/uml/uml_driver.c
index fe6d5ba..faea313 100644
--- i/src/uml/uml_driver.c
+++ w/src/uml/uml_driver.c
@@ -1033,56 +1033,51 @@ static int umlStartVMDaemon(virConnectPtr conn,
/* Do this upfront, so any part of the startup process can add
* runtime state to vm->def that won't be persisted. This let's us
* report implicit runtime defaults in the XML, like vnc listen/socket
*/
VIR_DEBUG("Setting current domain def as transient");
if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) {
VIR_FORCE_CLOSE(logfd);
return -1;
}
- if (!(cmd = umlBuildCommandLine(conn, driver, vm))) {
- VIR_FORCE_CLOSE(logfd);
- virDomainConfVMNWFilterTeardown(vm);
- umlCleanupTapDevices(vm);
- return -1;
- }
+ if (!(cmd = umlBuildCommandLine(conn, driver, vm)))
+ goto cleanup;
for (i = 0 ; i < vm->def->nconsoles ; i++) {
VIR_FREE(vm->def->consoles[i]->info.alias);
if (virAsprintf(&vm->def->consoles[i]->info.alias,
"console%zu", i) < 0) {
virReportOOMError();
- VIR_FORCE_CLOSE(logfd);
goto cleanup;
}
}
virCommandWriteArgLog(cmd, logfd);
priv->monitor = -1;
virCommandClearCaps(cmd);
virCommandSetOutputFD(cmd, &logfd);
virCommandSetErrorFD(cmd, &logfd);
virCommandDaemonize(cmd);
ret = virCommandRun(cmd, NULL);
- VIR_FORCE_CLOSE(logfd);
if (ret < 0)
goto cleanup;
if (autoDestroy &&
- umlProcessAutoDestroyAdd(driver, vm, conn) < 0)
+ (ret = umlProcessAutoDestroyAdd(driver, vm, conn)) < 0)
goto cleanup;
ret = virDomainObjSetDefTransient(driver->caps, vm, false);
cleanup:
+ VIR_FORCE_CLOSE(logfd);
virCommandFree(cmd);
if (ret < 0) {
virDomainConfVMNWFilterTeardown(vm);
umlCleanupTapDevices(vm);
if (vm->newDef) {
virDomainDefFree(vm->def);
vm->def = vm->newDef;
vm->def->id = -1;
vm->newDef = NULL;
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:11 p.m.
New subject: [libvirt] [PATCH 4/6] uml: Plug memory leak on umlStartVMDaemon() error path
On 12/01/2011 07:39 AM, Eric Blake wrote:
On 11/29/2011 10:57 PM, ajia(a)redhat.com wrote:
> From: Alex Jia<ajia(a)redhat.com>
>
> Detected by Coverity. Leak introduced in commit 8866eed.
>
> Signed-off-by: Alex Jia<ajia(a)redhat.com>
> ---
> src/uml/uml_driver.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 073f362..fe6d5ba 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -1051,6 +1051,7 @@ static int umlStartVMDaemon(virConnectPtr conn,
> VIR_FREE(vm->def->consoles[i]->info.alias);
> if (virAsprintf(&vm->def->consoles[i]->info.alias,
"console%zu", i)< 0) {
> virReportOOMError();
> + VIR_FORCE_CLOSE(logfd);
> goto cleanup;
Good catch, but while reading the function, I found another bug - if we
failed to add the domain to autodestroy, we still returned success but
without setting the domain to transient. I'm pushing this more
comprehensive fix:
Indeed, the return value hasn't been stored in 'ret'
if we failed to add
domain
to autodestroy. Eric, thanks.
diff --git i/src/uml/uml_driver.c w/src/uml/uml_driver.c
index fe6d5ba..faea313 100644
--- i/src/uml/uml_driver.c
+++ w/src/uml/uml_driver.c
@@ -1033,56 +1033,51 @@ static int umlStartVMDaemon(virConnectPtr conn,
/* Do this upfront, so any part of the startup process can add
* runtime state to vm->def that won't be persisted. This let's us
* report implicit runtime defaults in the XML, like vnc listen/socket
*/
VIR_DEBUG("Setting current domain def as transient");
if (virDomainObjSetDefTransient(driver->caps, vm, true)< 0) {
VIR_FORCE_CLOSE(logfd);
return -1;
}
- if (!(cmd = umlBuildCommandLine(conn, driver, vm))) {
- VIR_FORCE_CLOSE(logfd);
- virDomainConfVMNWFilterTeardown(vm);
- umlCleanupTapDevices(vm);
- return -1;
- }
+ if (!(cmd = umlBuildCommandLine(conn, driver, vm)))
+ goto cleanup;
for (i = 0 ; i< vm->def->nconsoles ; i++) {
VIR_FREE(vm->def->consoles[i]->info.alias);
if (virAsprintf(&vm->def->consoles[i]->info.alias,
"console%zu", i)< 0) {
virReportOOMError();
- VIR_FORCE_CLOSE(logfd);
goto cleanup;
}
}
virCommandWriteArgLog(cmd, logfd);
priv->monitor = -1;
virCommandClearCaps(cmd);
virCommandSetOutputFD(cmd,&logfd);
virCommandSetErrorFD(cmd,&logfd);
virCommandDaemonize(cmd);
ret = virCommandRun(cmd, NULL);
- VIR_FORCE_CLOSE(logfd);
if (ret< 0)
goto cleanup;
if (autoDestroy&&
- umlProcessAutoDestroyAdd(driver, vm, conn)< 0)
+ (ret = umlProcessAutoDestroyAdd(driver, vm, conn))< 0)
goto cleanup;
ret = virDomainObjSetDefTransient(driver->caps, vm, false);
cleanup:
+ VIR_FORCE_CLOSE(logfd);
virCommandFree(cmd);
if (ret< 0) {
virDomainConfVMNWFilterTeardown(vm);
umlCleanupTapDevices(vm);
if (vm->newDef) {
virDomainDefFree(vm->def);
vm->def = vm->newDef;
vm->def->id = -1;
vm->newDef = NULL;
11:57 p.m.
New subject: [libvirt] [PATCH 5/6] util: Plug memory leak on virNetDevBridgeGet() sucessful path
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit c1df2c1.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/util/virnetdevbridge.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index 060445d..d9708fa 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -168,6 +168,7 @@ static int virNetDevBridgeGet(const char *brname,
virReportSystemError(EINVAL,
_("Unable to get bridge %s %s"), brname,
paramname);
}
+ VIR_FREE(valuestr);
} else {
struct __bridge_info info;
unsigned long args[] = { BRCTL_GET_BRIDGE_INFO, (unsigned long)&info, 0, 0
};
--
1.7.1
5:46 p.m.
New subject: [libvirt] [PATCH 5/6] util: Plug memory leak on virNetDevBridgeGet() sucessful path
On 11/29/2011 10:57 PM, ajia(a)redhat.com wrote:
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit c1df2c1.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/util/virnetdevbridge.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index 060445d..d9708fa 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -168,6 +168,7 @@ static int virNetDevBridgeGet(const char *brname,
virReportSystemError(EINVAL,
_("Unable to get bridge %s %s"), brname,
paramname);
}
+ VIR_FREE(valuestr);
An improvement, but still not quite right. If we reported EINVAL, we
lacked the goto cleanup that let this function return -1.
Pushing with this squashed in:
diff --git i/src/util/virnetdevbridge.c w/src/util/virnetdevbridge.c
index d9708fa..0440a73 100644
--- i/src/util/virnetdevbridge.c
+++ w/src/util/virnetdevbridge.c
@@ -161,12 +161,16 @@ static int virNetDevBridgeGet(const char *brname,
if (virFileExists(path)) {
char *valuestr;
- if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long),
&valuestr) < 0)
+ if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long),
+ &valuestr) < 0)
goto cleanup;
if (virStrToLong_ul(valuestr, NULL, 10, value) < 0) {
virReportSystemError(EINVAL,
- _("Unable to get bridge %s %s"),
brname, paramname);
+ _("Unable to get bridge %s %s"),
+ brname, paramname);
+ VIR_FREE(valuestr);
+ goto cleanup;
}
VIR_FREE(valuestr);
} else {
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:57 p.m.
New subject: [libvirt] [PATCH 6/6] util: Plug memory leak on virNetDevMacVLanCreateWithVPortProfile() error path
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 90074ec.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/util/virnetdevmacvlan.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 12fc411..80a9f80 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -582,6 +582,7 @@ create_name:
virNetDevError(VIR_ERR_INTERNAL_ERROR,
_("cannot set bandwidth limits on %s"),
cr_ifname);
+ VIR_FORCE_CLOSE(rc);
rc = -1;
goto disassociate_exit;
}
--
1.7.1
5:56 p.m.
New subject: [libvirt] [PATCH 6/6] util: Plug memory leak on virNetDevMacVLanCreateWithVPortProfile() error path
On 11/29/2011 10:57 PM, ajia(a)redhat.com wrote:
From: Alex Jia <ajia(a)redhat.com>
Detected by Coverity. Leak introduced in commit 90074ec.
Signed-off-by: Alex Jia <ajia(a)redhat.com>
---
src/util/virnetdevmacvlan.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 12fc411..80a9f80 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -582,6 +582,7 @@ create_name:
virNetDevError(VIR_ERR_INTERNAL_ERROR,
_("cannot set bandwidth limits on %s"),
cr_ifname);
+ VIR_FORCE_CLOSE(rc);
rc = -1;
goto disassociate_exit;
Close, but this botches the case when !withTap (then, rc is 0 and you
just nuked stdin). Here's what I'm squashing before pushing (and
hopefully Coverity isn't too confused because we overload rc to be
either an fd or just 0 depending on withTap):
diff --git i/src/util/virnetdevmacvlan.c w/src/util/virnetdevmacvlan.c
index 80a9f80..5e55b72 100644
--- i/src/util/virnetdevmacvlan.c
+++ w/src/util/virnetdevmacvlan.c
@@ -582,8 +582,10 @@ create_name:
virNetDevError(VIR_ERR_INTERNAL_ERROR,
_("cannot set bandwidth limits on %s"),
cr_ifname);
- VIR_FORCE_CLOSE(rc);
- rc = -1;
+ if (withTap)
+ VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
+ else
+ rc = -1;
goto disassociate_exit;
}
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:17 p.m.
New subject: [libvirt] [PATCH 6/6] util: Plug memory leak on virNetDevMacVLanCreateWithVPortProfile() error path
On 12/01/2011 07:56 AM, Eric Blake wrote:
On 11/29/2011 10:57 PM, ajia(a)redhat.com wrote:
> From: Alex Jia<ajia(a)redhat.com>
>
> Detected by Coverity. Leak introduced in commit 90074ec.
>
> Signed-off-by: Alex Jia<ajia(a)redhat.com>
> ---
> src/util/virnetdevmacvlan.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 12fc411..80a9f80 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -582,6 +582,7 @@ create_name:
> virNetDevError(VIR_ERR_INTERNAL_ERROR,
> _("cannot set bandwidth limits on %s"),
> cr_ifname);
> + VIR_FORCE_CLOSE(rc);
> rc = -1;
> goto disassociate_exit;
Close, but this botches the case when !withTap (then, rc is 0 and you
just nuked stdin). Here's what I'm squashing before pushing (and
hopefully Coverity isn't too confused because we overload rc to be
either an fd or just 0 depending on withTap):
Yeah, Coverity complains 'rc'
is overwritten, it has a little confused
for this.
diff --git i/src/util/virnetdevmacvlan.c
w/src/util/virnetdevmacvlan.c
index 80a9f80..5e55b72 100644
--- i/src/util/virnetdevmacvlan.c
+++ w/src/util/virnetdevmacvlan.c
@@ -582,8 +582,10 @@ create_name:
virNetDevError(VIR_ERR_INTERNAL_ERROR,
_("cannot set bandwidth limits on %s"),
cr_ifname);
- VIR_FORCE_CLOSE(rc);
- rc = -1;
+ if (withTap)
+ VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
+ else
+ rc = -1;
goto disassociate_exit;
}
4829
days inactive
4830
days old
20 comments
4 participants
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Wen Congyang