[libvirt] [PATCH] qemuDomainAttachNetDevice: Avoid @originalError leak

[John Ferlan]

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 at 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);
>