[PATCH] daemon: fix memory allocation and leaks in OCaml stubs
by Pino Toscano
Use the cleanup handlers to free the structs (and list of structs) in
case their OCaml->C transformation fails for any reason; use calloc()
to not try to use uninitialized memory.
In case of lists, avoid allocating the memory for the array if there
are no elements, since the returned pointer in that case is either NULL,
or a free()-only pointer; also, set the list size only after the array
is allocated, to not confuse the XDR routines.
---
generator/daemon.ml | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/generator/daemon.ml b/generator/daemon.ml
index 559ed6898..265f0a475 100644
--- a/generator/daemon.ml
+++ b/generator/daemon.ml
@@ -605,16 +605,18 @@ let generate_daemon_caml_stubs () =
(* Implement code for returning structs and struct lists. *)
let emit_return_struct typ =
let struc = Structs.lookup_struct typ in
+ let uc_typ = String.uppercase_ascii typ in
pr "/* Implement RStruct (%S, _). */\n" typ;
pr "static guestfs_int_%s *\n" typ;
pr "return_%s (value retv)\n" typ;
pr "{\n";
- pr " guestfs_int_%s *ret;\n" typ;
+ pr " CLEANUP_FREE_%s guestfs_int_%s *ret = NULL;\n" uc_typ typ;
+ pr " guestfs_int_%s *real_ret;\n" typ;
pr " value v;\n";
pr "\n";
- pr " ret = malloc (sizeof (*ret));\n";
+ pr " ret = calloc (1, sizeof (*ret));\n";
pr " if (ret == NULL) {\n";
- pr " reply_with_perror (\"malloc\");\n";
+ pr " reply_with_perror (\"calloc\");\n";
pr " return NULL;\n";
pr " }\n";
pr "\n";
@@ -644,16 +646,20 @@ let generate_daemon_caml_stubs () =
pr " ret->%s = Int_val (v);\n" n
) struc.s_cols;
pr "\n";
- pr " return ret;\n";
+ pr " real_ret = ret;\n";
+ pr " ret = NULL;\n";
+ pr " return real_ret;\n";
pr "}\n";
pr "\n"
and emit_return_struct_list typ =
+ let uc_typ = String.uppercase_ascii typ in
pr "/* Implement RStructList (%S, _). */\n" typ;
pr "static guestfs_int_%s_list *\n" typ;
pr "return_%s_list (value retv)\n" typ;
pr "{\n";
- pr " guestfs_int_%s_list *ret;\n" typ;
+ pr " CLEANUP_FREE_%s_LIST guestfs_int_%s_list *ret = NULL;\n" uc_typ typ;
+ pr " guestfs_int_%s_list *real_ret;\n" typ;
pr " guestfs_int_%s *r;\n" typ;
pr " size_t i, len;\n";
pr " value v, rv;\n";
@@ -666,32 +672,35 @@ let generate_daemon_caml_stubs () =
pr " rv = Field (rv, 1);\n";
pr " }\n";
pr "\n";
- pr " ret = malloc (sizeof *ret);\n";
+ pr " ret = calloc (1, sizeof *ret);\n";
pr " if (ret == NULL) {\n";
- pr " reply_with_perror (\"malloc\");\n";
- pr " return NULL;\n";
- pr " }\n";
- pr " ret->guestfs_int_%s_list_len = len;\n" typ;
- pr " ret->guestfs_int_%s_list_val =\n" typ;
- pr " calloc (len, sizeof (guestfs_int_%s));\n" typ;
- pr " if (ret->guestfs_int_%s_list_val == NULL) {\n" typ;
pr " reply_with_perror (\"calloc\");\n";
- pr " free (ret);\n";
pr " return NULL;\n";
pr " }\n";
+ pr " if (len > 0) {\n";
+ pr " ret->guestfs_int_%s_list_val =\n" typ;
+ pr " calloc (len, sizeof (guestfs_int_%s));\n" typ;
+ pr " if (ret->guestfs_int_%s_list_val == NULL) {\n" typ;
+ pr " reply_with_perror (\"calloc\");\n";
+ pr " return NULL;\n";
+ pr " }\n";
+ pr " ret->guestfs_int_%s_list_len = len;\n" typ;
+ pr " }\n";
pr "\n";
pr " rv = retv;\n";
pr " for (i = 0; i < len; ++i) {\n";
pr " v = Field (rv, 0);\n";
pr " r = return_%s (v);\n" typ;
pr " if (r == NULL)\n";
- pr " return NULL; /* XXX leaks memory along this error path */\n";
+ pr " return NULL;\n";
pr " memcpy (&ret->guestfs_int_%s_list_val[i], r, sizeof (*r));\n" typ;
pr " free (r);\n";
pr " rv = Field (rv, 1);\n";
pr " }\n";
pr "\n";
- pr " return ret;\n";
+ pr " real_ret = ret;\n";
+ pr " ret = NULL;\n";
+ pr " return real_ret;\n";
pr "}\n";
pr "\n";
in
--
2.14.3
6 years, 6 months
[PATCH v7 0/6] daemon: list_filesystems: filter out block devices which cannot hold filesystem.
by Mykola Ivanets
This patch series addresses comments after v6 series review.
Mykola Ivanets (6):
daemon: Changing the way that we detect if a device contains
partitions.
daemon: list-filesystems: Ignore partitioned MD devices.
tests: list-filesystems command ignores partitioned MD devices.
daemon: list-filesystems: Change the way we filter out LDM partitions.
daemon: list-filesystems: Filter out Microsoft Reserved and Windows
Snapshot partitions.
daemon: list-filesystems: Filter out MBR extended partitions.
daemon/listfs.ml | 132 ++++++++++++++----------
tests/md/Makefile.am | 3 +-
tests/md/test-partitioned-md-devices.sh | 79 ++++++++++++++
3 files changed, 157 insertions(+), 57 deletions(-)
create mode 100755 tests/md/test-partitioned-md-devices.sh
--
2.17.0
6 years, 6 months
[PATCH v2] fuse: mount_local: Fix crash when called from Java binding.
by Mykola Ivanets
"localmountpoint" parameter is allocated in JNI before calling
mount_local and freed afterward. But guestfs handle keeps reference
to passed "localmountpoint" parameter and will try to access it in
umount_local and free after mount_local_run caller thread ends
which leads to a crash (an attempt to access to already freed memory).
---
lib/fuse.c | 5 +++--
lib/handle.c | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/fuse.c b/lib/fuse.c
index 9731db962..82bddec00 100644
--- a/lib/fuse.c
+++ b/lib/fuse.c
@@ -1047,7 +1047,7 @@ guestfs_impl_mount_local (guestfs_h *g, const char *localmountpoint,
/* Set g->localmountpoint in the handle. */
gl_lock_lock (mount_local_lock);
- g->localmountpoint = localmountpoint;
+ g->localmountpoint = safe_strdup (g, localmountpoint);
gl_lock_unlock (mount_local_lock);
return 0;
@@ -1090,6 +1090,7 @@ guestfs_impl_mount_local_run (guestfs_h *g)
guestfs_int_free_fuse (g);
gl_lock_lock (mount_local_lock);
+ free (g->localmountpoint);
g->localmountpoint = NULL;
gl_lock_unlock (mount_local_lock);
@@ -1148,7 +1149,7 @@ guestfs_impl_umount_local (guestfs_h *g,
return -1;
if (WIFEXITED (r) && WEXITSTATUS (r) == EXIT_SUCCESS)
/* External fusermount succeeded. Note that the original thread
- * is responsible for setting g->localmountpoint to NULL.
+ * is responsible for freeing memory and setting g->localmountpoint to NULL.
*/
return 0;
diff --git a/lib/handle.c b/lib/handle.c
index 449ab42a6..bc45d29b2 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -399,6 +399,7 @@ guestfs_close (guestfs_h *g)
free (g->hv);
free (g->backend);
free (g->backend_data);
+ free (g->localmountpoint);
guestfs_int_free_string_list (g->backend_settings);
free (g->append);
guestfs_int_free_error_data_list (g);
--
2.17.0
6 years, 6 months
[PATCH v6 0/7] daemon: list_filesystems: filter out block devices which cannot hold filesystem
by Mykola Ivanets
This patch series:
1. Addresses comments from v5 series review
2. Large commit is splitted to more granular commits for better code review.
Mykola Ivanets (6):
daemon: Changing the way that we detect if a device contains
partitions.
daemon: list-filesystems: Ignore partitioned MD devices.
tests: list-filesystems command ignores partitioned MD devices.
daemon: list-filesystems: Change the way we filter out LDM partitions.
daemon: list-filesystems: Filter out Microsoft Reserved and Windows
Snapshot partitions.
daemon: list-ilesystems: Filter out MBR extended partitions.
Nikolay Ivanets (1):
daemon: Reimplement 'part_get_mbr_part_type' API in OCaml.
daemon/listfs.ml | 127 +++++++++++++-----------
daemon/parted.c | 106 --------------------
daemon/parted.ml | 13 +++
generator/actions_core.ml | 1 +
tests/md/Makefile.am | 3 +-
tests/md/test-partitioned-md-devices.sh | 79 +++++++++++++++
6 files changed, 166 insertions(+), 163 deletions(-)
create mode 100755 tests/md/test-partitioned-md-devices.sh
--
2.17.0
6 years, 6 months
[RFC] fuse: mount_local: Fix crash when called from Java binding
by Mykola Ivanets
"localmountpoint" parameter is allocated in JNI before calling
mount_local and freed afterward. But guestfs handle keeps reference
to passed "localmountpoint" argument and will try to use and free it
in umount_local which leads to a crash because an attempt to access
already freed memory region.
It is not easy to fix on JNI side because the code is auto-generated.
And I don't think it should be fixed there.
However I doubt this patch is correct because this might lead to memory
leak for other language bindings or in C library.
I'd like to hear your thoughts how we should proceed in this situation.
---
lib/fuse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/fuse.c b/lib/fuse.c
index 9731db962..7df765b81 100644
--- a/lib/fuse.c
+++ b/lib/fuse.c
@@ -1047,7 +1047,7 @@ guestfs_impl_mount_local (guestfs_h *g, const char *localmountpoint,
/* Set g->localmountpoint in the handle. */
gl_lock_lock (mount_local_lock);
- g->localmountpoint = localmountpoint;
+ g->localmountpoint = safe_strdup(g, localmountpoint);
gl_lock_unlock (mount_local_lock);
return 0;
--
2.17.0
6 years, 6 months
[PATCH 4/4] daemon: simplify string allocation
by Pino Toscano
When creating an helper string for do_aug_match(), use a simpler
asprintf() with manual free(), since the code block is small enough.
This slightly helps static code analyzers.
---
daemon/augeas.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/daemon/augeas.c b/daemon/augeas.c
index bd54c4849..453251337 100644
--- a/daemon/augeas.c
+++ b/daemon/augeas.c
@@ -420,17 +420,15 @@ do_aug_ls (const char *path)
if (STREQ (path, "/"))
matches = do_aug_match ("/*");
else {
- CLEANUP_FREE char *buf = NULL;
+ char *buf = NULL;
- len += 3; /* / * + terminating \0 */
- buf = malloc (len);
- if (buf == NULL) {
- reply_with_perror ("malloc");
+ if (asprintf (&buf, "%s/*", path) == -1) {
+ reply_with_perror ("asprintf");
return NULL;
}
- snprintf (buf, len, "%s/*", path);
matches = do_aug_match (buf);
+ free (buf);
}
if (matches == NULL)
--
2.14.3
6 years, 6 months
[nbdkit PATCH v2] tests: Skip guestfs code on CentOS 6
by Eric Blake
CentOS 6 has libguestfs-devel 1.20.11, which predates the support
in guestfs_add_drive_opts() for requesting an nbd drive instead
of a local file (annoyingly, guestfs documentation merely states
the function was available since 0.3, without saying which later
releases added new options); causing a compilation failure during
'make check'. Maybe the guestfs plugin should still be built,
even though the tests that use guestfs can't work without support
for GUESTFS_ADD_DRIVE_OPTS_PROTOCOL; but it's easier to just
declare that on this old platform, we'll just disable guestfs
integration altogether. With that in place, 'make check' now
runs to completion, passing 14 and skipping 5 remaining tests.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
configure.ac | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/configure.ac b/configure.ac
index c9a6948..d498d05 100644
--- a/configure.ac
+++ b/configure.ac
@@ -403,10 +403,20 @@ AC_ARG_WITH([libguestfs],[
[with_libguestfs=check])
AS_IF([test "$with_libguestfs" != "no"],[
PKG_CHECK_MODULES([LIBGUESTFS], [libguestfs],[
+ # Although the library was found, we want to make sure it supports nbd
+ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
+#include <guestfs.h>
+ ]], [[
+#ifndef GUESTFS_ADD_DRIVE_OPTS_PROTOCOL
+#error unsupported
+#endif
+ ]])], [
AC_SUBST([LIBGUESTFS_CFLAGS])
AC_SUBST([LIBGUESTFS_LIBS])
AC_DEFINE([HAVE_LIBGUESTFS],[1],[libguestfs found at compile time.])
+ ],[
+ LIBGUESTFS_LIBS=
+ AC_MSG_WARN([libguestfs too old, guestfs plugin and tests will be disabled])])
],
[AC_MSG_WARN([libguestfs not found, guestfs plugin and tests will be disabled])])
])
--
2.14.3
6 years, 6 months
Curious sgdisk behavior
by Nikolay Ivanets
FYI
Just found interesting behavior of sgdisk (it is used in daemon code for
various things) which might surprise us: sgdisk won't work if it finds
"valid" MBR and GPT partition tables on the disk. In this case sgdisk fails
with "Invalid partition data!" message.
How to reproduce (esiest way is to use DISPART command line tool in Windows
but you can do the same via Disk Manager UI):
1. Have an empty disk in Windows
2. Launch DISKPART (with elevated privileges)
3. Select corresponding disk by number (SELECT DISK N)
4. Create GPT partition table (CONVERT GPT)
5. Convert to MBR partition table (CONVERT MBR)
6. Attach disk to guestfish and try using, for example, part-get-disk-guid
command on affected disk (or assign disk to a loop or nbd device and use
sgdisk on affected disk directly from your host).
In both cases you will get "Invalid partition data!".
Why it happens?
After step #4: GPT partition table at LBA1 is created and Protective MBR
partition table at LBA0 is created as well. More over Windows automatically
creates Microsoft Reserved Partition but this actually doesn't matter: we
can delete this partition or pretend it doesn't exist. At this step sgdisk
works as expected: it finds both Protective MBR and GPT partitions. Because
they both exist sgdisk correctly detects it is actually GPT partition table.
After step #5: Real MBR (with no partitions) partition table is created at
LBA0 (overwriting early created Protective MBR). But neither primary no
backup GPT partition tables is erased. Now we have two partition tables:
MBR and GPT. And at this step sgdisk fails. It finds both partition tables
and don't know which one to choise. If we run gdisk - it will display
interactive question which one to use.
I think it is a bug (or extra safety) in gdisk code because by standard (
https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows...)
Protective MBR should be present for GPT partition to be valid. Or it does
make sense for extra sefity if for the reason Protective MBR was not
created or accedently erased.
However other tools (gparted for example or Windows itself) will treat this
situation as a disk with MBR partition.
This way or other but it might create real troubles for libguestfs and
related tools like guestfish.
It might be rare but real use-case when Windows user accedently creates GPT
partition table and converts it immediately to MBR. I just fell into this
trap myself.
--
Mykola Ivanets
6 years, 6 months
[PATCH v5 0/3] libguestfs: guestfs_list_filesystems: skip block devices which cannot hold file system
by Mykola Ivanets
This patch series:
1. Addresses comments from last review:
part_get_mbr_part_type doesn't break original implementation in C.
2. Rebased on top of master and little bit refactored for readability.
Mykola Ivanets (1):
tests: md: Test guestfish list-filesystems command skips partitioned
md devices.
Nikolay Ivanets (2):
daemon: Reimplement 'part_get_mbr_part_type' API in OCaml.
daemon: list-filesystems: Don't list partitions which cannot hold file
system.
daemon/listfs.ml | 130 +++++++++++++++---------
daemon/parted.c | 106 -------------------
daemon/parted.ml | 13 +++
generator/actions_core.ml | 1 +
tests/md/Makefile.am | 3 +-
tests/md/test-partitioned-md-devices.sh | 79 ++++++++++++++
6 files changed, 176 insertions(+), 156 deletions(-)
create mode 100755 tests/md/test-partitioned-md-devices.sh
--
2.17.0
6 years, 6 months
