diff --git a/env_universal.cpp b/env_universal.cpp index 5fcd97a65..c7d060ad7 100644 --- a/env_universal.cpp +++ b/env_universal.cpp @@ -135,7 +135,7 @@ static int try_get_socket_once(void) return -1; } - if ((fcntl(s, F_SETFL, O_NONBLOCK) != 0) || (fcntl(s, F_SETFD, FD_CLOEXEC) != 0)) + if ((make_fd_nonblocking(s) != 0) || (fcntl(s, F_SETFD, FD_CLOEXEC) != 0)) { wperror(L"fcntl"); close(s); diff --git a/exec.cpp b/exec.cpp index 633a16e43..3c5d74ed0 100644 --- a/exec.cpp +++ b/exec.cpp @@ -502,10 +502,10 @@ static void internal_exec_helper(parser_t &parser, } /* Returns whether we can use posix spawn for a given process in a given job. - Per https://github.com/fish-shell/fish-shell/issues/364 , error handling for file redirections is too difficult with posix_spawn - So in that case we use fork/exec + Per https://github.com/fish-shell/fish-shell/issues/364 , error handling for file redirections is too difficult with posix_spawn, + so in that case we use fork/exec. - Furthermore, to avoid the race between the caller calling tcsetpgrp() and the client checking the foreground process group, we don't use posix_spawn if we're going to foreground the process. (If we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the racse). + Furthermore, to avoid the race between the caller calling tcsetpgrp() and the client checking the foreground process group, we don't use posix_spawn if we're going to foreground the process. (If we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the race). */ static bool can_use_posix_spawn_for_job(const job_t *job, const process_t *process) { @@ -1312,6 +1312,9 @@ void exec(parser_t &parser, job_t *j) /* Get argv and envv before we fork */ null_terminated_array_t argv_array; convert_wide_array_to_narrow(p->get_argv_array(), &argv_array); + + /* Ensure that stdin is blocking before we hand it off (see issue #176). It's a little strange that we only do this with stdin and not with stdout or stderr. However in practice, setting or clearing O_NONBLOCK on stdin also sets it for the other two fds, presumably because they refer to the same underlying file (/dev/tty?) */ + make_fd_blocking(STDIN_FILENO); const char * const *argv = argv_array.get(); const char * const *envv = env_export_arr(false); diff --git a/fishd.cpp b/fishd.cpp index b703b3960..638e17622 100644 --- a/fishd.cpp +++ b/fishd.cpp @@ -578,7 +578,7 @@ static int get_socket(void) goto unlock; } - if (fcntl(s, F_SETFL, O_NONBLOCK) != 0) + if (make_fd_nonblocking(s) != 0) { wperror(L"fcntl"); close(s); @@ -988,7 +988,7 @@ int main(int argc, char ** argv) { debug(4, L"Connected with new child on fd %d", child_socket); - if (fcntl(child_socket, F_SETFL, O_NONBLOCK) != 0) + if (make_fd_nonblocking(child_socket) != 0) { wperror(L"fcntl"); close(child_socket); diff --git a/input_common.cpp b/input_common.cpp index ef979c3c5..0f9fa52bd 100644 --- a/input_common.cpp +++ b/input_common.cpp @@ -81,7 +81,7 @@ void input_common_destroy() } /** - Internal function used by input_common_readch to read one byte from fd 1. This function should only be called by + Internal function used by input_common_readch to read one byte from fd 0. This function should only be called by input_common_readch(). */ static wint_t readb() diff --git a/io.cpp b/io.cpp index eb7d432d0..da1ebb0cd 100644 --- a/io.cpp +++ b/io.cpp @@ -138,9 +138,7 @@ io_buffer_t *io_buffer_t::create(bool is_input, int fd) wperror(L"pipe"); success = false; } - else if (fcntl(buffer_redirect->pipe_fd[0], - F_SETFL, - O_NONBLOCK)) + else if (make_fd_nonblocking(buffer_redirect->pipe_fd[0]) != 0) { debug(1, PIPE_ERROR); wperror(L"fcntl"); diff --git a/proc.cpp b/proc.cpp index dd94da18b..c8d6c598c 100644 --- a/proc.cpp +++ b/proc.cpp @@ -362,9 +362,7 @@ int job_signal(job_t *j, int signal) Return 0 if all went well, nonzero otherwise. This is called from a signal handler. */ -static void mark_process_status(const job_t *j, - process_t *p, - int status) +static void mark_process_status(const job_t *j, process_t *p, int status) { // debug( 0, L"Process %ls %ls", p->argv[0], WIFSTOPPED (status)?L"stopped":(WIFEXITED( status )?L"exited":(WIFSIGNALED( status )?L"signaled to exit":L"BLARGH")) ); p->status = status; @@ -418,8 +416,8 @@ void job_mark_process_as_failed(const job_t *job, process_t *p) static void handle_child_status(pid_t pid, int status) { bool found_proc = false; - const job_t *j=0; - process_t *p=0; + const job_t *j = NULL; + process_t *p = NULL; // char mess[MESS_SIZE]; /* snprintf( mess, @@ -965,7 +963,7 @@ static void read_try(job_t *j) a job that has previously been stopped. In that case, we need to set the terminal attributes to those saved in the job. */ -static int terminal_give_to_job(job_t *j, int cont) +static bool terminal_give_to_job(job_t *j, int cont) { if (tcsetpgrp(0, j->pgid)) @@ -975,7 +973,7 @@ static int terminal_give_to_job(job_t *j, int cont) j->job_id, j->command_wcstr()); wperror(L"tcsetpgrp"); - return 0; + return false; } if (cont) @@ -987,10 +985,10 @@ static int terminal_give_to_job(job_t *j, int cont) j->job_id, j->command_wcstr()); wperror(L"tcsetattr"); - return 0; + return false; } } - return 1; + return true; } /** @@ -1059,18 +1057,17 @@ void job_continue(job_t *j, bool cont) { if (job_get_flag(j, JOB_TERMINAL) && job_get_flag(j, JOB_FOREGROUND)) { - /* Put the job into the foreground. */ - int ok; - + /* Put the job into the foreground. Hack: ensure that stdin is marked as blocking first (#176). */ + make_fd_blocking(STDIN_FILENO); + signal_block(); - ok = terminal_give_to_job(j, cont); + bool ok = terminal_give_to_job(j, cont); signal_unblock(); if (!ok) return; - } /* @@ -1186,7 +1183,6 @@ void job_continue(job_t *j, bool cont) // were sometimes having their output combined with the set_color calls in the wrong order! read_try(j); - process_t *p = j->first_process; while (p->next) p = p->next; @@ -1205,9 +1201,8 @@ void job_continue(job_t *j, bool cont) } } } - /* - Put the shell back in the foreground. - */ + + /* Put the shell back in the foreground. */ if (job_get_flag(j, JOB_TERMINAL) && job_get_flag(j, JOB_FOREGROUND)) { int ok; diff --git a/reader.cpp b/reader.cpp index 2d4c1d4a9..c664c4f63 100644 --- a/reader.cpp +++ b/reader.cpp @@ -2328,12 +2328,12 @@ void set_env_cmd_duration(struct timeval *after, struct timeval *before) } } -void reader_run_command(parser_t &parser, const wchar_t *cmd) +void reader_run_command(parser_t &parser, const wcstring &cmd) { struct timeval time_before, time_after; - wcstring ft = tok_first(cmd); + wcstring ft = tok_first(cmd.c_str()); if (! ft.empty()) env_set(L"_", ft.c_str(), ENV_GLOBAL); @@ -2709,7 +2709,7 @@ static void handle_end_loop() Read interactively. Read input from stdin while providing editing facilities. */ -static int read_i() +static int read_i(void) { reader_push(L"fish"); reader_set_complete_function(&complete); @@ -2724,8 +2724,6 @@ static int read_i() while ((!data->end_loop) && (!sanity_check())) { - const wchar_t *tmp; - event_fire_generic(L"fish_prompt"); if (function_exists(LEFT_PROMPT_FUNCTION_NAME)) reader_set_left_prompt(LEFT_PROMPT_FUNCTION_NAME); @@ -2744,9 +2742,7 @@ static int read_i() during evaluation. */ - - tmp = reader_readline(); - + const wchar_t *tmp = reader_readline(); if (data->end_loop) { @@ -2754,13 +2750,11 @@ static int read_i() } else if (tmp) { - tmp = wcsdup(tmp); - + wcstring command = tmp; data->buff_pos=0; data->command_line.clear(); data->command_line_changed(); - reader_run_command(parser, tmp); - free((void *)tmp); + reader_run_command(parser, command); if (data->end_loop) { handle_end_loop(); @@ -2837,7 +2831,7 @@ static wchar_t unescaped_quote(const wcstring &str, size_t pos) } -const wchar_t *reader_readline() +const wchar_t *reader_readline(void) { wint_t c; int last_char=0; @@ -3612,7 +3606,7 @@ static int read_ni(int fd, const io_chain_t &io) wchar_t *buff=0; std::vector acc; - int des = fd == 0 ? dup(0) : fd; + int des = (fd == STDIN_FILENO ? dup(STDIN_FILENO) : fd); int res=0; if (des == -1) @@ -3629,14 +3623,23 @@ static int read_ni(int fd, const io_chain_t &io) char buff[4096]; size_t c = fread(buff, 1, 4096, in_stream); - if (ferror(in_stream) && (errno != EINTR)) + if (ferror(in_stream)) { - debug(1, - _(L"Error while reading from file descriptor")); + if (errno == EINTR) + { + /* We got a signal, just keep going */ + continue; + } + else if ((errno == EAGAIN || errno == EWOULDBLOCK) && make_fd_blocking(des) == 0) + { + /* We succeeded in making the fd blocking, try again */ + continue; + } + + /* Fatal error */ + debug(1, _(L"Error while reading from file descriptor")); - /* - Reset buffer on error. We won't evaluate incomplete files. - */ + /* Reset buffer on error. We won't evaluate incomplete files. */ acc.clear(); break; diff --git a/reader.h b/reader.h index 80dbe2585..dfe8d1ed5 100644 --- a/reader.h +++ b/reader.h @@ -85,7 +85,7 @@ void reader_repaint_if_needed(); Run the specified command with the correct terminal modes, and while taking care to perform job notification, set the title, etc. */ -void reader_run_command(const wchar_t *buff); +void reader_run_command(const wcstring &buff); /** Get the string of character currently entered into the command diff --git a/wutil.cpp b/wutil.cpp index 0e52294d7..fe5a3f49f 100644 --- a/wutil.cpp +++ b/wutil.cpp @@ -264,12 +264,10 @@ int wstat(const wcstring &file_name, struct stat *buf) int lwstat(const wcstring &file_name, struct stat *buf) { - // fprintf(stderr, "%s\n", __PRETTY_FUNCTION__); cstring tmp = wcs2string(file_name); return lstat(tmp.c_str(), buf); } - int waccess(const wcstring &file_name, int mode) { cstring tmp = wcs2string(file_name); @@ -292,6 +290,28 @@ void wperror(const wcstring &s) fwprintf(stderr, L"%s\n", strerror(e)); } +int make_fd_nonblocking(int fd) +{ + int flags = fcntl(fd, F_GETFL, 0); + int err = 0; + if (! (flags & O_NONBLOCK)) + { + err = fcntl(fd, F_SETFL, flags | O_NONBLOCK); + } + return err == -1 ? errno : 0; +} + +int make_fd_blocking(int fd) +{ + int flags = fcntl(fd, F_GETFL, 0); + int err = 0; + if (flags & O_NONBLOCK) + { + err = fcntl(fd, F_SETFL, flags & ~O_NONBLOCK); + } + return err == -1 ? errno : 0; +} + static inline void safe_append(char *buffer, const char *s, size_t buffsize) { strncat(buffer, s, buffsize - strlen(buffer) - 1); diff --git a/wutil.h b/wutil.h index 396412c95..347e7313d 100644 --- a/wutil.h +++ b/wutil.h @@ -48,9 +48,13 @@ int wopen(const wcstring &pathname, int flags, mode_t mode = 0); /** Wide character version of open() that also sets the close-on-exec flag (atomically when possible). */ int wopen_cloexec(const wcstring &pathname, int flags, mode_t mode = 0); -/** - Wide character version of creat(). -*/ +/** Mark an fd as nonblocking; returns errno or 0 on success */ +int make_fd_nonblocking(int fd); + +/** Mark an fd as blocking; returns errno or 0 on success */ +int make_fd_blocking(int fd); + +/** Wide character version of creat(). */ int wcreat(const wcstring &pathname, mode_t mode);