[libvirt] [PATCH 0/2] Last two coverity patches
tools/virsh-pool.c: CHECKED_RETURN src/nwfilter/nwfilter_driver.c: UNREACHABLE Ján Tomko (2): virsh: check the return value of virStoragePoolGetAutostart nwfilter: fix error handling src/nwfilter/nwfilter_driver.c | 11 +++++------ tools/virsh-pool.c | 5 +---- 2 files changed, 6 insertions(+), 10 deletions(-) -- 1.7.8.6
On error, virStoragePoolGetAutostart would return -1 leaving autostart untouched. Removed the misleading debug message as well. Error: CHECKED_RETURN (CWE-252): libvirt-0.10.2/tools/virsh-pool.c:1386: unchecked_value: No check of the return value of "virStoragePoolGetAutostart(pool, &autostart)". --- tools/virsh-pool.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index c08cce6..5fee2cd 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -1383,10 +1383,7 @@ cmdPoolInfo(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%-15s %s\n", _("Persistent:"), persistent ? _("yes") : _("no")); /* Check and display whether the pool is autostarted or not */ - virStoragePoolGetAutostart(pool, &autostart); - vshDebug(ctl, VSH_ERR_DEBUG, "Pool autostart flag value: %d\n", - autostart); - if (autostart < 0) + if (virStoragePoolGetAutostart(pool, &autostart) < 0) vshPrint(ctl, "%-15s %s\n", _("Autostart:"), _("no autostart")); else vshPrint(ctl, "%-15s %s\n", _("Autostart:"), autostart ? _("yes") : _("no")); -- 1.7.8.6
On 11/30/12 13:09, Ján Tomko wrote:
On error, virStoragePoolGetAutostart would return -1 leaving autostart untouched.
Removed the misleading debug message as well.
Error: CHECKED_RETURN (CWE-252): libvirt-0.10.2/tools/virsh-pool.c:1386: unchecked_value: No check of the return value of "virStoragePoolGetAutostart(pool, &autostart)". --- tools/virsh-pool.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
ACK. Peter
Commit 4efde75f introduced a return in the cleanup path and removed virNWFilterConfLayerShutdown. Found by coverity: Error: UNREACHABLE (CWE-561): libvirt-0.10.2/src/nwfilter/nwfilter_driver.c:259: unreachable: This code cannot be reached: "nwfilterDriverUnlock(driver...". --- I wonder if the if (!privileged) check should be moved before driverState allocation? --- src/nwfilter/nwfilter_driver.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index a0ee4f1..cd185e2 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -174,8 +174,10 @@ nwfilterDriverStartup(bool privileged) sysbus = virDBusGetSystemBus(); #endif /* HAVE_DBUS */ - if (VIR_ALLOC(driverState) < 0) - goto alloc_err_exit; + if (VIR_ALLOC(driverState) < 0) { + virReportOOMError(); + return -1; + } if (virMutexInit(&driverState->lock) < 0) goto err_free_driverstate; @@ -247,10 +249,7 @@ error: nwfilterDriverUnlock(driverState); nwfilterDriverShutdown(); -alloc_err_exit: - return -1; - - nwfilterDriverUnlock(driverState); + virNWFilterConfLayerShutdown(); err_techdrivers_shutdown: virNWFilterTechDriversShutdown(); -- 1.7.8.6
On 11/30/12 13:09, Ján Tomko wrote:
Commit 4efde75f introduced a return in the cleanup path and removed virNWFilterConfLayerShutdown.
Found by coverity: Error: UNREACHABLE (CWE-561): libvirt-0.10.2/src/nwfilter/nwfilter_driver.c:259: unreachable: This code cannot be reached: "nwfilterDriverUnlock(driver...". --- I wonder if the if (!privileged) check should be moved before driverState allocation? --- src/nwfilter/nwfilter_driver.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index a0ee4f1..cd185e2 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -174,8 +174,10 @@ nwfilterDriverStartup(bool privileged) sysbus = virDBusGetSystemBus(); #endif /* HAVE_DBUS */
- if (VIR_ALLOC(driverState) < 0) - goto alloc_err_exit; + if (VIR_ALLOC(driverState) < 0) { + virReportOOMError(); + return -1; + }
if (virMutexInit(&driverState->lock) < 0) goto err_free_driverstate; @@ -247,10 +249,7 @@ error: nwfilterDriverUnlock(driverState); nwfilterDriverShutdown();
-alloc_err_exit: - return -1;
When you remove this hunk you cause any jump to the error label to fall through all of the following labeled error cleanup sections. Those sections are also called in nwfilterDriverShutdown(). I'm wondering if the functions are safe to call even if the driver wasn't initialized completely. In that case we might be just calling nwfilterDriverShutdown() and get rid of the cleanup labels. If we're not allowed to do that, the return statement has to stay there.
- - nwfilterDriverUnlock(driverState); + virNWFilterConfLayerShutdown();
err_techdrivers_shutdown: virNWFilterTechDriversShutdown();
On 11/30/12 13:51, Peter Krempa wrote:
On 11/30/12 13:09, Ján Tomko wrote:
Commit 4efde75f introduced a return in the cleanup path and removed virNWFilterConfLayerShutdown.
Found by coverity: Error: UNREACHABLE (CWE-561): libvirt-0.10.2/src/nwfilter/nwfilter_driver.c:259: unreachable: This code cannot be reached: "nwfilterDriverUnlock(driver...". --- I wonder if the if (!privileged) check should be moved before driverState allocation? --- src/nwfilter/nwfilter_driver.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index a0ee4f1..cd185e2 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -174,8 +174,10 @@ nwfilterDriverStartup(bool privileged) sysbus = virDBusGetSystemBus(); #endif /* HAVE_DBUS */
- if (VIR_ALLOC(driverState) < 0) - goto alloc_err_exit; + if (VIR_ALLOC(driverState) < 0) { + virReportOOMError(); + return -1; + }
if (virMutexInit(&driverState->lock) < 0) goto err_free_driverstate; @@ -247,10 +249,7 @@ error: nwfilterDriverUnlock(driverState); nwfilterDriverShutdown();
-alloc_err_exit: - return -1;
When you remove this hunk you cause any jump to the error label to fall through all of the following labeled error cleanup sections.
Those sections are also called in nwfilterDriverShutdown(). I'm wondering if the functions are safe to call even if the driver wasn't initialized completely. In that case we might be just calling nwfilterDriverShutdown() and get rid of the cleanup labels. If we're not allowed to do that, the return statement has to stay there.
virNWFilterDHCPSnoopShutdown(void) doesn't look that it can be called without being initialized first, so you will need to either have to fix virNWFilterDHCPSnoopShutdown and make sure that other are callable even when not initialized, or you need to exit from the function at the point the label was before.
- - nwfilterDriverUnlock(driverState); + virNWFilterConfLayerShutdown();
err_techdrivers_shutdown: virNWFilterTechDriversShutdown();
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Also removed some unreachable code found by coverity: libvirt-0.10.2/src/nwfilter/nwfilter_driver.c:259: unreachable: This code cannot be reached: "nwfilterDriverUnlock(driver...". --- diff to v1: don't break everything src/nwfilter/nwfilter_driver.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index a0ee4f1..cff384e 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -174,8 +174,10 @@ nwfilterDriverStartup(bool privileged) sysbus = virDBusGetSystemBus(); #endif /* HAVE_DBUS */ - if (VIR_ALLOC(driverState) < 0) - goto alloc_err_exit; + if (VIR_ALLOC(driverState) < 0) { + virReportOOMError(); + return -1; + } if (virMutexInit(&driverState->lock) < 0) goto err_free_driverstate; @@ -247,11 +249,8 @@ error: nwfilterDriverUnlock(driverState); nwfilterDriverShutdown(); -alloc_err_exit: return -1; - nwfilterDriverUnlock(driverState); - err_techdrivers_shutdown: virNWFilterTechDriversShutdown(); virNWFilterDHCPSnoopShutdown(); -- 1.7.8.6
On 11/30/12 15:07, Ján Tomko wrote:
Also removed some unreachable code found by coverity: libvirt-0.10.2/src/nwfilter/nwfilter_driver.c:259: unreachable: This code cannot be reached: "nwfilterDriverUnlock(driver...". --- diff to v1: don't break everything
src/nwfilter/nwfilter_driver.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)
ACK. Peter
On 11/30/12 13:09, Ján Tomko wrote:
tools/virsh-pool.c: CHECKED_RETURN src/nwfilter/nwfilter_driver.c: UNREACHABLE
Ján Tomko (2): virsh: check the return value of virStoragePoolGetAutostart nwfilter: fix error handling
src/nwfilter/nwfilter_driver.c | 11 +++++------ tools/virsh-pool.c | 5 +---- 2 files changed, 6 insertions(+), 10 deletions(-)
Series pushed. Peter
participants (2)
-
Ján Tomko -
Peter Krempa