Libguestfs - Mar 2017 - [PATCH 6/6] p2v: fix possible close(-1) issue

Hi,

this patch series fixes few more issues discovered by Coverity.

Thanks,

Pino Toscano (6):
  tail: check the return value pf guestfs_set_pgroup
  daemon: btrfs: check end_stringsbuf return values everywhere
  java: use cleanup handlers for structs (lists) as return values
  lib: qemu: improve handling of FILE*
  p2v: check more return values
  p2v: fix possible close(-1) issue

 cat/tail.c        |   5 ++-
 daemon/btrfs.c    |   3 +-
 generator/java.ml |   8 ++---
 lib/qemu.c        | 104 +++++++++++++++++++++++++++---------------------------
 p2v/nbd.c         |  19 +++++++---
 5 files changed, 77 insertions(+), 62 deletions(-)

-- 
2.9.3

It is done when creating the handle, so do it also when duplicating the
handle.
---
 cat/tail.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/cat/tail.c b/cat/tail.c
index 51da5fc..ef5bbb1 100644
--- a/cat/tail.c
+++ b/cat/tail.c
@@ -495,7 +495,10 @@ reopen_handle (void)
 
   guestfs_set_verbose (g2, guestfs_get_verbose (g));
   guestfs_set_trace (g2, guestfs_get_trace (g));
-  guestfs_set_pgroup (g2, guestfs_get_pgroup (g));
+  if (guestfs_set_pgroup (g2, guestfs_get_pgroup (g)) == -1) {
+    perror ("guestfs_set_pgroup");
+    return -1;
+  }
 
   guestfs_close (g);
   g = g2;
-- 
2.9.3

Make sure to check the return value of end_stringsbuf everywhere, as
that would generate invalid string lists.
---
 daemon/btrfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/daemon/btrfs.c b/daemon/btrfs.c
index 711f7eb..d18f518 100644
--- a/daemon/btrfs.c
+++ b/daemon/btrfs.c
@@ -2280,7 +2280,8 @@ do_btrfs_filesystem_show (const char *device)
     }
   }
 
-  end_stringsbuf (&ret);
+  if (end_stringsbuf (&ret) == -1)
+    return NULL;
 
   return take_stringsbuf (&ret);
 }
-- 
2.9.3

Filling some of their fields may cause the flow to skip to throw the
"out of memory" exception, and return immediately.  To avoid leaking
the
struct, or struct list, from the C implementation, use a cleanup handler
so there is no need to manually clean it up.
---
 generator/java.ml | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/generator/java.ml b/generator/java.ml
index ea35aa2..0bbb559 100644
--- a/generator/java.ml
+++ b/generator/java.ml
@@ -696,13 +696,15 @@ throw_out_of_memory (JNIEnv *env, const char *msg)
            pr "  jobject jr;\n";
            pr "  jclass cl;\n";
            pr "  jfieldID fl;\n";
-           pr "  struct guestfs_%s *r;\n" typ
+           pr "  CLEANUP_FREE_%s struct guestfs_%s *r = NULL;\n"
+             (String.uppercase_ascii typ) typ
        | RStructList (_, typ) ->
            pr "  jobjectArray jr;\n";
            pr "  jclass cl;\n";
            pr "  jfieldID fl;\n";
            pr "  jobject jfl;\n";
-           pr "  struct guestfs_%s_list *r;\n" typ
+           pr "  CLEANUP_FREE_%s_LIST struct guestfs_%s_list *r =
NULL;\n"
+             (String.uppercase_ascii typ) typ
        | RBufferOut _ ->
            pr "  jstring jr;\n";
            pr "  char *r;\n";
@@ -1008,7 +1010,6 @@ and generate_java_struct_return typ jtyp cols          pr
"  fl = (*env)->GetFieldID (env, cl, \"%s\",
\"C\");\n" name;
         pr "  (*env)->SetCharField (env, jr, fl, r->%s);\n"
name;
   ) cols;
-  pr "  guestfs_free_%s (r);\n" typ;
   pr "  return jr;\n"
 
 and generate_java_struct_list_return typ jtyp cols @@ -1069,7 +1070,6 @@ and
generate_java_struct_list_return typ jtyp cols    pr "   
(*env)->SetObjectArrayElement (env, jr, i, jfl);\n";
   pr "  }\n";
   pr "\n";
-  pr "  guestfs_free_%s_list (r);\n" typ;
   pr "  return jr;\n"
 
 and generate_java_makefile_inc () -- 
2.9.3

Create own blocks for all the parts dealing with FILE*: this way there
is no need to recycle the same FILE* variable for all the operations,
and have each block its own variable automatically cleaned up.

This also fixes a potential undefined behaviour on error: POSIX says
that after a call fclose(), a FILE* cannot be used anymore, not even
on fclose() failure. The previous behaviour for fclose == -1 was to jump
to the error label, which would then try to call fclose() again (since
the FILE* pointer was still non-null).
---
 lib/qemu.c | 104 ++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/lib/qemu.c b/lib/qemu.c
index e61bc3c..d60692f 100644
--- a/lib/qemu.c
+++ b/lib/qemu.c
@@ -80,7 +80,6 @@ guestfs_int_test_qemu (guestfs_h *g, struct version
*qemu_version)
   struct stat statbuf;
   CLEANUP_FREE char *cachedir = NULL, *qemu_stat_filename = NULL,
     *qemu_help_filename = NULL, *qemu_devices_filename = NULL;
-  FILE *fp;
   int generation;
   uint64_t prev_size, prev_mtime;
 
@@ -101,15 +100,16 @@ guestfs_int_test_qemu (guestfs_h *g, struct version
*qemu_version)
   debug (g, "checking for previously cached test results of %s, in
%s",
          g->hv, cachedir);
 
-  fp = fopen (qemu_stat_filename, "r");
-  if (fp == NULL)
-    goto do_test;
-  if (fscanf (fp, "%d %" SCNu64 " %" SCNu64,
-              &generation, &prev_size, &prev_mtime) != 3) {
-    fclose (fp);
-    goto do_test;
+  {
+    CLEANUP_FCLOSE FILE *fp = NULL;
+    fp = fopen (qemu_stat_filename, "r");
+    if (fp == NULL)
+      goto do_test;
+    if (fscanf (fp, "%d %" SCNu64 " %" SCNu64,
+                &generation, &prev_size, &prev_mtime) != 3) {
+      goto do_test;
+    }
   }
-  fclose (fp);
 
   if (generation == MEMO_GENERATION &&
       (uint64_t) statbuf.st_size == prev_size &&
@@ -153,54 +153,54 @@ guestfs_int_test_qemu (guestfs_h *g, struct version
*qemu_version)
   /* Now memoize the qemu output in the cache directory. */
   debug (g, "saving test results");
 
-  fp = fopen (qemu_help_filename, "w");
-  if (fp == NULL) {
-  help_error:
-    perrorf (g, "%s", qemu_help_filename);
-    if (fp != NULL) fclose (fp);
-    guestfs_int_free_qemu_data (data);
-    return NULL;
+  {
+    CLEANUP_FCLOSE FILE *fp = NULL;
+    fp = fopen (qemu_help_filename, "w");
+    if (fp == NULL) {
+    help_error:
+      perrorf (g, "%s", qemu_help_filename);
+      guestfs_int_free_qemu_data (data);
+      return NULL;
+    }
+    if (fprintf (fp, "%s", data->qemu_help) == -1)
+      goto help_error;
   }
-  if (fprintf (fp, "%s", data->qemu_help) == -1)
-    goto help_error;
-  if (fclose (fp) == -1)
-    goto help_error;
 
-  fp = fopen (qemu_devices_filename, "w");
-  if (fp == NULL) {
-  devices_error:
-    perrorf (g, "%s", qemu_devices_filename);
-    if (fp != NULL) fclose (fp);
-    guestfs_int_free_qemu_data (data);
-    return NULL;
+  {
+    CLEANUP_FCLOSE FILE *fp = NULL;
+    fp = fopen (qemu_devices_filename, "w");
+    if (fp == NULL) {
+    devices_error:
+      perrorf (g, "%s", qemu_devices_filename);
+      guestfs_int_free_qemu_data (data);
+      return NULL;
+    }
+    if (fprintf (fp, "%s", data->qemu_devices) == -1)
+      goto devices_error;
   }
-  if (fprintf (fp, "%s", data->qemu_devices) == -1)
-    goto devices_error;
-  if (fclose (fp) == -1)
-    goto devices_error;
 
-  /* Write the qemu.stat file last so that its presence indicates that
-   * the qemu.help and qemu.devices files ought to exist.
-   */
-  fp = fopen (qemu_stat_filename, "w");
-  if (fp == NULL) {
-  stat_error:
-    perrorf (g, "%s", qemu_stat_filename);
-    if (fp != NULL) fclose (fp);
-    guestfs_int_free_qemu_data (data);
-    return NULL;
+  {
+    /* Write the qemu.stat file last so that its presence indicates that
+     * the qemu.help and qemu.devices files ought to exist.
+     */
+    CLEANUP_FCLOSE FILE *fp = NULL;
+    fp = fopen (qemu_stat_filename, "w");
+    if (fp == NULL) {
+    stat_error:
+      perrorf (g, "%s", qemu_stat_filename);
+      guestfs_int_free_qemu_data (data);
+      return NULL;
+    }
+    /* The path to qemu is stored for information only, it is not
+     * used when we parse the file.
+     */
+    if (fprintf (fp, "%d %" PRIu64 " %" PRIu64 "
%s\n",
+                 MEMO_GENERATION,
+                 (uint64_t) statbuf.st_size,
+                 (uint64_t) statbuf.st_mtime,
+                 g->hv) == -1)
+      goto stat_error;
   }
-  /* The path to qemu is stored for information only, it is not
-   * used when we parse the file.
-   */
-  if (fprintf (fp, "%d %" PRIu64 " %" PRIu64 "
%s\n",
-               MEMO_GENERATION,
-               (uint64_t) statbuf.st_size,
-               (uint64_t) statbuf.st_mtime,
-               g->hv) == -1)
-    goto stat_error;
-  if (fclose (fp) == -1)
-    goto stat_error;
 
   return data;
 }
-- 
2.9.3

Check for the (rare) failures of open(/dev/null), and setsockopt.
---
 p2v/nbd.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/p2v/nbd.c b/p2v/nbd.c
index b1caf2f..ee21005 100644
--- a/p2v/nbd.c
+++ b/p2v/nbd.c
@@ -401,7 +401,10 @@ start_qemu_nbd (const char *device,
 
   if (pid == 0) {               /* Child. */
     close (0);
-    open ("/dev/null", O_RDONLY);
+    if (open ("/dev/null", O_RDONLY) == -1) {
+      perror ("open: /dev/null");
+      _exit (EXIT_FAILURE);
+    }
 
     if (fds == NULL) {          /* without socket activation */
       execlp ("qemu-nbd",
@@ -474,7 +477,10 @@ start_nbdkit (const char *device,
 
   if (pid == 0) {               /* Child. */
     close (0);
-    open ("/dev/null", O_RDONLY);
+    if (open ("/dev/null", O_RDONLY) == -1) {
+      perror ("open: /dev/null");
+      _exit (EXIT_FAILURE);
+    }
 
     if (fds == NULL) {         /* without socket activation */
       execlp ("nbdkit",
@@ -691,7 +697,11 @@ wait_for_nbd_server_to_start (const char *ipaddr, int port)
 
   time (&now_t);
   timeout.tv_sec = (start_t + WAIT_NBD_TIMEOUT) - now_t;
-  setsockopt (sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof timeout);
+  if (setsockopt (sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof
timeout) == -1) {
+    set_nbd_error ("waiting for NBD server to start: "
+                   "setsockopt(SO_RCVTIMEO): %m");
+    goto cleanup;
+  }
 
   do {
     recvd = recv (sockfd, magic, sizeof magic - bytes_read, 0);
-- 
2.9.3

Make sure the error handler (i.e. the code after the 'cleanup' label)
does not attempt to call close(-1), in case 'sockfd' is not initialized
yet.
---
 p2v/nbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/p2v/nbd.c b/p2v/nbd.c
index ee21005..29ca40d 100644
--- a/p2v/nbd.c
+++ b/p2v/nbd.c
@@ -722,7 +722,8 @@ wait_for_nbd_server_to_start (const char *ipaddr, int port)
 
   result = 0;
  cleanup:
-  close (sockfd);
+  if (sockfd >= 0)
+    close (sockfd);
 
   return result;
 }
-- 
2.9.3

ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v