[libvirt] [PATCH] qemuDomainAttachNetDevice: Avoid @originalError leak
Coverity identified that this variable might be leaked. And it's right. If an error occurred and we have to roll back the control jumps to try_remove label where we save the current error (see 0e82fa4c34 for more info). However, inside the code a jump onto other label is possible thus leaking the error object. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Best viewed with 'git show --ignore-space-change' src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1bc2706..0dcbee6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1326,32 +1326,32 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (vlan < 0) { if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { char *netdev_name; - if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) - goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - if (charDevPlugged && - qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) - VIR_WARN("Failed to remove associated chardev %s", charDevAlias); - if (netdevPlugged && - qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) - VIR_WARN("Failed to remove network backend for netdev %s", - netdev_name); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - VIR_FREE(netdev_name); + if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) { + qemuDomainObjEnterMonitor(driver, vm); + if (charDevPlugged && + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) + VIR_WARN("Failed to remove associated chardev %s", charDevAlias); + if (netdevPlugged && + qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) + VIR_WARN("Failed to remove network backend for netdev %s", + netdev_name); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + VIR_FREE(netdev_name); + } } else { VIR_WARN("Unable to remove network backend"); } } else { char *hostnet_name; - if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) - goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - if (hostPlugged && - qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) - VIR_WARN("Failed to remove network backend for vlan %d, net %s", - vlan, hostnet_name); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - VIR_FREE(hostnet_name); + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) >= 0) { + qemuDomainObjEnterMonitor(driver, vm); + if (hostPlugged && + qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) + VIR_WARN("Failed to remove network backend for vlan %d, net %s", + vlan, hostnet_name); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + VIR_FREE(hostnet_name); + } } virSetError(originalError); virFreeError(originalError); -- 2.8.4
On 11/11/2016 10:33 AM, Michal Privoznik wrote:
Coverity identified that this variable might be leaked. And it's right. If an error occurred and we have to roll back the control jumps to try_remove label where we save the current error (see 0e82fa4c34 for more info). However, inside the code a jump onto other label is possible thus leaking the error object.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Best viewed with 'git show --ignore-space-change'
src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
Yuck ;-) All because qemuBuildHostNetStr adds the "id=" string into the middle somewhere...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1bc2706..0dcbee6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1326,32 +1326,32 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (vlan < 0) { if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { char *netdev_name; - if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) - goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - if (charDevPlugged && - qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) - VIR_WARN("Failed to remove associated chardev %s", charDevAlias); - if (netdevPlugged && - qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) - VIR_WARN("Failed to remove network backend for netdev %s", - netdev_name); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - VIR_FREE(netdev_name); + if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) { + qemuDomainObjEnterMonitor(driver, vm); + if (charDevPlugged && + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) + VIR_WARN("Failed to remove associated chardev %s", charDevAlias); + if (netdevPlugged && + qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) + VIR_WARN("Failed to remove network backend for netdev %s", + netdev_name); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + VIR_FREE(netdev_name); + } } else { VIR_WARN("Unable to remove network backend"); } } else { char *hostnet_name; - if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) - goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - if (hostPlugged && - qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) - VIR_WARN("Failed to remove network backend for vlan %d, net %s", - vlan, hostnet_name); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - VIR_FREE(hostnet_name); + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) >= 0) {
In qemuBuildHostNetStr when vlan >= 0, the printing of the id is gated on net->info.alias - that doesn't happen here, but then again it's printing into "name=".. So looking at qemuDomainRemoveNetDevice - I see a slightly different removal algorithm... John
+ qemuDomainObjEnterMonitor(driver, vm); + if (hostPlugged && + qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) + VIR_WARN("Failed to remove network backend for vlan %d, net %s", + vlan, hostnet_name); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + VIR_FREE(hostnet_name); + } } virSetError(originalError); virFreeError(originalError);
On 11/11/2016 11:37 AM, John Ferlan wrote:
On 11/11/2016 10:33 AM, Michal Privoznik wrote:
Coverity identified that this variable might be leaked. And it's right. If an error occurred and we have to roll back the control jumps to try_remove label where we save the current error (see 0e82fa4c34 for more info). However, inside the code a jump onto other label is possible thus leaking the error object.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Best viewed with 'git show --ignore-space-change'
src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
Yuck ;-) All because qemuBuildHostNetStr adds the "id=" string into the middle somewhere...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1bc2706..0dcbee6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1326,32 +1326,32 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, if (vlan < 0) { if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) { char *netdev_name; - if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0) - goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - if (charDevPlugged && - qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) - VIR_WARN("Failed to remove associated chardev %s", charDevAlias); - if (netdevPlugged && - qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) - VIR_WARN("Failed to remove network backend for netdev %s", - netdev_name); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - VIR_FREE(netdev_name); + if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) { + qemuDomainObjEnterMonitor(driver, vm); + if (charDevPlugged && + qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) + VIR_WARN("Failed to remove associated chardev %s", charDevAlias); + if (netdevPlugged && + qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0) + VIR_WARN("Failed to remove network backend for netdev %s", + netdev_name); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + VIR_FREE(netdev_name); + } } else { VIR_WARN("Unable to remove network backend"); } } else { char *hostnet_name; - if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0) - goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - if (hostPlugged && - qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) - VIR_WARN("Failed to remove network backend for vlan %d, net %s", - vlan, hostnet_name); - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - VIR_FREE(hostnet_name); + if (virAsprintf(&hostnet_name, "host%s", net->info.alias) >= 0) {
In qemuBuildHostNetStr when vlan >= 0, the printing of the id is gated on net->info.alias - that doesn't happen here, but then again it's printing into "name="..
So looking at qemuDomainRemoveNetDevice - I see a slightly different removal algorithm...
John
Oy... Let me revisit... To the degree that this patch fixes the issue I noted from Coverity - that's true - so ACK for that. John The error path logic itself is still a bit confusing as the attach and remove logic is based on QEMU_CAPS_NETDEV while this error path is based on vlan < 0 logic and isn't necessarily self documenting ~/~
+ qemuDomainObjEnterMonitor(driver, vm); + if (hostPlugged && + qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0) + VIR_WARN("Failed to remove network backend for vlan %d, net %s", + vlan, hostnet_name); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + VIR_FREE(hostnet_name); + } } virSetError(originalError); virFreeError(originalError);
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
John Ferlan -
Michal Privoznik