[PATCH 1/2] lib: info: Move common code for setting child rlimits.
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
participants (1)
-
Richard W.M. Jones