8:57 a.m.
On 02/16/2017 09:26 AM, Joao Martins wrote:
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.
What about:
args->migcookie = mig;
and then successful virThreadCreate(&thread, false,
libxlDoMigrateReceive, args) where 'args' is unref'd, but the cookie
isn't free'd.
Certainly you wouldn't want to free the cookie and then let the thread
use it!
So I think the libxlMigrationCookieFree needs to be done in the error:
label and in the thread cleanup. Also after you assign @mig to
migcookie, set 'mig = NULL';
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;
This doesn't do much... since @tc would be allocated below and *tnl
wouldn't be updated... Thus, the following is better:
if (VIR_ALLOC(tc) < 0)
goto out;
*tnl = tc;
John
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);