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

[John Ferlan]

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 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

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>