10:14 a.m.
On 02/16/2017 02:57 PM, John Ferlan wrote:
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!
\facepalm - you're right.
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';
Yeap, let me send that in a follow-up patch
shortly.
The thread cleanup you mean libxlDoMigrateReceive right? There I think we could
simply move unref args to cleanup label (which would free the cookie). But
probably should go into a separate patch as it seems to be an issue to all
migration paths.
> 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;
OK.
> 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);
>