[libvirt] [PATCH 4/6] uml: Plug memory leak on umlStartVMDaemon() error path

[Alex Jia]

On 12/01/2011 07:39 AM, Eric Blake wrote:
> On 11/29/2011 10:57 PM, ajia at redhat.com wrote:
>> From: Alex Jia<ajia at redhat.com>
>>
>> Detected by Coverity.  Leak introduced in commit 8866eed.
>>
>> Signed-off-by: Alex Jia<ajia at redhat.com>
>> ---
>>   src/uml/uml_driver.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
>> index 073f362..fe6d5ba 100644
>> --- a/src/uml/uml_driver.c
>> +++ b/src/uml/uml_driver.c
>> @@ -1051,6 +1051,7 @@ static int umlStartVMDaemon(virConnectPtr conn,
>>           VIR_FREE(vm->def->consoles[i]->info.alias);
>>           if (virAsprintf(&vm->def->consoles[i]->info.alias, "console%zu", i)<  0) {
>>               virReportOOMError();
>> +            VIR_FORCE_CLOSE(logfd);
>>               goto cleanup;
> Good catch, but while reading the function, I found another bug - if we
> failed to add the domain to autodestroy, we still returned success but
> without setting the domain to transient.  I'm pushing this more
> comprehensive fix:
Indeed, the return value hasn't been stored in 'ret' if we failed to add 
domain
to autodestroy. Eric, thanks.
> diff --git i/src/uml/uml_driver.c w/src/uml/uml_driver.c
> index fe6d5ba..faea313 100644
> --- i/src/uml/uml_driver.c
> +++ w/src/uml/uml_driver.c
> @@ -1033,56 +1033,51 @@ static int umlStartVMDaemon(virConnectPtr conn,
>       /* Do this upfront, so any part of the startup process can add
>        * runtime state to vm->def that won't be persisted. This let's us
>        * report implicit runtime defaults in the XML, like vnc listen/socket
>        */
>       VIR_DEBUG("Setting current domain def as transient");
>       if (virDomainObjSetDefTransient(driver->caps, vm, true)<  0) {
>           VIR_FORCE_CLOSE(logfd);
>           return -1;
>       }
>
> -    if (!(cmd = umlBuildCommandLine(conn, driver, vm))) {
> -        VIR_FORCE_CLOSE(logfd);
> -        virDomainConfVMNWFilterTeardown(vm);
> -        umlCleanupTapDevices(vm);
> -        return -1;
> -    }
> +    if (!(cmd = umlBuildCommandLine(conn, driver, vm)))
> +        goto cleanup;
>
>       for (i = 0 ; i<  vm->def->nconsoles ; i++) {
>           VIR_FREE(vm->def->consoles[i]->info.alias);
>           if (virAsprintf(&vm->def->consoles[i]->info.alias,
> "console%zu", i)<  0) {
>               virReportOOMError();
> -            VIR_FORCE_CLOSE(logfd);
>               goto cleanup;
>           }
>       }
>
>       virCommandWriteArgLog(cmd, logfd);
>
>       priv->monitor = -1;
>
>       virCommandClearCaps(cmd);
>       virCommandSetOutputFD(cmd,&logfd);
>       virCommandSetErrorFD(cmd,&logfd);
>       virCommandDaemonize(cmd);
>
>       ret = virCommandRun(cmd, NULL);
> -    VIR_FORCE_CLOSE(logfd);
>       if (ret<  0)
>           goto cleanup;
>
>       if (autoDestroy&&
> -        umlProcessAutoDestroyAdd(driver, vm, conn)<  0)
> +        (ret = umlProcessAutoDestroyAdd(driver, vm, conn))<  0)
>           goto cleanup;
>
>       ret = virDomainObjSetDefTransient(driver->caps, vm, false);
>   cleanup:
> +    VIR_FORCE_CLOSE(logfd);
>       virCommandFree(cmd);
>
>       if (ret<  0) {
>           virDomainConfVMNWFilterTeardown(vm);
>           umlCleanupTapDevices(vm);
>           if (vm->newDef) {
>               virDomainDefFree(vm->def);
>               vm->def = vm->newDef;
>               vm->def->id = -1;
>               vm->newDef = NULL;
>
>