Libguestfs - Mar 2017 - Re: [PATCH 01/11] java: link libguestfs_jni against libutils

Hi,

this patch series fixes some issues discovered by Coverity.
Most of them are memory leaks, usually on error; there are also invalid
memory access issues.

Thanks,

Pino Toscano (11):
  java: link libguestfs_jni against libutils
  java: fix invalid memory access for FBuffer in struct lists
  daemon: tsk: properly use GUESTFS_MAX_CHUNK_SIZE
  edit: fix small memory leak on error
  java: fix possible memory leak on error
  fish: fully init the msghdr buffers
  tail: pass the right path for Windows guests
  ocaml: do not try to malloc 0 elements in get_all_event_callbacks
  python: do not try to malloc 0 elements in get_all_event_callbacks
  ruby: do not try to malloc 0 elements in get_all_event_callbacks
  java: do not try to malloc 0 elements in get_all_event_callbacks

 cat/tail.c                | 2 +-
 common/edit/file-edit.c   | 1 +
 daemon/sleuthkit.c        | 2 +-
 fish/rc.c                 | 9 +++------
 generator/java.ml         | 8 +++-----
 java/Makefile.am          | 4 +++-
 java/handle.c             | 3 ++-
 ocaml/guestfs-c.c         | 3 ++-
 python/handle.c           | 3 ++-
 ruby/ext/guestfs/handle.c | 3 ++-
 10 files changed, 20 insertions(+), 18 deletions(-)

-- 
2.9.3

The JNI library uses CLEANUP_FREE macros, whose functions are built in
the internal libutils.  Currently, trying to use functions that use
CLEANUP_FREE variables will cause the java execution to stop with a
symbol lookup error (for guestfs_int_cleanup_free).
---
 java/Makefile.am | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/java/Makefile.am b/java/Makefile.am
index 06d0fc7..ea64f55 100644
--- a/java/Makefile.am
+++ b/java/Makefile.am
@@ -115,7 +115,9 @@ libguestfs_jni_la_CFLAGS = \
 	$(WARN_CFLAGS) $(WERROR_CFLAGS) \
 	$(JNI_CFLAGS)
 
-libguestfs_jni_la_LIBADD = $(top_builddir)/lib/libguestfs.la
+libguestfs_jni_la_LIBADD = \
+	$(top_builddir)/common/utils/libutils.la \
+	$(top_builddir)/lib/libguestfs.la
 
 libguestfs_jni_la_LDFLAGS = -version-info $(JNI_VERSION_INFO) -shared
 
-- 
2.9.3

When convering FBuffer fields of structs in each element of the return
list, make sure to allocate enough buffer to hold also the trailing null
character.
---
 generator/java.ml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator/java.ml b/generator/java.ml
index 8f71feb..2606f97 100644
--- a/generator/java.ml
+++ b/generator/java.ml
@@ -1038,7 +1038,7 @@ and generate_java_struct_list_return typ jtyp cols       
| FBuffer ->
         pr "    {\n";
         pr "      size_t len = r->val[i].%s_len;\n" name;
-        pr "      CLEANUP_FREE char *s = malloc (len);\n";
+        pr "      CLEANUP_FREE char *s = malloc (len + 1);\n";
         pr "      if (s == NULL) {\n";
         pr "        throw_out_of_memory (env,
\"malloc\");\n";
         pr "        goto ret_error;\n";
-- 
2.9.3

Pass to fread the size of the buffer allocated, so it can be filled
completely. This is even faster than reading only 4/8 bytes each
iteration.
---
 daemon/sleuthkit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemon/sleuthkit.c b/daemon/sleuthkit.c
index e642731..bdbdb0f 100644
--- a/daemon/sleuthkit.c
+++ b/daemon/sleuthkit.c
@@ -122,7 +122,7 @@ send_command_output (const char *cmd)
   /* Send reply message before the file content. */
   reply (NULL, NULL);
 
-  while ((ret = fread (buffer, 1, sizeof buffer, fp)) > 0) {
+  while ((ret = fread (buffer, 1, GUESTFS_MAX_CHUNK_SIZE, fp)) > 0) {
     ret = send_file_write (buffer, ret);
     if (ret < 0) {
       pclose (fp);
-- 
2.9.3

If guestfs_int_random_string fails, make sure to free the buffer
allocated as return string.
---
 common/edit/file-edit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/edit/file-edit.c b/common/edit/file-edit.c
index 81b1f1f..b0347e7 100644
--- a/common/edit/file-edit.c
+++ b/common/edit/file-edit.c
@@ -325,6 +325,7 @@ generate_random_name (const char *filename)
    */
   if (guestfs_int_random_string (p, 8) == -1) {
     perror ("guestfs_int_random_string");
+    free (ret);
     return NULL;
   }
 
-- 
2.9.3

Use CLEANUP_FREE to properly dispose the temporary array used for
StringList, DeviceList, FilenameList, and OStringList parameters: this
way, an early return (jumping to the ret_error label) will not forget
cleaning them up.
---
 generator/java.ml | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/generator/java.ml b/generator/java.ml
index 2606f97..66729c6 100644
--- a/generator/java.ml
+++ b/generator/java.ml
@@ -715,7 +715,7 @@ throw_out_of_memory (JNIEnv *env, const char *msg)
             pr "  size_t %s_size;\n" n
         | StringList n | DeviceList n | FilenameList n ->
             pr "  size_t %s_len;\n" n;
-            pr "  char **%s;\n" n
+            pr "  CLEANUP_FREE char **%s = NULL;\n" n
         | Bool n
         | Int n ->
             pr "  int %s;\n" n
@@ -734,7 +734,7 @@ throw_out_of_memory (JNIEnv *env, const char *msg)
           | OBool _ | OInt _ | OInt64 _ | OString _ -> ()
           | OStringList n ->
             pr "  size_t %s_len;\n" n;
-            pr "  char **%s;\n" n
+            pr "  CLEANUP_FREE char **%s = NULL;\n" n
         ) optargs
       );
 
@@ -857,7 +857,6 @@ throw_out_of_memory (JNIEnv *env, const char *msg)
               n;
             pr "    (*env)->ReleaseStringUTFChars (env, o,
%s[i]);\n" n;
             pr "  }\n";
-            pr "  free (%s);\n" n
         | Bool _
         | Int _
         | Int64 _
@@ -875,7 +874,6 @@ throw_out_of_memory (JNIEnv *env, const char *msg)
               n;
             pr "    (*env)->ReleaseStringUTFChars (env, o,
optargs_s.%s[i]);\n" n;
             pr "  }\n";
-            pr "  free (%s);\n" n
       ) optargs;
 
       pr "\n";
-- 
2.9.3

This way no fields (msg_flags in particular) remain uninitialized.
---
 fish/rc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fish/rc.c b/fish/rc.c
index 9c6dec8..2517921 100644
--- a/fish/rc.c
+++ b/fish/rc.c
@@ -101,14 +101,13 @@ receive_stdout (int s)
   int fd;
   char buf[1];
 
+  memset (&msg, 0, sizeof msg);
+
   msg.msg_iov = &iov;
   msg.msg_iovlen = 1;
   iov.iov_base = buf;
   iov.iov_len = sizeof buf;
 
-  msg.msg_name = NULL;
-  msg.msg_namelen = 0;
-
   msg.msg_control = control_un.control;
   msg.msg_controllen = sizeof (control_un.control);
 
@@ -163,6 +162,7 @@ send_stdout (int s)
    * It's unclear if this is hiding a real problem or not.  XXX
    */
   memset (&control_un, 0, sizeof control_un);
+  memset (&msg, 0, sizeof msg);
 
   /* On Linux you have to transmit at least 1 byte of real data. */
   msg.msg_iov = &iov;
@@ -171,9 +171,6 @@ send_stdout (int s)
   iov.iov_base = buf;
   iov.iov_len = sizeof buf;
 
-  msg.msg_name = NULL;
-  msg.msg_namelen = 0;
-
   msg.msg_control = control_un.control;
   msg.msg_controllen = sizeof (control_un.control);
 
-- 
2.9.3

Call windows_path with the actual path to resolve, instead of a null
pointer ('filename' is still null at that point), so Windows paths can
be properly resolved.
---
 cat/tail.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cat/tail.c b/cat/tail.c
index 8785d45..51da5fc 100644
--- a/cat/tail.c
+++ b/cat/tail.c
@@ -309,7 +309,7 @@ do_tail (int argc, char *argv[], /* list of files in the
guest */
       CLEANUP_FREE_STATNS struct guestfs_statns *stat = NULL;
 
       if (windows) {
-        filename = windows_path (g, root, filename, 1 /* readonly */);
+        filename = windows_path (g, root, argv[i], 1 /* readonly */);
         if (filename == NULL)
           return -1; /* windows_path printed an error */
       }
-- 
2.9.3

In case there are no event handlers registered with the handle,
get_all_event_callbacks will count 0 elements, trying to malloc a buffer
of that size.  POSIX says that this can result in either a null pointer,
or an unusable pointer.  Since we assume a null pointer means failure,
then always add a null element at the end, so we do not rely on
implementation-defined behaviour of malloc.

The output parameter 'len_rtn' already keeps the number of valid items
in the returned array, so there are no behaviour changes for callers of
get_all_event_callbacks.
---
 ocaml/guestfs-c.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ocaml/guestfs-c.c b/ocaml/guestfs-c.c
index 9042752..f14eee3 100644
--- a/ocaml/guestfs-c.c
+++ b/ocaml/guestfs-c.c
@@ -311,7 +311,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
   }
 
   /* Copy them into the return array. */
-  r = malloc (sizeof (value *) * (*len_rtn));
+  r = malloc (sizeof (value *) * (*len_rtn + 1));
   if (r == NULL) caml_raise_out_of_memory ();
 
   i = 0;
@@ -323,6 +323,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
     }
     root = guestfs_next_private (g, &key);
   }
+  r[i] = NULL;
 
   return r;
 }
-- 
2.9.3

In case there are no event handlers registered with the handle,
get_all_event_callbacks will count 0 elements, trying to malloc a buffer
of that size.  POSIX says that this can result in either a null pointer,
or an unusable pointer.  Since we assume a null pointer means failure,
then always add a null element at the end, so we do not rely on
implementation-defined behaviour of malloc.

The output parameter 'len_rtn' already keeps the number of valid items
in the returned array, so there are no behaviour changes for callers of
get_all_event_callbacks.
---
 python/handle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/python/handle.c b/python/handle.c
index f4cc8ca..ef2fdd8 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -261,7 +261,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
   }
 
   /* Copy them into the return array. */
-  r = malloc (sizeof (PyObject *) * (*len_rtn));
+  r = malloc (sizeof (PyObject *) * (*len_rtn + 1));
   if (r == NULL) {
     PyErr_SetNone (PyExc_MemoryError);
     return NULL;
@@ -276,6 +276,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
     }
     cb = guestfs_next_private (g, &key);
   }
+  r[i] = NULL;
 
   return r;
 }
-- 
2.9.3

In case there are no event handlers registered with the handle,
get_all_event_callbacks will count 0 elements, trying to malloc a buffer
of that size.  POSIX says that this can result in either a null pointer,
or an unusable pointer.  Since we assume a null pointer means failure,
then always add a null element at the end, so we do not rely on
implementation-defined behaviour of malloc.

The output parameter 'len_rtn' already keeps the number of valid items
in the returned array, so there are no behaviour changes for callers of
get_all_event_callbacks.
---
 ruby/ext/guestfs/handle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ruby/ext/guestfs/handle.c b/ruby/ext/guestfs/handle.c
index aa10825..0fa230c 100644
--- a/ruby/ext/guestfs/handle.c
+++ b/ruby/ext/guestfs/handle.c
@@ -385,7 +385,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
   }
 
   /* Copy them into the return array. */
-  r = malloc (sizeof (VALUE *) * (*len_rtn));
+  r = malloc (sizeof (VALUE *) * (*len_rtn + 1));
   if (r == NULL)
     rb_raise (rb_eNoMemError, "malloc: %m");
 
@@ -398,6 +398,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
     }
     root = guestfs_next_private (g, &key);
   }
+  r[i] = NULL;
 
   return r;
 }
-- 
2.9.3

In case there are no event handlers registered with the handle,
get_all_event_callbacks will count 0 elements, trying to malloc a buffer
of that size.  POSIX says that this can result in either a null pointer,
or an unusable pointer.  Since we assume a null pointer means failure,
then always add a null element at the end, so we do not rely on
implementation-defined behaviour of malloc.

The output parameter 'len_rtn' already keeps the number of valid items
in the returned array, so there are no behaviour changes for callers of
get_all_event_callbacks.
---
 java/handle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/java/handle.c b/java/handle.c
index f225361..beb8bdc 100644
--- a/java/handle.c
+++ b/java/handle.c
@@ -275,7 +275,7 @@ get_all_event_callbacks (JNIEnv *env, guestfs_h *g, size_t
*len_rtn)
   }
 
   /* Copy them into the return array. */
-  r = malloc (sizeof (struct callback_data *) * (*len_rtn));
+  r = malloc (sizeof (struct callback_data *) * (*len_rtn + 1));
   if (r == NULL) {
     throw_out_of_memory (env, "malloc");
     return NULL;
@@ -290,6 +290,7 @@ get_all_event_callbacks (JNIEnv *env, guestfs_h *g, size_t
*len_rtn)
     }
     data = guestfs_next_private (g, &key);
   }
+  r[i] = NULL;
 
   return r;
 }
-- 
2.9.3

On Fri, Mar 03, 2017 at 03:32:55PM +0100, Pino Toscano
wrote:
> The JNI library uses CLEANUP_FREE macros, whose functions are built in
> the internal libutils.  Currently, trying to use functions that use
> CLEANUP_FREE variables will cause the java execution to stop with a
> symbol lookup error (for guestfs_int_cleanup_free).
[comment only]
I wonder why our tests didn't pick this up?

Rich.
>  java/Makefile.am | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/java/Makefile.am b/java/Makefile.am
> index 06d0fc7..ea64f55 100644
> --- a/java/Makefile.am
> +++ b/java/Makefile.am
> @@ -115,7 +115,9 @@ libguestfs_jni_la_CFLAGS = \
>  	$(WARN_CFLAGS) $(WERROR_CFLAGS) \
>  	$(JNI_CFLAGS)
>  
> -libguestfs_jni_la_LIBADD = $(top_builddir)/lib/libguestfs.la
> +libguestfs_jni_la_LIBADD = \
> +	$(top_builddir)/common/utils/libutils.la \
> +	$(top_builddir)/lib/libguestfs.la
>  
>  libguestfs_jni_la_LDFLAGS = -version-info $(JNI_VERSION_INFO) -shared
>  
> -- 
> 2.9.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Fri, Mar 03, 2017 at 03:33:02PM +0100, Pino Toscano
wrote:
> In case there are no event handlers registered with the handle,
> get_all_event_callbacks will count 0 elements, trying to malloc a buffer
> of that size.  POSIX says that this can result in either a null pointer,
> or an unusable pointer.  Since we assume a null pointer means failure,
> then always add a null element at the end, so we do not rely on
> implementation-defined behaviour of malloc.
> 
> The output parameter 'len_rtn' already keeps the number of valid
items
> in the returned array, so there are no behaviour changes for callers of
> get_all_event_callbacks.
> ---
>  ocaml/guestfs-c.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ocaml/guestfs-c.c b/ocaml/guestfs-c.c
> index 9042752..f14eee3 100644
> --- a/ocaml/guestfs-c.c
> +++ b/ocaml/guestfs-c.c
> @@ -311,7 +311,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
>    }
>  
>    /* Copy them into the return array. */
> -  r = malloc (sizeof (value *) * (*len_rtn));
> +  r = malloc (sizeof (value *) * (*len_rtn + 1));
>    if (r == NULL) caml_raise_out_of_memory ();
Isn't it better to fix this by doing:

   r = malloc (sizeof (value *) * (*len_rtn));
-  if (r == NULL) caml_raise_out_of_memory ();
+  if (*len_rtn > 0 && r == NULL) caml_raise_out_of_memory ();

(same comment in the following patches)

Rich.
>  
>    i = 0;
> @@ -323,6 +323,7 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
>      }
>      root = guestfs_next_private (g, &key);
>    }
> +  r[i] = NULL;
>  
>    return r;
>  }
> -- 
> 2.9.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Fri, Mar 03, 2017 at 03:33:01PM +0100, Pino Toscano
wrote:
> Call windows_path with the actual path to resolve, instead of a null
> pointer ('filename' is still null at that point), so Windows paths
can
> be properly resolved.
> ---
>  cat/tail.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cat/tail.c b/cat/tail.c
> index 8785d45..51da5fc 100644
> --- a/cat/tail.c
> +++ b/cat/tail.c
> @@ -309,7 +309,7 @@ do_tail (int argc, char *argv[], /* list of files in
the guest */
>        CLEANUP_FREE_STATNS struct guestfs_statns *stat = NULL;
>  
>        if (windows) {
> -        filename = windows_path (g, root, filename, 1 /* readonly */);
> +        filename = windows_path (g, root, argv[i], 1 /* readonly */);
>          if (filename == NULL)
>            return -1; /* windows_path printed an error */
>        }
ACK patches 1-7.

I made a comment about patches 8-11.

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