[libvirt] [PATCH V2 1/3] libxl: fix ref counting of libxlMigrationDstArgs

[Michal Privoznik]

On 07.08.2015 19:53, Jim Fehlig wrote:
> This patch fixes some flawed logic around ref counting the
> libxlMigrationDstArgs object.
> 
> First, when adding sockets to the event loop with
> virNetSocketAddIOCallback(), the generic virObjectFreeCallback()
> was registered as a free function, with libxlMigrationDstArgs as
> its parameter. A reference was also taken on
> libxlMigrationDstArgs for each successful call to
> virNetSocketAddIOCallback(). The rational behind this logic was
> that the libxlMigrationDstArgs object had to out-live the socket
> objects. But virNetSocketAddIOCallback() already takes a
> reference on socket objects, ensuring their life until removed
> from the event loop and unref'ed in virNetSocketEventFree(). We
> only need to ensure libxlMigrationDstArgs lives until
> libxlDoMigrateReceive() finishes, which can be done by simply
> unref'ing libxlMigrationDstArgs at the end of
> libxlDoMigrateReceive().
> 
> The second flaw was unref'ing the sockets in the failure path of
> libxlMigrateReceive() and at the end of libxlDoMigrateReceive().
> As mentioned above, the sockets are already unref'ed by
> virNetSocketEventFree() when removed from the event loop.
> Attempting to unref the socket a second time resulted in a
> libvirtd crash since the socket was previously unref'ed and
> disposed.
> 
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
> 
> V2: Initialize args in libxlDomainMigrationPrepare
> 
>  src/libxl/libxl_migration.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index aa9547b..f9673c8 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -112,11 +112,11 @@ libxlDoMigrateReceive(void *opaque)
>          virNetSocketUpdateIOCallback(socks[i], 0);

This is pre-existing, but since the socket callback is removed right
after, it does not make much sense to update its events to listen for.

>          virNetSocketRemoveIOCallback(socks[i]);
>          virNetSocketClose(socks[i]);
> -        virObjectUnref(socks[i]);

This will leak the socks[i] object, wouldn't it? I mean, in
libxlDomainMigrationPrepare() on line 392 virNetSocketNewListenTCP() is
called. This initialize the array with object pointers. Then
virNetSocketAddIOCallback() + virNetSocketRemoveIOCallback() pair keep
the ref counter consistent. This makes me think you should not remove
this line.

>          socks[i] = NULL;
>      }
>      args->nsocks = 0;
>      VIR_FORCE_CLOSE(recvfd);
> +    virObjectUnref(args);
>  }
>  
>  
> @@ -164,11 +164,11 @@ libxlMigrateReceive(virNetSocketPtr sock,
>          virNetSocketUpdateIOCallback(socks[i], 0);
>          virNetSocketRemoveIOCallback(socks[i]);
>          virNetSocketClose(socks[i]);
> -        virObjectUnref(socks[i]);
>          socks[i] = NULL;
>      }
>      args->nsocks = 0;
>      VIR_FORCE_CLOSE(recvfd);
> +    virObjectUnref(args);
>  }
>  
>  static int
> @@ -318,7 +318,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>      virNetSocketPtr *socks = NULL;
>      size_t nsocks = 0;
>      int nsocks_listen = 0;
> -    libxlMigrationDstArgs *args;
> +    libxlMigrationDstArgs *args = NULL;
>      size_t i;
>      int ret = -1;
>  
> @@ -420,22 +420,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>                                        VIR_EVENT_HANDLE_READABLE,
>                                        libxlMigrateReceive,
>                                        args,
> -                                      virObjectFreeCallback) < 0)
> +                                      NULL) < 0)
>              continue;
>  
> -        /*
> -         * Successfully added sock to event loop.  Take a ref on args to
> -         * ensure it is not freed until sock is removed from the event loop.
> -         * Ref is dropped in virObjectFreeCallback after being removed
> -         * from the event loop.
> -         */
> -        virObjectRef(args);
>          nsocks_listen++;
>      }
>  
> -    /* Done with args in this function, drop reference */
> -    virObjectUnref(args);
> -
>      if (!nsocks_listen)
>          goto error;
>  
> @@ -448,6 +438,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>          virObjectUnref(socks[i]);
>      }
>      VIR_FREE(socks);
> +    virObjectUnref(args);
> +
>      /* Remove virDomainObj from domain list */
>      if (vm) {
>          virDomainObjListRemove(driver->domains, vm);
> 

Otherwise looking good.

Michal