[libvirt] [PATCH] fix segfault during virsh save in pv guest

[Jim Fehlig]

Bamvor Jian Zhang wrote:
> this patch fix the wrong sequence for fd and timeout register. the sequence
> was right in dfa1e1dd for fd register, but it changed in e0622ca2.
> in this patch, set priv, xl_priv in info and increase info->priv ref count
> before virEventAddHandle. if do this after virEventAddHandle, the fd
> callback or fd deregister maybe got the empty priv, xl_priv or wrong ref
> count.
>   

Bamvor and I discussed this patch off-list while investigating reports
of segfaults in the libxl driver, e.g.

https://www.redhat.com/archives/libvir-list/2013-March/msg00502.html

> after apply this patch, test more than 100 rounds passed compare to fail
> within 3 rounds without this patch. each round includes define -> start ->
> destroy -> create -> suspend -> resume -> reboot -> shutdown -> save ->
> resotre -> dump -> destroy -> create -> setmem -> setvcpus -> destroy.
>   

ACK.  I'll push this since it is a bug fix.

Regards,
Jim

> Signed-off-by: Bamvor Jian Zhang <bjzhang at suse.com>
> ---
>  src/libxl/libxl_driver.c |   39 +++++++++++++++++++++------------------
>  1 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b4f1889..212d0fc 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -181,26 +181,28 @@ libxlFDRegisterEventHook(void *priv, int fd, void **hndp,
>          return -1;
>      }
>  
> +    info->priv = priv;
> +    /*
> +     * Take a reference on the domain object.  Reference is dropped in
> +     * libxlEventHookInfoFree, ensuring the domain object outlives the fd
> +     * event objects.
> +     */
> +    virObjectRef(info->priv);
> +    info->xl_priv = xl_priv;
> +
>      if (events & POLLIN)
>          vir_events |= VIR_EVENT_HANDLE_READABLE;
>      if (events & POLLOUT)
>          vir_events |= VIR_EVENT_HANDLE_WRITABLE;
> +
>      info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback,
>                                   info, libxlEventHookInfoFree);
>      if (info->id < 0) {
> +        virObjectUnref(info->priv);
>          VIR_FREE(info);
>          return -1;
>      }
>  
> -    info->priv = priv;
> -    /*
> -     * Take a reference on the domain object.  Reference is dropped in
> -     * libxlEventHookInfoFree, ensuring the domain object outlives the fd
> -     * event objects.
> -     */
> -    virObjectRef(info->priv);
> -
> -    info->xl_priv = xl_priv;
>      *hndp = info;
>  
>      return 0;
> @@ -283,6 +285,15 @@ libxlTimeoutRegisterEventHook(void *priv,
>          return -1;
>      }
>  
> +    info->priv = priv;
> +    /*
> +     * Also take a reference on the domain object.  Reference is dropped in
> +     * libxlEventHookInfoFree, ensuring the domain object outlives the timeout
> +     * event objects.
> +     */
> +    virObjectRef(info->priv);
> +    info->xl_priv = xl_priv;
> +
>      gettimeofday(&now, NULL);
>      timersub(&abs_t, &now, &res);
>      /* Ensure timeout is not overflowed */
> @@ -296,22 +307,14 @@ libxlTimeoutRegisterEventHook(void *priv,
>      info->id = virEventAddTimeout(timeout, libxlTimerCallback,
>                                    info, libxlEventHookInfoFree);
>      if (info->id < 0) {
> +        virObjectUnref(info->priv);
>          VIR_FREE(info);
>          return -1;
>      }
>  
> -    info->priv = priv;
> -    /*
> -     * Also take a reference on the domain object.  Reference is dropped in
> -     * libxlEventHookInfoFree, ensuring the domain object outlives the timeout
> -     * event objects.
> -     */
> -    virObjectRef(info->priv);
> -
>      virObjectLock(info->priv);
>      LIBXL_EV_REG_APPEND(info->priv->timerRegistrations, info);
>      virObjectUnlock(info->priv);
> -    info->xl_priv = xl_priv;
>      *hndp = info;
>  
>      return 0;
>