Skip to content

Commit

Permalink
lib: Avoid calling setenv between fork and exec
Browse files Browse the repository at this point in the history
setenv can call malloc and is not safe to call here.  Glibc is usually
tolerant of this and we haven't had problems before, but if you use
GLIBC_TUNABLES glibc.malloc.check=1 (or any alternate malloc / libc
which serializes) then you would see hangs if starting multiple
libguestfs handles from different threads at the same time.

This commit also updates the common submodule to pick up:

  commit 3c64bcdeaf684f05f46f3928b55aadafdfe72720
  Author: Richard W.M. Jones <rjones@redhat.com>
  Date:   Fri Oct 14 11:07:21 2022 +0100

    utils: Add function for copying the environment and adding new entries

    libguestfs is currently calling setenv at an unsafe location between
    fork and exec.  To fix this we need a way to copy and modify the
    environment before fork and then we can pass the modified environ to
    execve-like functions.  nbdkit already does the same so use that code.

    This function is copied and adapted from here under a compatible license:
    https://gitlab.com/nbdkit/nbdkit/-/blob/master/common/utils/environ.c

Thanks: Siddhesh Poyarekar
  • Loading branch information
rwmjones committed Oct 14, 2022
1 parent 4004e8e commit e1c9bbb
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 12 deletions.
2 changes: 1 addition & 1 deletion common
28 changes: 21 additions & 7 deletions lib/command.c
Original file line number Diff line number Diff line change
Expand Up @@ -452,13 +452,15 @@ debug_command (struct command *cmd)
}
}

static void run_child (struct command *cmd) __attribute__((noreturn));
static void run_child (struct command *cmd,
char **env) __attribute__((noreturn));

static int
run_command (struct command *cmd)
{
int errorfd[2] = { -1, -1 };
int outfd[2] = { -1, -1 };
CLEANUP_FREE_STRING_LIST char **env = NULL;

/* Set up a pipe to capture command output and send it to the error log. */
if (cmd->capture_errors) {
Expand All @@ -476,6 +478,10 @@ run_command (struct command *cmd)
}
}

env = guestfs_int_copy_environ (environ, "LC_ALL", "C", NULL);
if (env == NULL)
goto error;

cmd->pid = fork ();
if (cmd->pid == -1) {
perrorf (cmd->g, "fork");
Expand Down Expand Up @@ -519,7 +525,7 @@ run_command (struct command *cmd)
if (cmd->stderr_to_stdout)
dup2 (1, 2);

run_child (cmd);
run_child (cmd, env);
/*NOTREACHED*/

error:
Expand All @@ -536,7 +542,7 @@ run_command (struct command *cmd)
}

static void
run_child (struct command *cmd)
run_child (struct command *cmd, char **env)
{
struct sigaction sa;
int i, err, fd, max_fd, r;
Expand Down Expand Up @@ -571,9 +577,6 @@ run_child (struct command *cmd)
close (fd);
}

/* Clean up the environment. */
setenv ("LC_ALL", "C", 1);

/* Set the umask for all subcommands to something sensible (RHBZ#610880). */
umask (022);

Expand Down Expand Up @@ -610,6 +613,12 @@ run_child (struct command *cmd)
* tests/regressions/test-big-heap.c
*/

/* Note the assignment of environ avoids using execvpe which is a
* GNU extension. See also:
* https://github.com/libguestfs/libnbd/commit/dc64ac5cdd0bc80ca4e18935ad0e8801d11a8644
*/
environ = env;

/* Run the command. */
switch (cmd->style) {
case COMMAND_STYLE_EXECV:
Expand Down Expand Up @@ -792,6 +801,7 @@ guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode)
int errfd = -1;
int r_mode;
int ret;
CLEANUP_FREE_STRING_LIST char **env = NULL;

finish_command (cmd);

Expand Down Expand Up @@ -826,6 +836,10 @@ guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode)
goto error;
}

env = guestfs_int_copy_environ (environ, "LC_ALL", "C", NULL);
if (env == NULL)
goto error;

cmd->pid = fork ();
if (cmd->pid == -1) {
perrorf (cmd->g, "fork");
Expand Down Expand Up @@ -864,7 +878,7 @@ guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode)
close (fd[0]);
}

run_child (cmd);
run_child (cmd, env);
/*NOTREACHED*/

error:
Expand Down
15 changes: 11 additions & 4 deletions lib/launch-direct.c
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
const char *cpu_model;
CLEANUP_FREE char *append = NULL;
CLEANUP_FREE_STRING_LIST char **argv = NULL;
CLEANUP_FREE_STRING_LIST char **env = NULL;

if (!g->nr_drives) {
error (g, _("you must call guestfs_add_drive before guestfs_launch"));
Expand Down Expand Up @@ -689,6 +690,15 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
/* Get the argv list from the command line. */
argv = qemuopts_to_argv (qopts);

/* Create the environ for the child process. */
env = guestfs_int_copy_environ (environ,
"LC_ALL", "C",
/* Prevents qemu opening /dev/dsp */
"QEMU_AUDIO_DRV", "none",
NULL);
if (env == NULL)
goto cleanup0;

r = fork ();
if (r == -1) {
perrorf (g, "fork");
Expand Down Expand Up @@ -753,10 +763,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
if (g->pgroup)
setpgid (0, 0);

setenv ("LC_ALL", "C", 1);
setenv ("QEMU_AUDIO_DRV", "none", 1); /* Prevents qemu opening /dev/dsp */

execv (g->hv, argv); /* Run qemu. */
execve (g->hv, argv, env); /* Run qemu. */
perror (g->hv);
_exit (EXIT_FAILURE);
}
Expand Down

0 comments on commit e1c9bbb

Please sign in to comment.