[PATCH 00/11] Various Coverity fixes
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
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 Friday, 3 March 2017 16:05:34 CET Richard W.M. Jones wrote:
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?
CLEANUP_FREE was used (before this patch series) only for FBuffer elements of structs, used only in the "xattr" struct; only few APIs return a list of "xattr" structs: lxattrlist, getxattrs, lgetxattrs, and internal_lxattrlist, and none of them is tested in Java tests. -- Pino Toscano
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
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
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
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 Friday, 3 March 2017 16:09:33 CET Richard W.M. Jones wrote:
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)
This could work for OCaml and Ruby, because NULL as return value is not mishandled, and they throw already errors. In the Python implementation, NULL short-circuits the guestfs_int_py_close flow. I'll change further the implementations, so when there are no handlers no memory is allocated at all. Thanks, -- Pino Toscano
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
participants (2)
-
Pino Toscano -
Richard W.M. Jones