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

[Jim Fehlig]

Michal Privoznik wrote:
> 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.
>   

Right. Should be removed since I'm touching this code.

>   
>>          virNetSocketRemoveIOCallback(socks[i]);
>>          virNetSocketClose(socks[i]);
>> -        virObjectUnref(socks[i]);
>>     
>
> This will leak the socks[i] object, wouldn't it? 

Yes, you are right. I verified the virNetSocket was not disposed with
the missing unref.

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

How does it look with the following squashed in?

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 8db3aea..9609e06 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -112,9 +112,9 @@ libxlDoMigrateReceive(void *opaque)
 
     /* Remove all listen socks from event handler, and close them. */
     for (i = 0; i < nsocks; i++) {
-        virNetSocketUpdateIOCallback(socks[i], 0);
         virNetSocketRemoveIOCallback(socks[i]);
         virNetSocketClose(socks[i]);
+        virObjectUnref(socks[i]);
         socks[i] = NULL;
     }
     args->nsocks = 0;

Thanks for taking time to review this series!

Regards,
Jim