Libguestfs - Mar 2017 - [PATCH v2 0/4] Avoid 0-bytes malloc in bindings

Hi,

some of the bindings may try to malloc with 0 bytes as size when closing
an handle, because there were no event handlers registered.  Since this
can have different behaviours in POSIX, avoid that situation altogether
by just skipping allocating anything when there were no event handlers.

Thanks,

Pino Toscano (4):
  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

 java/handle.c             | 17 ++++++++++++-----
 ocaml/guestfs-c.c         | 17 ++++++++++++-----
 python/handle.c           | 22 +++++++++++++++++-----
 ruby/ext/guestfs/handle.c | 17 ++++++++++++-----
 4 files changed, 53 insertions(+), 20 deletions(-)

-- 
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.

Short-circuit get_all_event_callbacks to allocate nothing when there are
no events, making sure to use its results only when there were events.
---
 ocaml/guestfs-c.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/ocaml/guestfs-c.c b/ocaml/guestfs-c.c
index 9042752..0df5775 100644
--- a/ocaml/guestfs-c.c
+++ b/ocaml/guestfs-c.c
@@ -75,7 +75,7 @@ guestfs_finalize (value gv)
      * user deletes events in one of the callbacks that we are
      * about to invoke, resulting in a double-free.  XXX
      */
-    size_t len, i;
+    size_t len;
     value **roots = get_all_event_callbacks (g, &len);
 
     /* Close the handle: this could invoke callbacks from the list
@@ -85,11 +85,14 @@ guestfs_finalize (value gv)
     guestfs_close (g);
 
     /* Now unregister the global roots. */
-    for (i = 0; i < len; ++i) {
-      caml_remove_generational_global_root (roots[i]);
-      free (roots[i]);
+    if (len > 0) {
+      size_t i;
+      for (i = 0; i < len; ++i) {
+        caml_remove_generational_global_root (roots[i]);
+        free (roots[i]);
+      }
+      free (roots);
     }
-    free (roots);
   }
 }
 
@@ -310,6 +313,10 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
     root = guestfs_next_private (g, &key);
   }
 
+  /* No events, so no need to allocate anything. */
+  if (*len_rtn == 0)
+    return NULL;
+
   /* Copy them into the return array. */
   r = malloc (sizeof (value *) * (*len_rtn));
   if (r == NULL) caml_raise_out_of_memory ();
-- 
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.

Short-circuit get_all_event_callbacks to allocate nothing when there are
no events, making sure to use its results only when there were events.
---
 python/handle.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/python/handle.c b/python/handle.c
index f4cc8ca..806408f 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -71,7 +71,7 @@ guestfs_int_py_close (PyObject *self, PyObject *args)
   PyThreadState *py_save = NULL;
   PyObject *py_g;
   guestfs_h *g;
-  size_t i, len;
+  size_t len;
   PyObject **callbacks;
 
   if (!PyArg_ParseTuple (args, (char *) "O:guestfs_close",
&py_g))
@@ -81,9 +81,14 @@ guestfs_int_py_close (PyObject *self, PyObject *args)
   /* As in the OCaml bindings, there is a hard to solve case where the
    * caller can delete a callback from within the callback, resulting
    * in a double-free here.  XXX
+   *
+   * Take care of the result of get_all_event_callbacks: NULL can be
+   * both an error (and some PyErr_* was called), and no events.
+   * 'len' is specifically 0 only in the latter case, so filter that
+   * out.
    */
   callbacks = get_all_event_callbacks (g, &len);
-  if (callbacks == NULL)
+  if (len != 0 && callbacks == NULL)
     return NULL;
 
   if (PyEval_ThreadsInitialized ())
@@ -92,9 +97,12 @@ guestfs_int_py_close (PyObject *self, PyObject *args)
   if (PyEval_ThreadsInitialized ())
     PyEval_RestoreThread (py_save);
 
-  for (i = 0; i < len; ++i)
-    Py_XDECREF (callbacks[i]);
-  free (callbacks);
+  if (len > 0) {
+    size_t i;
+    for (i = 0; i < len; ++i)
+      Py_XDECREF (callbacks[i]);
+    free (callbacks);
+  }
 
   Py_INCREF (Py_None);
   return Py_None;
@@ -260,6 +268,10 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
     cb = guestfs_next_private (g, &key);
   }
 
+  /* No events, so no need to allocate anything. */
+  if (*len_rtn == 0)
+    return NULL;
+
   /* Copy them into the return array. */
   r = malloc (sizeof (PyObject *) * (*len_rtn));
   if (r == NULL) {
-- 
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.

Short-circuit get_all_event_callbacks to allocate nothing when there are
no events, making sure to use its results only when there were events.
---
 ruby/ext/guestfs/handle.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/ruby/ext/guestfs/handle.c b/ruby/ext/guestfs/handle.c
index aa10825..b04192d 100644
--- a/ruby/ext/guestfs/handle.c
+++ b/ruby/ext/guestfs/handle.c
@@ -54,7 +54,7 @@ free_handle (void *gvp)
      * the callbacks that we are about to invoke, resulting in
      * a double-free.  XXX
      */
-    size_t len, i;
+    size_t len;
     VALUE **roots = get_all_event_callbacks (g, &len);
 
     /* Close the handle: this could invoke callbacks from the list
@@ -64,11 +64,14 @@ free_handle (void *gvp)
     guestfs_close (g);
 
     /* Now unregister the global roots. */
-    for (i = 0; i < len; ++i) {
-      rb_gc_unregister_address (roots[i]);
-      free (roots[i]);
+    if (len > 0) {
+      size_t i;
+      for (i = 0; i < len; ++i) {
+        rb_gc_unregister_address (roots[i]);
+        free (roots[i]);
+      }
+      free (roots);
     }
-    free (roots);
   }
 }
 
@@ -384,6 +387,10 @@ get_all_event_callbacks (guestfs_h *g, size_t *len_rtn)
     root = guestfs_next_private (g, &key);
   }
 
+  /* No events, so no need to allocate anything. */
+  if (*len_rtn == 0)
+    return NULL;
+
   /* Copy them into the return array. */
   r = malloc (sizeof (VALUE *) * (*len_rtn));
   if (r == NULL)
-- 
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.

Short-circuit get_all_event_callbacks to allocate nothing when there are
no events, making sure to use its results only when there were events.
---
 java/handle.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/java/handle.c b/java/handle.c
index f225361..0993f33 100644
--- a/java/handle.c
+++ b/java/handle.c
@@ -85,7 +85,7 @@ Java_com_redhat_et_libguestfs_GuestFS__1close
   (JNIEnv *env, jobject obj, jlong jg)
 {
   guestfs_h *g = (guestfs_h *) (long) jg;
-  size_t len, i;
+  size_t len;
   struct callback_data **data;
 
   /* There is a nasty, difficult to solve case here where the
@@ -96,11 +96,14 @@ Java_com_redhat_et_libguestfs_GuestFS__1close
 
   guestfs_close (g);
 
-  for (i = 0; i < len; ++i) {
-    (*env)->DeleteGlobalRef (env, data[i]->callback);
-    free (data[i]);
+  if (len > 0) {
+    size_t i;
+    for (i = 0; i < len; ++i) {
+      (*env)->DeleteGlobalRef (env, data[i]->callback);
+      free (data[i]);
+    }
+    free (data);
   }
-  free (data);
 }
 
 /* See EventCallback interface. */
@@ -274,6 +277,10 @@ get_all_event_callbacks (JNIEnv *env, guestfs_h *g, size_t
*len_rtn)
     data = guestfs_next_private (g, &key);
   }
 
+  /* No events, so no need to allocate anything. */
+  if (*len_rtn == 0)
+    return NULL;
+
   /* Copy them into the return array. */
   r = malloc (sizeof (struct callback_data *) * (*len_rtn));
   if (r == NULL) {
-- 
2.9.3

On Fri, Mar 03, 2017 at 06:13:15PM +0100, Pino Toscano
wrote:
> Hi,
> 
> some of the bindings may try to malloc with 0 bytes as size when closing
> an handle, because there were no event handlers registered.  Since this
> can have different behaviours in POSIX, avoid that situation altogether
> by just skipping allocating anything when there were no event handlers.
Looks good, 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