From 557fb493ca06e9cdcd104a48fd47f754a4ab5a8a Mon Sep 17 00:00:00 2001 From: George T Kramer Date: Tue, 26 Sep 2017 15:48:59 -0700 Subject: [PATCH] Make full file creation for directories thread-safe When running many instances of swupd_make_fullfiles in parallel, the read end of the pipe between the tar's for creating the full file of a directory becomes reused. The observed behavior of this is swupd_make_fullfiles hangs indefinitely with the expected tar reader missing. The corresponding tar writer is not killed with SIGPIPE because there is at least one reader still for the pipe, swupd_make_fullfiles. First forking from swupd_make_fullfiles, creating the pipe, and then fork-and-exec'ing for each tar ensures that pipe and file descriptor management is contained for the directory rename in question and cannot be reused by other directory renames. Signed-off-by: George T Kramer --- include/swupd.h | 13 ++- src/fullfiles.c | 11 +-- src/helpers.c | 215 +++++++++++++++++++++++------------------------- 3 files changed, 114 insertions(+), 125 deletions(-) diff --git a/include/swupd.h b/include/swupd.h index 8a0a5de..16fa44d 100644 --- a/include/swupd.h +++ b/include/swupd.h @@ -262,10 +262,17 @@ extern FILE *fopen_exclusive(const char *filename); /* no mode, opens for write extern void dump_file_info(struct file *file); extern void string_or_die(char **strp, const char *fmt, ...); extern void print_elapsed_time(const char *step, struct timeval *previous_time, struct timeval *current_time); +extern int system_argv_pipe(char *const lhscmd[], char *const rhscmd[]); +extern int system_argv_pipe_fd(int lnewstdinfd, int lnewstderrfd, char *const lhscmd[], + int rnewstdoutfd, int rnewstderrfd, char *const rhscmd[]); +extern void pipe_monitor(int lnewstdinfd, int lnewstderrfd, char *const lhscmd[], + int rnewstdoutfd, int rnewstderrfd, char *const rhscmd[]); extern int system_argv(char *const argv[]); -extern int system_argv_fd(char *const argv[], int newstdin, int newstdout, int newstderr); -extern int system_argv_pipe(char *const argvp1[], int stdinp1, int stderrp1, - char *const argvp2[], int stdoutp2, int stderrp2); +extern int system_argv_fd(int newstdinfd, int newstdoutfd, int newstderrfd, char *const cmd[]); +extern pid_t system_argv_fd_nowait(int newstdinfd, int newstdoutfd, int newstderrfd, int closefd, char *const cmd[]); +extern void exec_cmd_fd(int newstdinfd, int newstdoutfd, int newstderrfd, int closefd, char *const cmd[]); +extern void move_fd(int oldfd, int newfd); +extern int wait_process_terminate(pid_t pid); extern int num_threads(float scaling); extern bool file_is_debuginfo(const char *path); diff --git a/src/fullfiles.c b/src/fullfiles.c index 5db3c6f..1c062f9 100644 --- a/src/fullfiles.c +++ b/src/fullfiles.c @@ -50,7 +50,6 @@ static void create_fullfile(struct file *file) struct stat sbuf; char *empty, *indir, *outdir; char *param1, *param2; - int stderrfd; if (file->is_deleted) { return; /* file got deleted -> by definition we cannot tar it up */ @@ -98,17 +97,13 @@ static void create_fullfile(struct file *file) char *const tarcfcmd[] = { TAR_COMMAND, "-C", dir, TAR_PERM_ATTR_ARGS_STRLIST, "-cf", "-", param1, param2, NULL }; char *const tarxfcmd[] = { TAR_COMMAND, "-C", rename_tmpdir, TAR_PERM_ATTR_ARGS_STRLIST, "-xf", "-", NULL }; - stderrfd = open("/dev/null", O_WRONLY); - if (stderrfd == -1) { - LOG(NULL, "Failed to open /dev/null", ""); - assert(0); - } - if (system_argv_pipe(tarcfcmd, -1, stderrfd, tarxfcmd, -1, stderrfd) != 0) { + int tarcmdresult = system_argv_pipe(tarcfcmd, tarxfcmd); + if (tarcmdresult != 0) { + LOG(NULL, "Tar command for copying directory full file failed with code %d", tarcmdresult); assert(0); } free(param1); free(param2); - close(stderrfd); string_or_die(&rename_source, "%s/%s", rename_tmpdir, base); string_or_die(&rename_target, "%s/%s", rename_tmpdir, file->hash); diff --git a/src/helpers.c b/src/helpers.c index 8e7303f..353ff90 100644 --- a/src/helpers.c +++ b/src/helpers.c @@ -149,130 +149,117 @@ void concat_str_array(char **output, char *const argv[]) } } +int system_argv_pipe(char *const lhscmd[], char *const rhscmd[]) +{ + return system_argv_pipe_fd(-1, -1, lhscmd, -1, -1, rhscmd); +} + +int system_argv_pipe_fd(int lnewstdinfd, int lnewstderrfd, char *const lhscmd[], + int rnewstdoutfd, int rnewstderrfd, char *const rhscmd[]) +{ + pid_t monitorpid = fork(); + if (monitorpid == -1) { + LOG(NULL, "Failed to create child process to monitor pipe between", "command %s and command %s", lhscmd[0], rhscmd[0]); + return -1; + } else if (monitorpid == 0) { + pipe_monitor(lnewstdinfd, lnewstderrfd, lhscmd, rnewstdoutfd, rnewstderrfd, rhscmd); + } + return wait_process_terminate(monitorpid); +} + +void pipe_monitor(int lnewstdinfd, int lnewstderrfd, char *const lhscmd[], + int rnewstdoutfd, int rnewstderrfd, char *const rhscmd[]) +{ + int pipefd[2]; + if (pipe(pipefd) == -1) { + LOG(NULL, "Failed to create a pipe between", "command %s and command %s", lhscmd[0], rhscmd[0]); + assert(0); + } + + pid_t lhspid = system_argv_fd_nowait(lnewstdinfd, pipefd[1], lnewstderrfd, pipefd[0], lhscmd); + pid_t rhspid = system_argv_fd_nowait(pipefd[0], rnewstdoutfd, rnewstderrfd, pipefd[1], rhscmd); + + if (close(pipefd[1]) == -1) { + LOG(NULL, "Could not close write end of pipe file descriptor", "%d", pipefd[1]); + assert(0); + } + if (close(pipefd[0]) == -1) { + LOG(NULL, "Could not close read end of pipe file descriptor", "%d", pipefd[0]); + assert(0); + } + + int lhsresult = wait_process_terminate(lhspid); + int rhsresult = wait_process_terminate(rhspid); + exit(rhsresult != EXIT_SUCCESS ? rhsresult : lhsresult); +} + int system_argv(char *const argv[]) { - int child_exit_status; - pid_t pid; - int status = -1; - - pid = fork(); - - if (pid == 0) { /* child */ - execvp(*argv, argv); - LOG(NULL, "This line must not be reached", ""); - assert(0); - } else if (pid < 0) { - LOG(NULL, "Failed to fork a child process", ""); - assert(0); - } else { - pid_t ws = waitpid(pid, &child_exit_status, 0); - - if (ws == -1) { - LOG(NULL, "Failed to wait for child process", ""); - assert(0); - } - - if (WIFEXITED(child_exit_status)) { - status = WEXITSTATUS(child_exit_status); - } else { - LOG(NULL, "Child process didn't exit", ""); - assert(0); - } - - if (status != 0) { - char *cmdline = NULL; - - concat_str_array(&cmdline, argv); - LOG(NULL, "Failed to run command:", "%s", cmdline); - free(cmdline); - } - } - - return status; + return system_argv_fd(-1, -1, -1, argv); } -int system_argv_fd(char *const argv[], int newstdin, int newstdout, int newstderr) +int system_argv_fd(int newstdinfd, int newstdoutfd, int newstderrfd, char *const cmd[]) { - int child_exit_status; - pid_t pid; - int status = -1; - - pid = fork(); - - if (pid == 0) { /* child */ - if (newstdin >= 0) { - if (dup2(newstdin, STDIN_FILENO) == -1) { - LOG(NULL, "Could not redirect stdin", ""); - assert(0); - } - close(newstdin); - } - if (newstdout >= 0) { - if (dup2(newstdout, STDOUT_FILENO) == -1) { - LOG(NULL, "Could not redirect stdout", ""); - assert(0); - } - close(newstdout); - } - if (newstderr >= 0) { - if (dup2(newstderr, STDERR_FILENO) == -1) { - LOG(NULL, "Could not redirect stderr", ""); - assert(0); - } - close(newstderr); - } - - execvp(*argv, argv); - LOG(NULL, "This line must not be reached", ""); - assert(0); - } else if (pid < 0) { - LOG(NULL, "Failed to fork a child process", ""); - assert(0); - } else { - pid_t ws = waitpid(pid, &child_exit_status, 0); - - if (ws == -1) { - LOG(NULL, "Failed to wait for child process", ""); - assert(0); - } - - if (WIFEXITED(child_exit_status)) { - status = WEXITSTATUS(child_exit_status); - } else { - LOG(NULL, "Child process didn't exit", ""); - assert(0); - } - - if (status != 0) { - char *cmdline = NULL; - - concat_str_array(&cmdline, argv); - LOG(NULL, "Failed to run command:", "%s", cmdline); - free(cmdline); - } - } - - return status; + pid_t cmdpid = system_argv_fd_nowait(newstdinfd, newstdoutfd, newstderrfd, -1, cmd); + return wait_process_terminate(cmdpid); } -int system_argv_pipe(char *const argvp1[], int stdinp1, int stderrp1, - char *const argvp2[], int stdoutp2, int stderrp2) +pid_t system_argv_fd_nowait(int newstdinfd, int newstdoutfd, int newstderrfd, int closefd, char *const cmd[]) { - int statusp2; - int pipefd[2]; - - if (pipe(pipefd)) { - LOG(NULL, "Failed to create a pipe", ""); - return -1; + pid_t cmdpid = fork(); + if (cmdpid == -1) { + LOG(NULL, "Failed to fork to execute command", "%s", cmd[0]); + assert(0); + } else if (cmdpid == 0) { + exec_cmd_fd(newstdinfd, newstdoutfd, newstderrfd, closefd, cmd); } - system_argv_fd(argvp1, stdinp1, pipefd[1], stderrp1); - close(pipefd[1]); - statusp2 = system_argv_fd(argvp2, pipefd[0], stdoutp2, stderrp2); - close(pipefd[0]); + return cmdpid; +} - /* Returns the status of the failed process if any - If both processes failed returns the status of first one */ - return statusp2; +void exec_cmd_fd(int newstdinfd, int newstdoutfd, int newstderrfd, int closefd, char *const cmd[]) +{ + move_fd(newstdinfd, STDIN_FILENO); + move_fd(newstdoutfd, STDOUT_FILENO); + move_fd(newstderrfd, STDERR_FILENO); + if (closefd >= 0 && close(closefd) == -1) { + LOG(NULL, "Could not close file descriptor", "%d", closefd); + assert(0); + } + execvp(*cmd, cmd); + LOG(NULL, "Command", "%s failed", cmd[0]); + assert(0); +} + +void move_fd(int oldfd, int newfd) +{ + if (oldfd < 0 || newfd < 0 || oldfd == newfd) { + return; + } + if (dup2(oldfd, newfd) == -1) { + LOG(NULL, "Could not create duplicate file descriptor", "%d from %d", newfd, oldfd); + assert(0); + } + if (close(oldfd) == -1) { + LOG(NULL, "Could not close file descriptor", "%d", oldfd); + assert(0); + } +} + +int wait_process_terminate(pid_t pid) +{ + int status; + do { + if (waitpid(pid, &status, 0) == -1) { + LOG(NULL, "Failed to wait for PID", "%d", pid); + return -1; + } + } while (!WIFEXITED(status) && !WIFSIGNALED(status)); + // Exit statuses fall in the range of [0, 255]. Make signal statuses fall in a non-overlapping range starting with 256. + if (WIFEXITED(status)) { + return WEXITSTATUS(status); + } else { + return 256 + WTERMSIG(status); + } } void check_root(void)