From 7864d0d416519774c614148920530bc7da4a3e3b Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 7 Jan 2015 18:07:06 -0800 Subject: [PATCH] Rework file descriptor handling Remove global array of file descriptors, in favor of relying on CLO_EXEC exclusively. Also correctly implement "pipe avoidance" so that fd redirections do not conflict with pipes. --- exec.cpp | 184 +++++++++++++++---------------------------------- exec.h | 12 +--- fish_tests.cpp | 2 +- io.cpp | 97 +++++++++++++++++++++----- io.h | 12 +++- postfork.cpp | 69 +------------------ 6 files changed, 149 insertions(+), 227 deletions(-) diff --git a/exec.cpp b/exec.cpp index 7f108e2bb..18d5313d2 100644 --- a/exec.cpp +++ b/exec.cpp @@ -71,15 +71,6 @@ */ #define OPEN_MASK 0666 -/** - List of all pipes used by internal pipes. These must be closed in - many situations in order to make sure that stray fds aren't lying - around. - - Note this is used after fork, so we must not do anything that may allocate memory. Hopefully methods like open_fds.at() don't. -*/ -static std::vector open_fds; - // Called in a forked child static void exec_write_and_exit(int fd, const char *buff, size_t count, int status) { @@ -113,12 +104,6 @@ void exec_close(int fd) break; } } - - /* Maybe remove this from our set of open fds */ - if ((size_t)fd < open_fds.size()) - { - open_fds[fd] = false; - } } int exec_pipe(int fd[2]) @@ -136,15 +121,11 @@ int exec_pipe(int fd[2]) } debug(4, L"Created pipe using fds %d and %d", fd[0], fd[1]); - - int max_fd = std::max(fd[0], fd[1]); - if (max_fd >= 0 && open_fds.size() <= (size_t)max_fd) - { - open_fds.resize(max_fd + 1, false); - } - open_fds.at(fd[0]) = true; - open_fds.at(fd[1]) = true; - + + // Pipes ought to be cloexec + // Pipes are dup2'd the corresponding fds; the resulting fds are not cloexec + set_cloexec(fd[0]); + set_cloexec(fd[1]); return res; } @@ -182,83 +163,6 @@ static bool chain_contains_redirection_to_real_file(const io_chain_t &io_chain) return result; } -void print_open_fds(void) -{ - for (size_t i=0; i < open_fds.size(); ++i) - { - if (open_fds.at(i)) - { - fprintf(stderr, "fd %lu\n", (unsigned long) i); - } - } -} - -/** - Check if the specified fd is used as a part of a pipeline in the - specidied set of IO redirections. - This is called after fork(). - - \param fd the fd to search for - \param io_chain the set of io redirections to search in -*/ -static bool use_fd_in_pipe(int fd, const io_chain_t &io_chain) -{ - for (size_t idx = 0; idx < io_chain.size(); idx++) - { - const shared_ptr &io = io_chain.at(idx); - if ((io->io_mode == IO_BUFFER) || - (io->io_mode == IO_PIPE)) - { - CAST_INIT(const io_pipe_t *, io_pipe, io.get()); - if (io_pipe->pipe_fd[0] == fd || io_pipe->pipe_fd[1] == fd) - return true; - } - } - return false; -} - - -/** - Close all fds in open_fds, except for those that are mentioned in - the redirection list io. This should make sure that there are no - stray opened file descriptors in the child. - - \param io the list of io redirections for this job. Pipes mentioned - here should not be closed. -*/ -void close_unused_internal_pipes(const io_chain_t &io) -{ - /* A call to exec_close will modify open_fds, so be careful how we walk */ - for (size_t i=0; i < open_fds.size(); i++) - { - if (open_fds[i]) - { - int fd = (int)i; - if (!use_fd_in_pipe(fd, io)) - { - debug(4, L"Close fd %d, used in other context", fd); - exec_close(fd); - i--; - } - } - } -} - -void get_unused_internal_pipes(std::vector &fds, const io_chain_t &io) -{ - for (size_t i=0; i < open_fds.size(); i++) - { - if (open_fds[i]) - { - int fd = (int)i; - if (!use_fd_in_pipe(fd, io)) - { - fds.push_back(fd); - } - } - } -} - /** Returns the interpreter for the specified script. Returns NULL if file is not a script with a shebang. @@ -657,6 +561,24 @@ void exec_job(parser_t &parser, job_t *j) } assert(0 && "This should be unreachable"); } + + /* We may have block IOs that conflict with fd redirections. For example, we may have a command with a redireciton like <&3; we may also have chosen 3 as the fd for our pipe. Ensure we have no conflicts. */ + for (size_t i=0; i < all_ios.size(); i++) + { + io_data_t *io = all_ios.at(i).get(); + if (io->io_mode == IO_BUFFER) + { + CAST_INIT(io_buffer_t *, io_buffer, io); + if (! io_buffer->avoid_conflicts_with_io_chain(all_ios)) + { + /* We could not avoid conflicts, probably due to fd exhaustion. Mark an error. */ + exec_error = true; + job_mark_process_as_failed(j, j->first_process); + break; + } + } + } + signal_block(); @@ -670,7 +592,7 @@ void exec_job(parser_t &parser, job_t *j) exit. */ - if (job_get_flag(j, JOB_CONTROL)) + if (job_get_flag(j, JOB_CONTROL) && ! exec_error) { for (const process_t *p = j->first_process; p; p = p->next) { @@ -734,7 +656,7 @@ void exec_job(parser_t &parser, job_t *j) */ int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1; - for (process_t *p=j->first_process; p; p = p->next) + for (process_t *p=j->first_process; p != NULL && ! exec_error; p = p->next) { /* The IO chain for this process. It starts with the block IO, then pipes, and then gets any from the process */ io_chain_t process_net_io_chain = j->block_io_chain(); @@ -808,10 +730,7 @@ void exec_job(parser_t &parser, job_t *j) env_export_arr(true); - /* - Set up fd:s that will be used in the pipe - */ - + /* Set up fds that will be used in the pipe. */ if (pipes_to_next_command) { // debug( 1, L"%ls|%ls" , p->argv[0], p->next->argv[0]); @@ -825,7 +744,19 @@ void exec_job(parser_t &parser, job_t *j) break; } + /* Ensure our pipe fds not conflict with any fd redirections. E.g. if the process is like 'cat <&5' then fd 5 must not be used by the pipe. */ + if (! pipe_avoid_conflicts_with_io_chain(local_pipe, all_ios)) + { + /* We failed. The pipes were closed for us. */ + wperror(L"dup"); + exec_error = true; + job_mark_process_as_failed(j, p); + break; + } + // This tells the redirection about the fds, but the redirection does not close them + assert(local_pipe[0] >= 0); + assert(local_pipe[1] >= 0); memcpy(pipe_write->pipe_fd, local_pipe, sizeof(int)*2); // Record our pipes @@ -837,9 +768,6 @@ void exec_job(parser_t &parser, job_t *j) pipe_next_read = local_pipe[0]; } - //fprintf(stderr, "before IO: "); - //io_print(j->io); - // This is the IO buffer we use for storing the output of a block or function when it is in a pipeline shared_ptr block_output_io_buffer; switch (p->type) @@ -888,7 +816,7 @@ void exec_job(parser_t &parser, job_t *j) if (p->next) { // Be careful to handle failure, e.g. too many open fds - block_output_io_buffer.reset(io_buffer_t::create(STDOUT_FILENO)); + block_output_io_buffer.reset(io_buffer_t::create(STDOUT_FILENO, all_ios)); if (block_output_io_buffer.get() == NULL) { exec_error = true; @@ -916,7 +844,7 @@ void exec_job(parser_t &parser, job_t *j) { if (p->next) { - block_output_io_buffer.reset(io_buffer_t::create(STDOUT_FILENO)); + block_output_io_buffer.reset(io_buffer_t::create(STDOUT_FILENO, all_ios)); if (block_output_io_buffer.get() == NULL) { /* We failed (e.g. no more fds could be created). */ @@ -939,7 +867,7 @@ void exec_job(parser_t &parser, job_t *j) case INTERNAL_BUILTIN: { - int builtin_stdin=0; + int local_builtin_stdin = 0; bool close_stdin = false; /* @@ -959,13 +887,13 @@ void exec_job(parser_t &parser, job_t *j) case IO_FD: { CAST_INIT(const io_fd_t *, in_fd, in.get()); - builtin_stdin = in_fd->old_fd; + local_builtin_stdin = in_fd->old_fd; break; } case IO_PIPE: { CAST_INIT(const io_pipe_t *, in_pipe, in.get()); - builtin_stdin = in_pipe->pipe_fd[0]; + local_builtin_stdin = in_pipe->pipe_fd[0]; break; } @@ -973,9 +901,9 @@ void exec_job(parser_t &parser, job_t *j) { /* Do not set CLO_EXEC because child needs access */ CAST_INIT(const io_file_t *, in_file, in.get()); - builtin_stdin=open(in_file->filename_cstr, + local_builtin_stdin=open(in_file->filename_cstr, in_file->flags, OPEN_MASK); - if (builtin_stdin == -1) + if (local_builtin_stdin == -1) { debug(1, FILE_ERROR, @@ -999,14 +927,14 @@ void exec_job(parser_t &parser, job_t *j) really don't do anything. How should this be handled? */ - builtin_stdin = -1; + local_builtin_stdin = -1; break; } default: { - builtin_stdin=-1; + local_builtin_stdin=-1; debug(1, _(L"Unknown input redirection type %d"), in->io_mode); @@ -1018,10 +946,10 @@ void exec_job(parser_t &parser, job_t *j) } else { - builtin_stdin = pipe_read->pipe_fd[0]; + local_builtin_stdin = pipe_read->pipe_fd[0]; } - if (builtin_stdin == -1) + if (local_builtin_stdin == -1) { exec_error = true; break; @@ -1046,7 +974,7 @@ void exec_job(parser_t &parser, job_t *j) to make exec handle things. */ - builtin_push_io(parser, builtin_stdin); + builtin_push_io(parser, local_builtin_stdin); builtin_out_redirect = has_fd(process_net_io_chain, STDOUT_FILENO); builtin_err_redirect = has_fd(process_net_io_chain, STDERR_FILENO); @@ -1055,7 +983,7 @@ void exec_job(parser_t &parser, job_t *j) job_set_flag(j, JOB_FOREGROUND, 0); signal_unblock(); - + p->status = builtin_run(parser, p->get_argv(), process_net_io_chain); builtin_out_redirect=old_out; @@ -1077,7 +1005,7 @@ void exec_job(parser_t &parser, job_t *j) */ if (close_stdin) { - exec_close(builtin_stdin); + exec_close(local_builtin_stdin); } break; } @@ -1284,7 +1212,6 @@ void exec_job(parser_t &parser, job_t *j) if (g_log_forks) { printf("fork #%d: Executing fork for internal builtin for '%ls'\n", g_fork_count, p->argv0()); - io_print(process_net_io_chain); } pid = execute_fork(false); if (pid == 0) @@ -1329,15 +1256,12 @@ void exec_job(parser_t &parser, job_t *j) std::string actual_cmd_str = wcs2string(p->actual_cmd); const char *actual_cmd = actual_cmd_str.c_str(); - + const wchar_t *reader_current_filename(void); if (g_log_forks) { const wchar_t *file = reader_current_filename(); printf("fork #%d: forking for '%s' in '%ls'\n", g_fork_count, actual_cmd, file ? file : L""); - - fprintf(stderr, "IO chain for %s:\n", actual_cmd); - io_print(process_net_io_chain); } #if FISH_USE_POSIX_SPAWN @@ -1501,7 +1425,7 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst, boo int subcommand_status = -1; //assume the worst // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may be null - const shared_ptr io_buffer(io_buffer_t::create(STDOUT_FILENO)); + const shared_ptr io_buffer(io_buffer_t::create(STDOUT_FILENO, io_chain_t())); if (io_buffer.get() != NULL) { parser_t &parser = parser_t::principal_parser(); diff --git a/exec.h b/exec.h index 10eda29b6..f0a0e148f 100644 --- a/exec.h +++ b/exec.h @@ -60,22 +60,16 @@ int exec_subshell(const wcstring &cmd, bool preserve_exit_status); /** Loops over close until the syscall was run without being - interrupted. Then removes the fd from the open_fds list. + interrupted. */ void exec_close(int fd); /** - Call pipe(), and add resulting fds to open_fds, the list of opend - file descriptors for pipes. + Call pipe(), and add resulting fds to open_fds, the list of opened + file descriptors for pipes. The pipes are marked CLO_EXEC. */ int exec_pipe(int fd[2]); -/* Close all fds in open_fds. This is called from postfork.cpp */ -void close_unused_internal_pipes(const io_chain_t &io); - -/* Gets all unused internal pipes into fds */ -void get_unused_internal_pipes(std::vector &fds, const io_chain_t &io); - /** Gets the interpreter for a given command */ char *get_interpreter(const char *command, char *interpreter, size_t buff_size); diff --git a/fish_tests.cpp b/fish_tests.cpp index 9fa6a110f..a1f984f3a 100644 --- a/fish_tests.cpp +++ b/fish_tests.cpp @@ -773,7 +773,7 @@ static int signal_main(test_cancellation_info_t *info) static void test_1_cancellation(const wchar_t *src) { - shared_ptr out_buff(io_buffer_t::create(STDOUT_FILENO)); + shared_ptr out_buff(io_buffer_t::create(STDOUT_FILENO, io_chain_t())); const io_chain_t io_chain(out_buff); test_cancellation_info_t ctx = {pthread_self(), 0.25 /* seconds */ }; iothread_perform(signal_main, (void (*)(test_cancellation_info_t *, int))NULL, &ctx); diff --git a/io.cpp b/io.cpp index 7f9d5d445..264f0b9ff 100644 --- a/io.cpp +++ b/io.cpp @@ -124,8 +124,17 @@ void io_buffer_t::read() } } +bool io_buffer_t::avoid_conflicts_with_io_chain(const io_chain_t &ios) +{ + bool result = pipe_avoid_conflicts_with_io_chain(this->pipe_fd, ios); + if (! result) + { + wperror(L"dup"); + } + return result; +} -io_buffer_t *io_buffer_t::create(int fd) +io_buffer_t *io_buffer_t::create(int fd, const io_chain_t &conflicts) { bool success = true; assert(fd >= 0); @@ -137,6 +146,11 @@ io_buffer_t *io_buffer_t::create(int fd) wperror(L"pipe"); success = false; } + else if (! buffer_redirect->avoid_conflicts_with_io_chain(conflicts)) + { + // The above call closes the fds on error + success = false; + } else if (make_fd_nonblocking(buffer_redirect->pipe_fd[0]) != 0) { debug(1, PIPE_ERROR); @@ -149,27 +163,11 @@ io_buffer_t *io_buffer_t::create(int fd) delete buffer_redirect; buffer_redirect = NULL; } - else - { - //fprintf(stderr, "Created pipes {%d, %d} for %p\n", buffer_redirect->pipe_fd[0], buffer_redirect->pipe_fd[1], buffer_redirect); - } - return buffer_redirect; } io_buffer_t::~io_buffer_t() { - - //fprintf(stderr, "Deallocating pipes {%d, %d} for %p\n", this->pipe_fd[0], this->pipe_fd[1], this); - /** - If this is an input buffer, then io_read_buffer will not have - been called, and we need to close the output fd as well. - */ - if (is_input && pipe_fd[1] >= 0) - { - exec_close(pipe_fd[1]); - } - if (pipe_fd[0] >= 0) { exec_close(pipe_fd[0]); @@ -236,6 +234,71 @@ void io_print(const io_chain_t &chain) } } +/* If the given fd is used by the io chain, duplicates it repeatedly until an fd not used in the io chain is found, or we run out. If we return a new fd or an error, closes the old one. Any fd created is marked close-on-exec. Returns -1 on failure (in which case the given fd is still closed). */ +static int move_fd_to_unused(int fd, const io_chain_t &io_chain) +{ + int new_fd = fd; + if (fd >= 0 && io_chain.get_io_for_fd(fd).get() != NULL) + { + /* We have fd >= 0, and it's a conflict. dup it and recurse. Note that we recurse before anything is closed; this forces the kernel to give us a new one (or report fd exhaustion). */ + int tmp_fd; + do + { + tmp_fd = dup(fd); + } while (tmp_fd < 0 && errno == EINTR); + + assert(tmp_fd != fd); + if (tmp_fd < 0) + { + /* Likely fd exhaustion. */ + new_fd = -1; + } + else + { + /* Ok, we have a new candidate fd. Recurse. If we get a valid fd, either it's the same as what we gave it, or it's a new fd and what we gave it has been closed. If we get a negative value, the fd also has been closed. */ + set_cloexec(tmp_fd); + new_fd = move_fd_to_unused(tmp_fd, io_chain); + } + + /* We're either returning a new fd or an error. In both cases, we promise to close the old one. */ + assert(new_fd != fd); + int saved_errno = errno; + exec_close(fd); + errno = saved_errno; + } + return new_fd; +} + +bool pipe_avoid_conflicts_with_io_chain(int fds[2], const io_chain_t &ios) +{ + bool success = true; + for (int i=0; i < 2; i++) + { + fds[i] = move_fd_to_unused(fds[i], ios); + if (fds[i] < 0) + { + success = false; + break; + } + } + + /* If any fd failed, close all valid fds */ + if (! success) + { + int saved_errno = errno; + for (int i=0; i < 2; i++) + { + if (fds[i] >= 0) + { + exec_close(fds[i]); + fds[i] = -1; + } + } + errno = saved_errno; + } + return success; +} + /* Return the last IO for the given fd */ shared_ptr io_chain_t::get_io_for_fd(int fd) const { diff --git a/io.h b/io.h index 38c1c7baa..98657e7e5 100644 --- a/io.h +++ b/io.h @@ -122,6 +122,7 @@ public: } }; +class io_chain_t; class io_buffer_t : public io_pipe_t { private: @@ -161,6 +162,9 @@ public: { return out_buffer.size(); } + + /* Ensures that the pipes do not conflict with any fd redirections in the chain */ + bool avoid_conflicts_with_io_chain(const io_chain_t &ios); /** Close output pipe, and read from input pipe until eof. @@ -170,11 +174,13 @@ public: /** Create a IO_BUFFER type io redirection, complete with a pipe and a vector for output. The default file descriptor used is STDOUT_FILENO - for buffering + for buffering. \param fd the fd that will be mapped in the child process, typically STDOUT_FILENO + \param conflicts A set of IO redirections. The function ensures that any pipe it makes + does not conflict with an fd redirection in this list. */ - static io_buffer_t *create(int fd); + static io_buffer_t *create(int fd, const io_chain_t &conflicts); }; class io_chain_t : public std::vector > @@ -199,6 +205,8 @@ public: shared_ptr io_chain_get(const io_chain_t &src, int fd); shared_ptr io_chain_get(io_chain_t &src, int fd); +/* Given a pair of fds, if an fd is used by the given io chain, duplicate that fd repeatedly until we find one that does not conflict, or we run out of fds. Returns the new fds by reference, closing the old ones. If we get an error, returns false (in which case both fds are closed and set to -1). */ +bool pipe_avoid_conflicts_with_io_chain(int fds[2], const io_chain_t &ios); /** Print debug information about the specified IO redirection chain to stderr. */ void io_print(const io_chain_t &chain); diff --git a/postfork.cpp b/postfork.cpp index 80318c473..8222862c2 100644 --- a/postfork.cpp +++ b/postfork.cpp @@ -107,53 +107,6 @@ int set_child_group(job_t *j, process_t *p, int print_errors) return res; } -/** Make sure the fd used by each redirection is not used by a pipe. Note that while this does not modify the vector, it does modify the IO redirections within (gulp) */ -static void free_redirected_fds_from_pipes(const io_chain_t &io_chain) -{ - size_t max = io_chain.size(); - for (size_t i = 0; i < max; i++) - { - int fd_to_free = io_chain.at(i)->fd; - - /* We only have to worry about fds beyond the three standard ones */ - if (fd_to_free <= 2) - continue; - - /* Make sure the fd is not used by a pipe */ - for (size_t j = 0; j < max; j++) - { - /* We're only interested in pipes */ - io_data_t *io = io_chain.at(j).get(); - if (io->io_mode != IO_PIPE && io->io_mode != IO_BUFFER) - continue; - - CAST_INIT(io_pipe_t *, possible_conflict, io); - /* If the pipe is a conflict, dup it to some other value */ - for (int k=0; k<2; k++) - { - /* If it's not a conflict, we don't care */ - if (possible_conflict->pipe_fd[k] != fd_to_free) - continue; - - /* Repeat until we have a replacement fd */ - int replacement_fd = -1; - while (replacement_fd < 0) - { - replacement_fd = dup(fd_to_free); - if (replacement_fd == -1 && errno != EINTR) - { - debug_safe_int(1, FD_ERROR, fd_to_free); - safe_perror("dup"); - FATAL_EXIT(); - } - } - possible_conflict->pipe_fd[k] = replacement_fd; - } - } - } -} - - /** Set up a childs io redirections. Should only be called by setup_child_process(). Does the following: First it closes any open @@ -168,9 +121,6 @@ static void free_redirected_fds_from_pipes(const io_chain_t &io_chain) */ static int handle_child_io(const io_chain_t &io_chain) { - //fprintf(stderr, "child IO for %d\n", getpid()); - close_unused_internal_pipes(io_chain); - free_redirected_fds_from_pipes(io_chain); for (size_t idx = 0; idx < io_chain.size(); idx++) { const io_data_t *io = io_chain.at(idx).get(); @@ -442,18 +392,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, posix_spawn_fil sigemptyset(&sigmask); if (! err && reset_sigmask) err = posix_spawnattr_setsigmask(attr, &sigmask); - - /* Make sure that our pipes don't use an fd that the redirection itself wants to use */ - free_redirected_fds_from_pipes(io_chain); - - /* Close unused internal pipes */ - std::vector files_to_close; - get_unused_internal_pipes(files_to_close, io_chain); - for (size_t i = 0; ! err && i < files_to_close.size(); i++) - { - err = posix_spawn_file_actions_addclose(actions, files_to_close.at(i)); - } - + for (size_t idx = 0; idx < io_chain.size(); idx++) { const shared_ptr io = io_chain.at(idx); @@ -465,12 +404,6 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, posix_spawn_fil continue; } - if (io->fd > 2) - { - /* Make sure the fd used by this redirection is not used by e.g. a pipe. */ - // free_fd(io_chain, io->fd ); - // PCA I don't think we need to worry about this. fd redirection is pretty uncommon anyways. - } switch (io->io_mode) { case IO_CLOSE: