diff --git a/src/env.h b/src/env.h index b5c8131c5..b7f3c2158 100644 --- a/src/env.h +++ b/src/env.h @@ -71,8 +71,8 @@ void misc_init(); class env_var_t { private: - wcstring val; bool is_missing; + wcstring val; public: static env_var_t missing_var() { diff --git a/src/exec.cpp b/src/exec.cpp index 6f4e705e1..0b18bb634 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -19,7 +19,6 @@ #include #include -#include #include #include #include @@ -38,7 +37,6 @@ #include "postfork.h" #include "proc.h" #include "reader.h" -#include "signal.h" #include "wutil.h" // IWYU pragma: keep /// File descriptor redirection error message. @@ -495,8 +493,8 @@ void exec_job(parser_t &parser, job_t *j) { // We are careful to set these to -1 when closed, so if we exit the loop abruptly, we can still // close them. bool pgrp_set = false; - // This is static since processes can block on input/output across jobs the main exec_job loop - // is only ever run in a single thread, so this is OK. + //this is static since processes can block on input/output across jobs + //the main exec_job loop is only ever run in a single thread, so this is OK static pid_t blocked_pid = -1; int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1; for (std::unique_ptr &unique_p : j->processes) { @@ -515,7 +513,7 @@ void exec_job(parser_t &parser, job_t *j) { // See if we need a pipe. const bool pipes_to_next_command = !p->is_last_in_job; - // Set to true if we end up forking for this process. + //set to true if we end up forking for this process bool child_forked = false; bool child_spawned = false; bool block_child = true; @@ -624,12 +622,12 @@ void exec_job(parser_t &parser, job_t *j) { // a pipeline. shared_ptr block_output_io_buffer; - // Used to SIGCONT the previously SIGSTOP'd process so the main loop or next command in job - // can read from its output. - auto unblock_previous = [&j]() { - // We've already called waitpid after forking the child, so we've guaranteed that - // they're already SIGSTOP'd. Otherwise we'd be risking a deadlock because we can call - // SIGCONT before they've actually stopped, and they'll remain suspended indefinitely. + //used to SIGCONT the previously SIGSTOP'd process so the main loop or next command in job can read + //from its output. + auto unblock_previous = [&j] () { + //we've already called waitpid after forking the child, so we've guaranteed that they're + //already SIGSTOP'd. Otherwise we'd be risking a deadlock because we can call SIGCONT before + //they've actually stopped, and they'll remain suspended indefinitely. if (blocked_pid != -1) { debug(2, L"Unblocking process %d.\n", blocked_pid); kill(blocked_pid, SIGCONT); @@ -655,7 +653,7 @@ void exec_job(parser_t &parser, job_t *j) { if (block_child) { kill(p->pid, SIGSTOP); } - // The parent will wake us up when we're ready to execute. + //the parent will wake us up when we're ready to execute setup_child_process(p, process_net_io_chain); child_action(); DIE("Child process returned control to do_fork lambda!"); @@ -680,9 +678,10 @@ void exec_job(parser_t &parser, job_t *j) { return true; }; - // Helper routine executed by INTERNAL_FUNCTION and INTERNAL_BLOCK_NODE to make sure an - // output buffer exists in case there is another command in the job chain that will be - // reading from this command's output. + + //helper routine executed by INTERNAL_FUNCTION and INTERNAL_BLOCK_NODE to make sure + //an output buffer exists in case there is another command in the job chain that will + //be reading from this command's output. auto verify_buffer_output = [&] () { if (!p->is_last_in_job) { // Be careful to handle failure, e.g. too many open fds. @@ -843,8 +842,8 @@ void exec_job(parser_t &parser, job_t *j) { const int fg = j->get_flag(JOB_FOREGROUND); j->set_flag(JOB_FOREGROUND, false); - // Main loop may need to perform a blocking read from previous command's output. - // Make sure read source is not blocked. + //main loop may need to perform a blocking read from previous command's output. + //make sure read source is not blocked unblock_previous(); p->status = builtin_run(parser, p->get_argv(), *builtin_io_streams); @@ -940,8 +939,8 @@ void exec_job(parser_t &parser, job_t *j) { bool must_fork = redirection_is_to_real_file(stdout_io.get()) || redirection_is_to_real_file(stderr_io.get()); if (!must_fork && p->is_last_in_job) { - // We are handling reads directly in the main loop. Make sure source is - // unblocked. Note that we may still end up forking. + //we are handling reads directly in the main loop. Make sure source is unblocked. + //Note that we may still end up forking. unblock_previous(); const bool stdout_is_to_buffer = stdout_io && stdout_io->io_mode == IO_BUFFER; const bool no_stdout_output = stdout_buffer.empty(); @@ -1111,19 +1110,18 @@ void exec_job(parser_t &parser, job_t *j) { bool child_blocked = block_child && child_forked; if (child_blocked) { - // We have to wait to ensure the child has set their progress group and is in SIGSTOP - // state otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be - // blocked indefinitely. The child is SIGSTOP'd and is guaranteed to have called - // child_set_group() at this point. but we only need to call set_child_group for the - // first process in the group. If needs_keepalive is set, this has already been called - // for the keepalive process. + //we have to wait to ensure the child has set their progress group and is in SIGSTOP state + //otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be blocked indefinitely. + //The child is SIGSTOP'd and is guaranteed to have called child_set_group() at this point. + //but we only need to call set_child_group for the first process in the group. + //If needs_keepalive is set, this has already been called for the keepalive process pid_t pid_status{}; int result; while ((result = waitpid(p->pid, &pid_status, WUNTRACED)) == -1 && errno == EINTR) { - // This could be a superfluous interrupt or Ctrl+C at the terminal In all cases, it - // is OK to retry since the forking code above is specifically designed to never, - // ever hang/block in a child process before the SIGSTOP call is reached. - ; // do nothing + //This could be a superfluous interrupt or Ctrl+C at the terminal + //In all cases, it is OK to retry since the forking code above is specifically designed + //to never, ever hang/block in a child process before the SIGSTOP call is reached. + continue; } if (result == -1) { exec_error = true; @@ -1131,26 +1129,26 @@ void exec_job(parser_t &parser, job_t *j) { wperror(L"waitpid"); break; } - // We are not unblocking the child via SIGCONT just yet to give the next process a - // chance to open the pipes and join the process group. We only unblock the last process - // in the job chain because no one awaits it. + //we are not unblocking the child via SIGCONT just yet to give the next process a chance to open + //the pipes and join the process group. We only unblock the last process in the job chain because + //no one awaits it. } - // Regardless of whether the child blocked or not: only once per job, and only once we've - // actually executed an external command. + //regardless of whether the child blocked or not: + //only once per job, and only once we've actually executed an external command if ((child_spawned || child_forked) && !pgrp_set) { - // This should be called after waitpid if child_forked and pipes_to_next_command it can - // be called at any time if child_spawned. + //this should be called after waitpid if child_forked && pipes_to_next_command + //it can be called at any time if child_spawned set_child_group(j, p->pid); - // we can't rely on p->is_first_in_job because a builtin may have been the first. + //we can't rely on p->is_first_in_job because a builtin may have been the first pgrp_set = true; } - // If the command we ran _before_ this one was SIGSTOP'd to let this one catch up, unblock - // it now. this must be after the wait_pid on the process we just started, if any. + //if the command we ran _before_ this one was SIGSTOP'd to let this one catch up, unblock it now. + //this must be after the wait_pid on the process we just started, if any. unblock_previous(); if (child_blocked) { - // Store the newly-blocked command's PID so that it can be SIGCONT'd once the next - // process in the chain is started. That may be in this job or in another job. + //store the newly-blocked command's PID so that it can be SIGCONT'd once the next process + //in the chain is started. That may be in this job or in another job. blocked_pid = p->pid; } @@ -1166,7 +1164,7 @@ void exec_job(parser_t &parser, job_t *j) { pipe_current_write = -1; } - // Unblock the last process because there's no need for it to stay SIGSTOP'd for anything. + //unblock the last process because there's no need for it to stay SIGSTOP'd for anything if (p->is_last_in_job) { unblock_previous(); } diff --git a/src/expand.h b/src/expand.h index 6643b8742..2fd0423bd 100644 --- a/src/expand.h +++ b/src/expand.h @@ -122,6 +122,8 @@ bool expand_one(wcstring &inout_str, expand_flags_t flags, parse_error_list_t *e /// Convert the variable value to a human readable form, i.e. escape things, handle arrays, etc. /// Suitable for pretty-printing. +/// +/// \param in the value to escape wcstring expand_escape_variable(const env_var_t &var); /// Perform tilde expansion and nothing else on the specified string, which is modified in place. diff --git a/src/postfork.cpp b/src/postfork.cpp index bdf788607..8b79c4f7f 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -78,9 +78,9 @@ bool child_set_group(job_t *j, process_t *p) { if (j->pgid == -2) { j->pgid = p->pid; } - // Retry on EPERM because there's no way that a child cannot join an existing progress group - // because we are SIGSTOPing the previous job in the chain. Sometimes we have to try a few - // times to get the kernel to see the new group. (Linux 4.4.0) + //retry on EPERM because there's no way that a child cannot join an existing progress group + //because we are SIGSTOPing the previous job in the chain. Sometimes we have to try a few + //times to get the kernel to see the new group. (Linux 4.4.0) int failure = setpgid(p->pid, j->pgid); while (failure == -1 && (errno == EPERM || errno == EINTR)) { debug_safe(4, "Retrying setpgid in child process"); @@ -112,7 +112,7 @@ bool child_set_group(job_t *j, process_t *p) { retval = false; } } else { - // This is probably stays unused in the child. + //this is probably stays unused in the child j->pgid = getpgrp(); } @@ -142,16 +142,16 @@ bool set_child_group(job_t *j, pid_t child_pid) { if (j->get_flag(JOB_TERMINAL) && j->get_flag(JOB_FOREGROUND)) { //!OCLINT(early exit) if (tcgetpgrp(STDIN_FILENO) == j->pgid) { - // We've already assigned the process group control of the terminal when the first - // process in the job was started. There's no need to do so again, and on some platforms - // this can cause an EPERM error. In addition, if we've given control of the terminal to - // a process group, attempting to call tcsetpgrp from the background will cause SIGTTOU - // to be sent to everything in our process group (unless we handle it). + //we've already assigned the process group control of the terminal when the first process in the job + //was started. There's no need to do so again, and on some platforms this can cause an EPERM error. + //In addition, if we've given control of the terminal to a process group, attempting to call tcsetpgrp + //from the background will cause SIGTTOU to be sent to everything in our process group (unless we + //handle it).. debug(4, L"Process group %d already has control of terminal\n", j->pgid); } else { - // No need to duplicate the code here, a function already exists that does just this. + //no need to duplicate the code here, a function already exists that does just this retval = terminal_give_to_job(j, false /*new job, so not continuing*/); } } @@ -264,7 +264,7 @@ int setup_child_process(process_t *p, const io_chain_t &io_chain) { bool ok = true; if (ok) { - // In the case of IO_FILE, this can hang until data is available to read/write! + //In the case of IO_FILE, this can hang until data is available to read/write! ok = (0 == handle_child_io(io_chain)); if (p != 0 && !ok) { debug_safe(4, "handle_child_io failed in setup_child_process"); diff --git a/src/postfork.h b/src/postfork.h index f29be4741..4a3b47e4e 100644 --- a/src/postfork.h +++ b/src/postfork.h @@ -27,6 +27,7 @@ bool child_set_group(job_t *j, process_t *p); //called by child /// inside the exec function, which blocks all signals), and all IO redirections and other file /// descriptor actions are performed. /// +/// \param j the job to set up the IO for /// \param p the child process to set up /// \param io_chain the IO chain to use /// diff --git a/src/proc.cpp b/src/proc.cpp index 3f005b34f..b0ebdf893 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -791,56 +791,53 @@ bool terminal_give_to_job(job_t *j, int cont) { signal_block(); - // It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no - // longer the controlling process group for the terminal and no longer have permission to set - // the process group that is in control, causing tcsetpgrp to return EPERM, even though that's - // not the documented behavior in tcsetpgrp(3), which instead says other bad things will happen - // (it says SIGTTOU will be sent to all members of the background *calling* process group, but - // it's more complicated than that, SIGTTOU may or may not be sent depending on the TTY - // configuration and whether or not signal handlers for SIGTTOU are installed. Read: - // http://curiousthing.org/sigttin-sigttou-deep-dive-linux In all cases, our goal here was just - // to hand over control of the terminal to this process group, which is a no-op if it's already - // been done. + //It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no longer + //the controlling process group for the terminal and no longer have permission to set the process + //group that is in control, causing tcsetpgrp to return EPERM, even though that's not the documented + //behavior in tcsetpgrp(3), which instead says other bad things will happen (it says SIGTTOU will be + //sent to all members of the background *calling* process group, but it's more complicated than that, + //SIGTTOU may or may not be sent depending on the TTY configuration and whether or not signal handlers + //for SIGTTOU are installed. Read: http://curiousthing.org/sigttin-sigttou-deep-dive-linux + //In all cases, our goal here was just to hand over control of the terminal to this process group, + //which is a no-op if it's already been done. if (tcgetpgrp(STDIN_FILENO) == j->pgid) { debug(2, L"Process group %d already has control of terminal\n", j->pgid); } else { debug(4, L"Attempting to bring process group to foreground via tcsetpgrp for job->pgid %d\n", j->pgid); - // The tcsetpgrp(2) man page says that EPERM is thrown if "pgrp has a supported value, but - // is not the process group ID of a process in the same session as the calling process." - // Since we _guarantee_ that this isn't the case (the child calls setpgid before it calls - // SIGSTOP, and the child was created in the same session as us), it seems that EPERM is - // being thrown because of an caching issue - the call to tcsetpgrp isn't seeing the - // newly-created process group just yet. On this developer's test machine (WSL running Linux - // 4.4.0), EPERM does indeed disappear on retry. The important thing is that we can - // guarantee the process isn't going to exit while we wait (which would cause us to possibly - // block indefinitely). + //the tcsetpgrp(2) man page says that EPERM is thrown if "pgrp has a supported value, but is not the + //process group ID of a process in the same session as the calling process." + //Since we _guarantee_ that this isn't the case (the child calls setpgid before it calls SIGSTOP, and + //the child was created in the same session as us), it seems that EPERM is being thrown because of an + //caching issue - the call to tcsetpgrp isn't seeing the newly-created process group just yet. On this + //developer's test machine (WSL running Linux 4.4.0), EPERM does indeed disappear on retry. The important + //thing is that we can guarantee the process isn't going to exit while we wait (which would cause us to + //possibly block indefinitely). + while (tcsetpgrp(STDIN_FILENO, j->pgid) != 0) { bool pgroup_terminated = false; if (errno == EINTR) { - ; // Always retry on EINTR, see comments in tcsetattr EINTR code below. + //always retry on EINTR, see comments in tcsetattr EINTR code below. } else if (errno == EINVAL) { - // OS X returns EINVAL if the process group no longer lives. Probably other OSes, - // too. Unlike EPERM below, EINVAL can only happen if the process group has - // terminated. + //OS X returns EINVAL if the process group no longer lives. Probably other OSes, too. + //Unlike EPERM below, EINVAL can only happen if the process group has terminated pgroup_terminated = true; } else if (errno == EPERM) { - // Retry so long as this isn't because the process group is dead. + //retry so long as this isn't because the process group is dead int wait_result = waitpid(-1 * j->pgid, &wait_result, WNOHANG); if (wait_result == -1) { - // Note that -1 is technically an "error" for waitpid in the sense that an - // invalid argument was specified because no such process group exists any - // longer. This is the observed behavior on Linux 4.4.0. a "success" result - // would mean processes from the group still exist but is still running in some - // state or the other. + //Note that -1 is technically an "error" for waitpid in the sense that an invalid argument was specified + //because no such process group exists any longer. This is the observed behavior on Linux 4.4.0. + //a "success" result would mean processes from the group still exist but is still running in some state + //or the other. pgroup_terminated = true; } else { - // Debug the original tcsetpgrp error (not the waitpid errno) to the log, and - // then retry until not EPERM or the process group has exited. + //debug the original tcsetpgrp error (not the waitpid errno) to the log, and then retry until not EPERM + //or the process group has exited. debug(2, L"terminal_give_to_job(): EPERM.\n", j->pgid); } } @@ -853,10 +850,9 @@ bool terminal_give_to_job(job_t *j, int cont) { } if (pgroup_terminated) { - // All processes in the process group has exited. Since we force all child procs to - // SIGSTOP on startup, the only way that can happen is if the very last process in - // the group terminated, and didn't need to access the terminal, otherwise it would - // have hung waiting for terminal IO (SIGTTIN). We can ignore this. + //all processes in the process group has exited. Since we force all child procs to SIGSTOP on startup, + //the only way that can happen is if the very last process in the group terminated, and didn't need + //to access the terminal, otherwise it would have hung waiting for terminal IO (SIGTTIN). We can ignore this. debug(3, L"tcsetpgrp called but process group %d has terminated.\n", j->pgid); break; }