Re: [PATCH v3 2/2] libxl: add tunnelled migration support

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 02/16/2017 11:47 AM, John Ferlan wrote:
> FYI: Couple of Coverity issues resulted from these patches.
> 
>>  int
>> +libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
>> +                                   virStreamPtr st,
>> +                                   virDomainDefPtr *def,
>> +                                   const char *cookiein,
>> +                                   int cookieinlen,
>> +                                   unsigned int flags)
>> +{
>> +    libxlMigrationCookiePtr mig = NULL;
>> +    libxlDriverPrivatePtr driver = dconn->privateData;
>> +    virDomainObjPtr vm = NULL;
>> +    libxlMigrationDstArgs *args = NULL;
>> +    virThread thread;
>> +    bool taint_hook = false;
>> +    libxlDomainObjPrivatePtr priv = NULL;
>> +    char *xmlout = NULL;
>> +    int dataFD[2] = { -1, -1 };
>> +    int ret = -1;
>> +
>> +    if (libxlDomainMigrationPrepareAny(dconn, def, cookiein, cookieinlen,
>> +                                       &mig, &xmlout, &taint_hook) < 0)
> 
> Coverity notes that '@mig' will be leaked in this case when "if
> (!cookiein || !cookieinlen) {" is true in libxlMigrationEatCookie and
> failures from libxlDomainMigrationPrepareAny don't free it.

/nods. This diff should do then? It covers both cases I think.

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index ba1ca5c..6a7d458 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -637,6 +637,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
     }

  done:
+    libxlMigrationCookieFree(mig);
     if (vm)
         virObjectUnlock(vm);



>> +        goto error;
>> +
>> +    if (!(vm = virDomainObjListAdd(driver->domains, *def,
>> +                                   driver->xmlopt,
>> +                                   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
>> +                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
>> +                                   NULL)))
>> +        goto error;
>> +
>> +    *def = NULL;
>> +    priv = vm->privateData;
>> +
>> +    if (taint_hook) {
>> +        /* Domain XML has been altered by a hook script. */
>> +        priv->hookRun = true;
>> +    }
>> +
>> +    /*
>> +     * The data flow of tunnel3 migration in the dest side:
>> +     * stream -> pipe -> recvfd of libxlDomainStartRestore
>> +     */
>> +    if (pipe(dataFD) < 0)
>> +        goto error;
>> +
>> +    /* Stream data will be written to pipeIn */
>> +    if (virFDStreamOpen(st, dataFD[1]) < 0)
>> +        goto error;
>> +    dataFD[1] = -1; /* 'st' owns the FD now & will close it */
>> +
>> +    if (libxlMigrationDstArgsInitialize() < 0)
>> +        goto error;
>> +
>> +    if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>> +        goto error;
>> +
>> +    args->conn = dconn;
>> +    args->vm = vm;
>> +    args->flags = flags;
>> +    args->migcookie = mig;
>> +    /* Receive from pipeOut */
>> +    args->recvfd = dataFD[0];
>> +    args->nsocks = 0;
>> +    if (virThreadCreate(&thread, false, libxlDoMigrateReceive, args) < 0) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                       _("Failed to create thread for receiving migration data"));
>> +        goto error;
>> +    }
>> +
>> +    ret = 0;
>> +    goto done;
>> +
>> + error:
>> +    VIR_FORCE_CLOSE(dataFD[1]);
>> +    VIR_FORCE_CLOSE(dataFD[0]);
>> +    virObjectUnref(args);
>> +    /* Remove virDomainObj from domain list */
>> +    if (vm) {
>> +        virDomainObjListRemove(driver->domains, vm);
>> +        vm = NULL;
>> +    }
>> +
>> + done:
>> +    if (vm)
>> +        virObjectUnlock(vm);
>> +
>> +    return ret;
>> +}
> 
> [...]
> 
>> +
>> +static int
>> +libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
>> +                          virDomainObjPtr vm,
>> +                          unsigned long flags,
>> +                          virStreamPtr st,
>> +                          struct libxlTunnelControl *tc)
>> +{
>> +    libxlTunnelMigrationThread *arg = NULL;
>> +    int ret = -1;
>> +
>> +    if (VIR_ALLOC(tc) < 0)
>> +        goto out;
>> +
>> +    tc->dataFD[0] = -1;
>> +    tc->dataFD[1] = -1;
>> +    if (pipe(tc->dataFD) < 0) {
>> +        virReportError(errno, "%s", _("Unable to make pipes"));
> 
> Coverity notes that failures would cause a leak for @tc (I assume here
> and of course if virThreadCreate fails.  Perhaps the 'best' way to
> handle that would be to set tc = NULL after ThreadCreate success and
> just call libxlMigrationStopTunnel in "out:"...

But libxlMigrationStopTunnel is always called (hence should be freeing @tc),
whether libxlMigrationStartTunnel failed or not. How about this?

@@ -907,8 +908,9 @@ libxlMigrationStartTunnel(libxlDriverPrivatePtr driver,
                           virDomainObjPtr vm,
                           unsigned long flags,
                           virStreamPtr st,
-                          struct libxlTunnelControl *tc)
+                          struct libxlTunnelControl **tnl)
 {
+    struct libxlTunnelControl *tc = *tnl;
     libxlTunnelMigrationThread *arg = NULL;
     int ret = -1;

@@ -1045,7 +1047,7 @@ libxlDoMigrateP2P(libxlDriverPrivatePtr driver,

     VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
     if (flags & VIR_MIGRATE_TUNNELLED)
-        ret = libxlMigrationStartTunnel(driver, vm, flags, st, tc);
+        ret = libxlMigrationStartTunnel(driver, vm, flags, st, &tc);
     else
         ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
                                           uri_out, NULL, flags);

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list