From a35d8ff76984fe9b5342be874909085b2d77a9b9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 11 Oct 2022 08:35:59 +0200 Subject: [PATCH 1/3] cli: don't call setenv() after fork setenv() cannot be called after fork, because it might allocate memory, which can deadlock. Instead, prepare the environment and use execvpe(). `man 2 fork` says: After a fork() in a multithreaded program, the child can safely call only async-signal-safe functions (see signal-safety(7)) until such time as it calls execve(2). This means, we are quite strongly limited what can be done in the child process, before exec. setenv() is not listed as async-signal-safe, obviously because it allocates memory, and malloc() isn't async-signal-safe either. See also glib's documentation of GSpawnChildSetupFunc ([1]) about what can be done in the child process. [1] https://gitlab.gnome.org/GNOME/glib/-/blob/08cb200aec399b38f0cd825340a71fe6caadfb1b/glib/gspawn.h#L124 --- src/nmcli/utils.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/nmcli/utils.c b/src/nmcli/utils.c index 2342449e38..07b778ff3f 100644 --- a/src/nmcli/utils.c +++ b/src/nmcli/utils.c @@ -1447,11 +1447,12 @@ pager_fallback(void) pid_t nmc_terminal_spawn_pager(const NmcConfig *nmc_config) { - const char *pager = getenv("PAGER"); - pid_t pager_pid; - pid_t parent_pid; - int fd[2]; - int errsv; + const char *pager = getenv("PAGER"); + pid_t pager_pid; + pid_t parent_pid; + int fd[2]; + int errsv; + gs_strfreev char **ev = NULL; if (nmc_config->in_editor || nmc_config->print_output == NMC_PRINT_TERSE || !nmc_config->use_colors || g_strcmp0(pager, "") == 0 || getauxval(AT_SECURE)) @@ -1465,6 +1466,10 @@ nmc_terminal_spawn_pager(const NmcConfig *nmc_config) parent_pid = getpid(); + ev = g_get_environ(); + ev = g_environ_setenv(ev, "LESS", "FRSXMK", TRUE); + ev = g_environ_setenv(ev, "LESSCHARSET", "utf-8", TRUE); + pager_pid = fork(); if (pager_pid == -1) { errsv = errno; @@ -1480,9 +1485,6 @@ nmc_terminal_spawn_pager(const NmcConfig *nmc_config) nm_close(fd[0]); nm_close(fd[1]); - setenv("LESS", "FRSXMK", 1); - setenv("LESSCHARSET", "utf-8", 1); - /* Make sure the pager goes away when the parent dies */ if (prctl(PR_SET_PDEATHSIG, SIGTERM) < 0) _exit(EXIT_FAILURE); @@ -1493,8 +1495,8 @@ nmc_terminal_spawn_pager(const NmcConfig *nmc_config) _exit(EXIT_SUCCESS); if (pager) { - execlp(pager, pager, NULL); - execl("/bin/sh", "sh", "-c", pager, NULL); + execvpe(pager, (char **) NM_MAKE_STRV(pager), ev); + execvpe("/bin/sh", (char **) NM_MAKE_STRV("sh", "-c", pager), ev); } /* Debian's alternatives command for pagers is @@ -1503,10 +1505,10 @@ nmc_terminal_spawn_pager(const NmcConfig *nmc_config) * shell script that implements a logic that * is similar to this one anyway, but is * Debian-specific. */ - execlp("pager", "pager", NULL); + execvpe("pager", (char **) NM_MAKE_STRV("pager"), ev); - execlp("less", "less", NULL); - execlp("more", "more", NULL); + execvpe("less", (char **) NM_MAKE_STRV("less"), ev); + execvpe("more", (char **) NM_MAKE_STRV("more"), ev); pager_fallback(); /* not reached */ From e843a7caa220ad201bceddd9990918a004f30b51 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 11 Oct 2022 08:36:21 +0200 Subject: [PATCH 2/3] cli: don't use unsafe functions in pager_fallback() The pager_fallback() runs in the forked child process. As such, it can only use functions from `man signal-safety` or that are explicitly allowed. We are mostly good, but g_printerr() is not allowed. It can deadlock. Just avoid it. It's not very to print those error messages anyway. --- src/nmcli/utils.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/nmcli/utils.c b/src/nmcli/utils.c index 07b778ff3f..503a20a43d 100644 --- a/src/nmcli/utils.c +++ b/src/nmcli/utils.c @@ -1423,20 +1423,18 @@ pager_fallback(void) { char buf[64]; int rb; - int errsv; + + /* We are still in the child process (after fork() and before exec()). + * We must only used functions listed in `man signal-safety`. */ do { rb = read(STDIN_FILENO, buf, sizeof(buf)); if (rb == -1) { - errsv = errno; - if (errsv == EINTR) + if (errno == EINTR) continue; - g_printerr(_("Error reading nmcli output: %s\n"), nm_strerror_native(errsv)); _exit(EXIT_FAILURE); } if (write(STDOUT_FILENO, buf, rb) == -1) { - errsv = errno; - g_printerr(_("Error writing nmcli output: %s\n"), nm_strerror_native(errsv)); _exit(EXIT_FAILURE); } } while (rb > 0); From 619032c6d04c2691b3312cfffec1eafbc94685c9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 11 Oct 2022 08:56:06 +0200 Subject: [PATCH 3/3] cli: increase buffer in pager_fallback() The stack is large enough. Let's use a larger buffer. --- src/nmcli/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nmcli/utils.c b/src/nmcli/utils.c index 503a20a43d..ebe234706a 100644 --- a/src/nmcli/utils.c +++ b/src/nmcli/utils.c @@ -1421,7 +1421,7 @@ nmc_print(const NmcConfig *nmc_config, static void pager_fallback(void) { - char buf[64]; + char buf[1024]; int rb; /* We are still in the child process (after fork() and before exec()).