Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/11/13 19:04, Andrew Cooper wrote:
> On 21/11/13 18:50, Ian Jackson wrote:
>> Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"):
>>> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"):
>>>> Coverity-ID: 1130517 and 1130518
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> I'm don't think that's the right fix.  I think the fds are leaked in
>>> the success case too.  How about this ?
>> Maybe you'd prefer a version which at least compiles...
>>
>> Ian.
>>
>> From: Ian Jackson <ian.jackson@eu.citrix.com>
>> Date: Thu, 21 Nov 2013 18:37:16 +0000
>> Subject: [PATCH v2] libxl: libxl__spawn_qdisk_backend closes fds
>>
>> This function needs to close both null and logfile_w on both error and
>> normal exits.  (The child gets its own copy during the fork, and the
>> parent doesn't need them any more.)
>>
>> Use the standard initialise-to-unallocated, always-free style.  As a
>> result the label "error" becomes "out", and only makes the callback if
>> rc is nonzero.
>>
>> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Coverity-ID: 1130517 and 1130518
>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
> Right - this clarifies my question about cleanup on the success case.
>
>> ---
>>  tools/libxl/libxl_dm.c |   19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 292e351..548378d 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>>      flexarray_t *dm_args;
>>      char **args;
>>      const char *dm;
>> -    int logfile_w, null, rc;
>> +    int logfile_w = -1, null = -1, rc;
> The rc logic is a little awkward.  Would it be better to initialise to
> -1 here...
>
>>      uint32_t domid = dmss->guest_domid;
>>  
>>      /* Always use qemu-xen as device model */
>> @@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>>      logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", domid));
>>      if (logfile_w < 0) {
>>          rc = logfile_w;
> ... and avoid this somewhat odd assignment?
>
> ~Andrew
>
>> -        goto error;
>> +        goto out;
>>      }
>>      null = open("/dev/null", O_RDONLY);

And thinking about it, this open() should also have some error checking.

~Andrew

>>  
>> @@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>>      dmss->spawn.detached_cb = device_model_detached;
>>      rc = libxl__spawn_spawn(egc, &dmss->spawn);
>>      if (rc < 0)
>> -        goto error;
>> +        goto out;
>>      if (!rc) { /* inner child */
>>          setsid();
>>          libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
>>      }
>>  
>> -    return;
>> +    rc = 0;
>>  
>> -error:
>> -    assert(rc);
>> -    dmss->callback(egc, dmss, rc);
>> + out:
>> +    if (logfile_w >= 0) close(logfile_w);
>> +    if (null >= 0) close(null);
>> +
>> +    /* rc is nonzero iff we had an error; if we had no error then
>> +     * spawn succeeded and we will continue in a further callback */
>> +    if (rc)
>> +        dmss->callback(egc, dmss, rc);
>>      return;
>>  }
>>  
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel