[libvirt] [PATCH 0/6] plug memory leaks
From: Alex Jia <ajia@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@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(-)
From: Alex Jia <ajia@redhat.com> Detected by Coverity. Leak introduced in commit 0873b68. Signed-off-by: Alex Jia <ajia@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
On 11/29/2011 10:57 PM, ajia@redhat.com wrote:
From: Alex Jia <ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 0873b68.
Signed-off-by: Alex Jia <ajia@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Alex Jia <ajia@redhat.com> Detected by Coverity. Leak introduced in commit 109efd7. Signed-off-by: Alex Jia <ajia@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
At 11/30/2011 01:57 PM, ajia@redhat.com Write:
From: Alex Jia <ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 109efd7.
Signed-off-by: Alex Jia <ajia@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
}
On 11/30/2011 02:20 PM, Wen Congyang wrote:
At 11/30/2011 01:57 PM, ajia@redhat.com Write:
From: Alex Jia<ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 109efd7.
Signed-off-by: Alex Jia<ajia@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
}
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@redhat.com Write:
From: Alex Jia<ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 109efd7.
Signed-off-by: Alex Jia<ajia@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
}
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@redhat.com Write:
From: Alex Jia<ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 109efd7.
Signed-off-by: Alex Jia<ajia@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
}
From: Alex Jia <ajia@redhat.com> Detected by Coverity. Leak introduced in commit 673adba. Signed-off-by: Alex Jia <ajia@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
At 11/30/2011 01:57 PM, ajia@redhat.com Write:
From: Alex Jia <ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 673adba.
Signed-off-by: Alex Jia <ajia@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; }
On 11/30/2011 02:26 PM, Wen Congyang wrote:
At 11/30/2011 01:57 PM, ajia@redhat.com Write:
From: Alex Jia<ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 673adba.
Signed-off-by: Alex Jia<ajia@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; }
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@redhat.com Write:
From: Alex Jia<ajia@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com Write:
From: Alex Jia<ajia@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; }
/*
From: Alex Jia <ajia@redhat.com> Detected by Coverity. Leak introduced in commit 8866eed. Signed-off-by: Alex Jia <ajia@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
On 11/29/2011 10:57 PM, ajia@redhat.com wrote:
From: Alex Jia <ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 8866eed.
Signed-off-by: Alex Jia <ajia@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/01/2011 07:39 AM, Eric Blake wrote:
On 11/29/2011 10:57 PM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 8866eed.
Signed-off-by: Alex Jia<ajia@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;
From: Alex Jia <ajia@redhat.com> Detected by Coverity. Leak introduced in commit c1df2c1. Signed-off-by: Alex Jia <ajia@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
On 11/29/2011 10:57 PM, ajia@redhat.com wrote:
From: Alex Jia <ajia@redhat.com>
Detected by Coverity. Leak introduced in commit c1df2c1.
Signed-off-by: Alex Jia <ajia@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Alex Jia <ajia@redhat.com> Detected by Coverity. Leak introduced in commit 90074ec. Signed-off-by: Alex Jia <ajia@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
On 11/29/2011 10:57 PM, ajia@redhat.com wrote:
From: Alex Jia <ajia@redhat.com>
Detected by Coverity. Leak introduced in commit 90074ec.
Signed-off-by: Alex Jia <ajia@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/01/2011 07:56 AM, Eric Blake wrote:
> On 11/29/2011 10:57 PM, ajia@redhat.com wrote:
>> From: Alex Jia<ajia@redhat.com>
>>
>> Detected by Coverity. Leak introduced in commit 90074ec.
>>
>> Signed-off-by: Alex Jia<ajia@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;
> }
>
>
>
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Wen Congyang