[libvirt] [PATCH] conf: utility function to update entry in def->nets array

[Michal Privoznik]

On 9/25/19 6:57 PM, Laine Stump wrote:
> A virDomainNetDef object in a domain's nets array might contain a
> virDomainHostdevDef, and when this is the case, the domain's hostdevs
> array will also have a pointer to this embedded hostdev (this is done
> so that internal functions that need to perform some operation on all
> hostdevs won't leave out the type='hostdev' network interfaces).
> 
> When a network device was updated with virDomainUpdateDeviceFlags(),
> we were replacing the entry in the nets array (and free'ing the
> original) but forgetting about the pointer in the hostdevs array
> (which would then point to the now-free'd hostdev contained in the old
> net object.) This often resulted in a libvirtd crash.
> 
> The solution is to add a function, virDomainNetUpdate(), called by
> qemuDomainUpdateDeviceConfig(), that updates the hostdevs array
> appropriately along with the nets array.
> 
> Resolves: https://bugzilla.redhat.com/1558934
> Signed-off-by: Laine Stump <laine at redhat.com>
> 
> ---
> 
> (I actually think that it was a bad idea to have this "reach over"
> pointer in the hostdevs array (I can say that, since it was my idea
> :-), and want to eliminate it in favor of using an iterator function
> for all operations that need to do something to all hostdevs, but this
> bug exists on old, maintained branches, and I wouldn't want to require
> backporting anything that disruptive to a stable branch.)
> 
> Also, I was unable to reproduce the crashes described in the bug
> report using current upstream code, but could witness (by adding a
> breakpoint in gdb) the stale pointer when running without this patch,
> and the correct pointer when running with the patch)

Agreed, refactoring the whole approach would require a significant 
amount of work and it's not worth it only to fix a crasher.

> 
>   src/conf/domain_conf.c   | 41 ++++++++++++++++++++++++++++++++++++++++
>   src/conf/domain_conf.h   |  1 +
>   src/libvirt_private.syms |  1 +
>   src/qemu/qemu_driver.c   |  6 ++++--
>   4 files changed, 47 insertions(+), 2 deletions(-)
> 

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0753904472..5f63c4f51e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8786,8 +8786,10 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
>                                            false) < 0)
>               return -1;
>   
> -        virDomainNetDefFree(vmdef->nets[pos]);
> -        vmdef->nets[pos] = net;
> +        if (virDomainNetUpdate(vmdef, pos, net))
> +            return -1;
> +
> +        virDomainNetDefFree(oldDev.data.net);
>           dev->data.net = NULL;
>           break;
>   
> 

The same code pattern occurrs in lxcDomainUpdateDeviceConfig() so you 
may want to fix it the same way as you're doing here. With that,

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

Michal