Libguestfs - Oct 2015 - [PATCH 2/2] lib: Add comment and regression test for case where main process has large heap.

This is almost just refactoring, but I also set the memory
limit to really 1 GB, and not 1×10⁹.
---
 src/info.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/src/info.c b/src/info.c
index d7f45f0..616ef50 100644
--- a/src/info.c
+++ b/src/info.c
@@ -56,6 +56,7 @@ static yajl_val get_json_output (guestfs_h *g, const char
*filename);
 static char *old_parser_disk_format (guestfs_h *g, const char *filename);
 static int64_t old_parser_disk_virtual_size (guestfs_h *g, const char
*filename);
 static int old_parser_disk_has_backing_file (guestfs_h *g, const char
*filename);
+static void set_child_rlimits (struct command *);
 
 char *
 guestfs_impl_disk_format (guestfs_h *g, const char *filename)
@@ -276,12 +277,7 @@ get_json_output (guestfs_h *g, const char *filename)
   guestfs_int_cmd_add_arg (cmd, fdpath);
   guestfs_int_cmd_set_stdout_callback (cmd, parse_json, &tree,
                                        CMD_STDOUT_FLAG_WHOLE_BUFFER);
-#ifdef RLIMIT_AS
-  guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_AS, 1000000000 /* 1GB */);
-#endif
-#ifdef RLIMIT_CPU
-  guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_CPU, 10 /* seconds */);
-#endif
+  set_child_rlimits (cmd);
   r = guestfs_int_cmd_run (cmd);
   close (fd);
   if (r == -1)
@@ -560,12 +556,7 @@ old_parser_run_qemu_img_info (guestfs_h *g, const char
*filename,
   guestfs_int_cmd_add_arg (cmd, "info");
   guestfs_int_cmd_add_arg (cmd, safe_filename);
   guestfs_int_cmd_set_stdout_callback (cmd, fn, data, 0);
-#ifdef RLIMIT_AS
-  guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_AS, 1000000000 /* 1GB */);
-#endif
-#ifdef RLIMIT_CPU
-  guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_CPU, 10 /* seconds */);
-#endif
+  set_child_rlimits (cmd);
   r = guestfs_int_cmd_run (cmd);
   if (r == -1)
     return -1;
@@ -576,3 +567,15 @@ old_parser_run_qemu_img_info (guestfs_h *g, const char
*filename,
 
   return 0;
 }
+
+static void
+set_child_rlimits (struct command *cmd)
+{
+#ifdef RLIMIT_AS
+  const long one_gb = 1024L * 1024 * 1024;
+  guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_AS, one_gb);
+#endif
+#ifdef RLIMIT_CPU
+  guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_CPU, 10 /* seconds */);
+#endif
+}
-- 
2.5.0

Thanks: Dan Berrangé for identifying the problem.
---
 .gitignore                        |  1 +
 src/command.c                     | 11 +++++++
 tests/regressions/Makefile.am     | 13 +++++++-
 tests/regressions/test-big-heap.c | 69 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 tests/regressions/test-big-heap.c

diff --git a/.gitignore b/.gitignore
index cb5bc49..2f571a3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -582,6 +582,7 @@ Makefile.in
 /tests/regressions/rhbz914931
 /tests/regressions/rhbz1044014.out
 /tests/regressions/rhbz1055452
+/tests/regressions/test-big-heap
 /tests/rsync/rsyncd.pid
 /tests/syslinux/extlinux-guest.img
 /tests/syslinux/syslinux-guest.img
diff --git a/src/command.c b/src/command.c
index 0547111..e9f357a 100644
--- a/src/command.c
+++ b/src/command.c
@@ -558,6 +558,17 @@ run_child (struct command *cmd)
   }
 #endif /* HAVE_SETRLIMIT */
 
+  /* NB: If the main process (which we have forked a copy of) uses
+   * more heap than the RLIMIT_AS we set above, then any call to
+   * malloc or any extension of the stack will fail with ENOMEM or
+   * SIGSEGV respectively.  Luckily we only use RLIMIT_AS followed by
+   * execvp below, so we get away with it, but adding any code here
+   * could cause a failure.
+   *
+   * There is a regression test for this.  See:
+   * tests/regressions/test-big-heap.c
+   */
+
   /* Run the command. */
   switch (cmd->style) {
   case COMMAND_STYLE_EXECV:
diff --git a/tests/regressions/Makefile.am b/tests/regressions/Makefile.am
index 74f23dd..c4f60ae 100644
--- a/tests/regressions/Makefile.am
+++ b/tests/regressions/Makefile.am
@@ -71,6 +71,7 @@ TESTS = \
 	rhbz1091803.sh \
 	rhbz1175196.sh \
 	rhbz1232192.sh \
+	test-big-heap \
 	test-noexec-stack.pl
 
 if HAVE_LIBVIRT
@@ -89,7 +90,8 @@ check_PROGRAMS = \
 	rhbz501893 \
 	rhbz790721 \
 	rhbz914931 \
-	rhbz1055452
+	rhbz1055452 \
+	test-big-heap
 
 rhbz501893_SOURCES = rhbz501893.c
 rhbz501893_CPPFLAGS = \
@@ -130,5 +132,14 @@ rhbz1055452_CFLAGS = \
 rhbz1055452_LDADD = \
 	$(top_builddir)/src/libguestfs.la
 
+test_big_heap_SOURCES = test-big-heap.c
+test_big_heap_CPPFLAGS = \
+	-I$(top_srcdir)/src -I$(top_builddir)/src \
+	-DGUESTFS_PRIVATE=1
+test_big_heap_CFLAGS = \
+	$(WARN_CFLAGS) $(WERROR_CFLAGS)
+test_big_heap_LDADD = \
+	$(top_builddir)/src/libguestfs.la
+
 check-slow:
 	$(MAKE) TESTS="rhbz909624.sh" check
diff --git a/tests/regressions/test-big-heap.c
b/tests/regressions/test-big-heap.c
new file mode 100644
index 0000000..ddce270
--- /dev/null
+++ b/tests/regressions/test-big-heap.c
@@ -0,0 +1,69 @@
+/* libguestfs
+ * Copyright (C) 2015 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
+ */
+
+/* Test that allocating lots of heap in the main program doesn't cause
+ * libguestfs to fail when it runs qemu-img.  When we call qemu-img,
+ * after forking but before execing, we set RLIMIT_AS to 1 GB.  If the
+ * main program is using more than 1 GB, then any malloc or stack
+ * extension will fail.  We get away with this by calling exec
+ * immediately after setting the rlimit, but it only just works, and
+ * this test is designed to catch any regressions.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+
+#include "guestfs.h"
+#include "guestfs-internal-frontend.h"
+
+int
+main (int argc, char *argv[])
+{
+  guestfs_h *g = guestfs_create ();
+  char *mem, *fmt;
+
+  /* Make sure we're using > 1GB in the main process. */
+  mem = calloc (2 * 1024, 1024 * 1024);
+  assert (mem != NULL);
+
+  /* Do something which forks qemu-img subprocess. */
+  fmt = guestfs_disk_format (g, "/dev/null");
+  if (fmt == NULL) {
+    /* Test failed. */
+    fprintf (stderr, "%s: unexpected failure of test, see earlier
messages\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+
+  if (STRNEQ (fmt, "raw")) {
+    /* Test failed. */
+    fprintf (stderr, "%s: unexpected output: expected 'raw' actual
'%s'\n",
+             argv[0], fmt);
+    exit (EXIT_FAILURE);
+  }
+
+  /* Test successful. */
+
+  free (fmt);
+  free (mem);
+  exit (EXIT_SUCCESS);
+}
-- 
2.5.0

On Wed, Oct 14, 2015 at 05:03:28PM +0100, Richard W.M. Jones
wrote:
> +  /* Make sure we're using > 1GB in the main process. */
> +  mem = calloc (2 * 1024, 1024 * 1024);
> +  assert (mem != NULL);
This won't work on 32 bit platforms, because it's unlikely we could
allocate 2 GB of contiguous memory.  So I'm proposing to keep the test
but have it skip if the calloc() call fails.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org