From 1b44f81e1e3cc09b4545b8c69918fe653ffb167b Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Tue, 13 Jul 2021 12:29:06 -0400 Subject: [PATCH 1/8] cbits/fork_exec: Ensure that value is returned from all branches --- cbits/posix/fork_exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cbits/posix/fork_exec.c b/cbits/posix/fork_exec.c index 282ee9c2..a6c52944 100644 --- a/cbits/posix/fork_exec.c +++ b/cbits/posix/fork_exec.c @@ -84,7 +84,7 @@ setup_std_handle_fork(int fd, if (close(b->use_pipe.parent_end) == -1) { child_failed(pipe, "close(parent_end)"); } - break; + return 0; } } From 14e6a2f6b063d037fceb542de49be30c299a8a94 Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Tue, 13 Jul 2021 12:29:27 -0400 Subject: [PATCH 2/8] cbits: Silence missing switch case warnings This code should be dead but for apparently some gcc versions cannot see this. --- cbits/posix/fork_exec.c | 5 +++++ cbits/posix/posix_spawn.c | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/cbits/posix/fork_exec.c b/cbits/posix/fork_exec.c index a6c52944..dee64e13 100644 --- a/cbits/posix/fork_exec.c +++ b/cbits/posix/fork_exec.c @@ -85,6 +85,11 @@ setup_std_handle_fork(int fd, child_failed(pipe, "close(parent_end)"); } return 0; + + default: + // N.B. this should be unreachable but some compilers apparently can't + // see this. + child_failed(pipe, "setup_std_handle_fork(invalid behavior)"); } } diff --git a/cbits/posix/posix_spawn.c b/cbits/posix/posix_spawn.c index 9683081e..44f58f76 100644 --- a/cbits/posix/posix_spawn.c +++ b/cbits/posix/posix_spawn.c @@ -64,6 +64,13 @@ setup_std_handle_spawn (int fd, return -1; } return 0; + + default: + // N.B. this should be unreachable + // but some compilers apparently can't + // see this. + *failed_doing = "posix_spawn_file_actions_addclose(invalid behavior)"; + return -1; } } From 305f37483af7c3b98376d36c0302a9ec747b0c25 Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Tue, 13 Jul 2021 12:29:55 -0400 Subject: [PATCH 3/8] cbits/fork_exec: Add parentheses for clarity --- cbits/posix/fork_exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cbits/posix/fork_exec.c b/cbits/posix/fork_exec.c index dee64e13..9f749d79 100644 --- a/cbits/posix/fork_exec.c +++ b/cbits/posix/fork_exec.c @@ -224,7 +224,7 @@ do_spawn_fork (char *const args[], /* Reset the SIGINT/SIGQUIT signal handlers in the child, if requested */ - if (flags & RESET_INT_QUIT_HANDLERS != 0) { + if ((flags & RESET_INT_QUIT_HANDLERS) != 0) { struct sigaction dfl; (void)sigemptyset(&dfl.sa_mask); dfl.sa_flags = 0; From 6a852c6b5b3e3f4fdc8023aef6dc32d4a8166b44 Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Tue, 13 Jul 2021 12:30:14 -0400 Subject: [PATCH 4/8] configure: Ensure that is included during posix_spawn checks --- configure.ac | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 115d4476..56f10c45 100644 --- a/configure.ac +++ b/configure.ac @@ -18,9 +18,18 @@ AC_CHECK_FUNCS([execvpe]) # posix_spawn checks AC_CHECK_HEADERS([spawn.h]) -AC_CHECK_FUNCS([posix_spawnp posix_spawn_file_actions_addchdir]) -AC_CHECK_DECLS([POSIX_SPAWN_SETSID, POSIX_SPAWN_SETSID_NP]) -AC_CHECK_DECLS([POSIX_SPAWN_SETPGROUP]) +AC_CHECK_FUNCS([posix_spawnp posix_spawn_file_actions_addchdir],[],[],[ + #define _GNU_SOURCE + #include +]) +AC_CHECK_DECLS([POSIX_SPAWN_SETSID, POSIX_SPAWN_SETSID_NP],[],[],[ + #define _GNU_SOURCE + #include +]) +AC_CHECK_DECLS([POSIX_SPAWN_SETPGROUP],[],[],[ + #define _GNU_SOURCE + #include +]) FP_CHECK_CONSTS([SIG_DFL SIG_IGN]) From c3a5bd1dd8872c3c7c74137ef4994e74e3a09d81 Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Tue, 13 Jul 2021 12:31:37 -0400 Subject: [PATCH 5/8] cbits/fork_exec: Drop redundant return --- cbits/posix/fork_exec.c | 1 - 1 file changed, 1 deletion(-) diff --git a/cbits/posix/fork_exec.c b/cbits/posix/fork_exec.c index 9f749d79..174f2ebf 100644 --- a/cbits/posix/fork_exec.c +++ b/cbits/posix/fork_exec.c @@ -62,7 +62,6 @@ setup_std_handle_fork(int fd, case STD_HANDLE_CLOSE: if (close(fd) == -1) { child_failed(pipe, "close"); - return -1; } return 0; From a29f731968e7e9d37c27d30ef8de94c81e8b28c0 Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Tue, 13 Jul 2021 12:48:53 -0400 Subject: [PATCH 6/8] cbits/posix_spawn: Don't claim to support setpgroup when we don't --- cbits/posix/posix_spawn.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cbits/posix/posix_spawn.c b/cbits/posix/posix_spawn.c index 44f58f76..7934486b 100644 --- a/cbits/posix/posix_spawn.c +++ b/cbits/posix/posix_spawn.c @@ -150,11 +150,13 @@ do_spawn_posix (char *const args[], #endif } -#if defined(HAVE_POSIX_SPAWN_SETPGROUP) if ((flags & RUN_PROCESS_IN_NEW_GROUP) != 0) { +#if defined(HAVE_POSIX_SPAWN_SETPGROUP) spawn_flags |= POSIX_SPAWN_SETPGROUP; - } +#else + goto not_supported; #endif + } if (setup_std_handle_spawn(STDIN_FILENO, stdInHdl, &fa, failed_doing) != 0) { goto fail; From 0ef66f1faa51298d2361fd1cc1b4771bc80c5fcc Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Tue, 13 Jul 2021 13:04:22 -0400 Subject: [PATCH 7/8] test: Normalise away spurious error output differences from process010 --- tests/all.T | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/all.T b/tests/all.T index 2339d8c4..9b02617d 100644 --- a/tests/all.T +++ b/tests/all.T @@ -38,7 +38,10 @@ test('T3994', [only_ways(['threaded1','threaded2']), test('T4889', normal, compile_and_run, ['']) test('process009', when(opsys('mingw32'), skip), compile_and_run, ['']) -test('process010', normalise_exec, compile_and_run, ['']) +test('process010', [ + normalise_fun(lambda s: s.replace('illegal operation (Inappropriate ioctl for device)', 'does not exist (No such file or directory)')), + normalise_exec +], compile_and_run, ['']) test('process011', when(opsys('mingw32'), skip), compile_and_run, ['']) test('T8343', normal, compile_and_run, ['']) From 278746ad6ea2df2a8a3b80673cf5400c4389ce2e Mon Sep 17 00:00:00 2001 From: Ben Gamari Date: Wed, 14 Jul 2021 10:02:08 -0400 Subject: [PATCH 8/8] cbits/find_executable: Workaround erroneous uninitialized use error It appears that some older versions of gcc (e.g. 6.3.0, used in Debian 9) cannot see that path = strtok_r(...); initialises `path`. Surgically disable this warning. --- cbits/posix/find_executable.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cbits/posix/find_executable.c b/cbits/posix/find_executable.c index d12ebca3..9f9dc6cf 100644 --- a/cbits/posix/find_executable.c +++ b/cbits/posix/find_executable.c @@ -11,7 +11,7 @@ #include "common.h" -// the below is only necessary when we don't have execvpe. +// the below is only necessary when we need to emulate execvpe. #if !defined(HAVE_execvpe) /* Return true if the given file exists and is an executable. */ @@ -28,7 +28,16 @@ static char *find_in_search_path(char *search_path, const char *filename) { char *tokbuf; char *path = strtok_r(search_path, ":", &tokbuf); while (path != NULL) { + // N.B. gcc 6.3.0, used by Debian 9, inexplicably warns that `path` + // may not be initialised with -Wall. Silence this warning. See #210. +#if defined(__GNUC__) && __GNUC__ == 6 && __GNUC_MINOR__ == 3 +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif const int tmp_len = filename_len + 1 + strlen(path) + 1; +#if defined(__GNUC__) && __GNUC__ == 6 && __GNUC_MINOR__ == 3 +#pragma GCC diagnostic pop +#endif char *tmp = malloc(tmp_len); snprintf(tmp, tmp_len, "%s/%s", path, filename); if (is_executable(tmp)) {