qemu_hotplug: Don't skip cleanup in qemuDomainAttachNetDevice()

[PATCH for 8.5.0] qemu_hotplug: Don't skip cleanup in qemuDomainAttachNetDevice()

Posted by Michal Privoznik 1 year, 10 months ago

Introduced in v8.4.0-rc1~183 but the first real problem
introduced in v8.4.0-rc1~170, there's a
qemuBuildInterfaceConnect() call inside of
qemuDomainAttachNetDevice(). If the former fails, then the
function is immediately returned from instead of jumping onto the
cleanup label. This is crucial, because at this point the domain
definition contains 'borrowed' net definition, which is then
freed, since an error was met. The domain definition is then left
with a dangling pointer which leads to all sorts of different
crashes.

Fixes: 29d022b1eb7b2330ed224a08509e6d3a5eeecc53
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2102009
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ee44649d48..27e68370cf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1265,7 +1265,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
     VIR_APPEND_ELEMENT_COPY(vm->def->nets, vm->def->nnets, net);
 
     if (qemuBuildInterfaceConnect(vm, net, VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0)
-         return -1;
+        goto cleanup;
 
     iface_connected = true;
 
-- 
2.35.1

Re: [PATCH for 8.5.0] qemu_hotplug: Don't skip cleanup in qemuDomainAttachNetDevice()

Posted by Jiri Denemark 1 year, 10 months ago

On Fri, Jul 01, 2022 at 09:43:16 +0200, Michal Privoznik wrote:
> Introduced in v8.4.0-rc1~183 but the first real problem
> introduced in v8.4.0-rc1~170, there's a
> qemuBuildInterfaceConnect() call inside of
> qemuDomainAttachNetDevice(). If the former fails, then the
> function is immediately returned from instead of jumping onto the
> cleanup label. This is crucial, because at this point the domain
> definition contains 'borrowed' net definition, which is then
> freed, since an error was met. The domain definition is then left
> with a dangling pointer which leads to all sorts of different
> crashes.
> 
> Fixes: 29d022b1eb7b2330ed224a08509e6d3a5eeecc53
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2102009
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index ee44649d48..27e68370cf 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1265,7 +1265,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
>      VIR_APPEND_ELEMENT_COPY(vm->def->nets, vm->def->nnets, net);
>  
>      if (qemuBuildInterfaceConnect(vm, net, VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0)
> -         return -1;
> +        goto cleanup;
>  
>      iface_connected = true;
>  

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

Re: [PATCH for 8.5.0] qemu_hotplug: Don't skip cleanup in qemuDomainAttachNetDevice()

Posted by Michal Prívozník 1 year, 10 months ago

On 7/1/22 10:43, Jiri Denemark wrote:
> On Fri, Jul 01, 2022 at 09:43:16 +0200, Michal Privoznik wrote:
>> Introduced in v8.4.0-rc1~183 but the first real problem
>> introduced in v8.4.0-rc1~170, there's a
>> qemuBuildInterfaceConnect() call inside of
>> qemuDomainAttachNetDevice(). If the former fails, then the
>> function is immediately returned from instead of jumping onto the
>> cleanup label. This is crucial, because at this point the domain
>> definition contains 'borrowed' net definition, which is then
>> freed, since an error was met. The domain definition is then left
>> with a dangling pointer which leads to all sorts of different
>> crashes.
>>
>> Fixes: 29d022b1eb7b2330ed224a08509e6d3a5eeecc53
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2102009
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_hotplug.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index ee44649d48..27e68370cf 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -1265,7 +1265,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
>>      VIR_APPEND_ELEMENT_COPY(vm->def->nets, vm->def->nnets, net);
>>  
>>      if (qemuBuildInterfaceConnect(vm, net, VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0)
>> -         return -1;
>> +        goto cleanup;
>>  
>>      iface_connected = true;
>>  
> 
> Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
> 

Merged, thanks.

Michal