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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
Coverity-ID: 1130517 and 1130518
Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>
Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
---
 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;
     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;
-        goto error;
+        goto out;
     }
     null = open("/dev/null", O_RDONLY);
 
@@ -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;
 }
 
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel