From 376529a46d6ba84caf8f429310cf8f5b8b0e8fd5 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Tue, 28 Jan 2020 11:39:26 -0800 Subject: [PATCH] Clean up reader_read Stop having reader_read close the input file descriptor. Make other modernizations. --- src/builtin_source.cpp | 16 ++++-- src/fish.cpp | 11 ++-- src/reader.cpp | 112 +++++++++++++++++------------------------ src/reader.h | 1 + 4 files changed, 62 insertions(+), 78 deletions(-) diff --git a/src/builtin_source.cpp b/src/builtin_source.cpp index 0765053fb..633487e90 100644 --- a/src/builtin_source.cpp +++ b/src/builtin_source.cpp @@ -37,7 +37,12 @@ int builtin_source(parser_t &parser, io_streams_t &streams, wchar_t **argv) { return STATUS_CMD_OK; } - int fd; + // If we open a file, this ensures we close it. + autoclose_fd_t opened_fd; + + // The fd that we read from, either from opened_fd or stdin. + int fd = -1; + struct stat buf; const wchar_t *fn, *fn_intern; @@ -49,17 +54,18 @@ int builtin_source(parser_t &parser, io_streams_t &streams, wchar_t **argv) { } fn = L"-"; fn_intern = fn; - fd = dup(streams.stdin_fd); + fd = streams.stdin_fd; } else { - if ((fd = wopen_cloexec(argv[optind], O_RDONLY)) == -1) { + opened_fd = autoclose_fd_t(wopen_cloexec(argv[optind], O_RDONLY)); + if (!opened_fd.valid()) { streams.err.append_format(_(L"%ls: Error encountered while sourcing file '%ls':\n"), cmd, argv[optind]); builtin_wperror(cmd, streams); return STATUS_CMD_ERROR; } + fd = opened_fd.fd(); if (fstat(fd, &buf) == -1) { - close(fd); streams.err.append_format(_(L"%ls: Error encountered while sourcing file '%ls':\n"), cmd, argv[optind]); builtin_wperror(L"source", streams); @@ -67,13 +73,13 @@ int builtin_source(parser_t &parser, io_streams_t &streams, wchar_t **argv) { } if (!S_ISREG(buf.st_mode)) { - close(fd); streams.err.append_format(_(L"%ls: '%ls' is not a file\n"), cmd, argv[optind]); return STATUS_CMD_ERROR; } fn_intern = intern(argv[optind]); } + assert(fd >= 0 && "Should have a valid fd"); const block_t *sb = parser.push_block(block_t::source_block(fn_intern)); auto &ld = parser.libdata(); diff --git a/src/fish.cpp b/src/fish.cpp index 7db3ade1d..f4b07f8fa 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -493,14 +493,11 @@ int main(int argc, char **argv) { // Implicitly interactive mode. res = reader_read(parser, STDIN_FILENO, {}); } else { - char *file = *(argv + (my_optind++)); - int fd = open(file, O_RDONLY); - if (fd == -1) { + const char *file = *(argv + (my_optind++)); + autoclose_fd_t fd(open_cloexec(file, O_RDONLY)); + if (!fd.valid()) { perror(file); } else { - // OK to not do this atomically since we cannot have gone multithreaded yet. - set_cloexec(fd); - wcstring_list_t list; for (char **ptr = argv + my_optind; *ptr; ptr++) { list.push_back(str2wcstring(*ptr)); @@ -511,7 +508,7 @@ int main(int argc, char **argv) { wcstring rel_filename = str2wcstring(file); scoped_push filename_push{&ld.current_filename, intern(rel_filename.c_str())}; - res = reader_read(parser, fd, {}); + res = reader_read(parser, fd.fd(), {}); if (res) { FLOGF(warning, _(L"Error while reading file %ls\n"), ld.current_filename ? ld.current_filename : _(L"Standard input")); diff --git a/src/reader.cpp b/src/reader.cpp index 7e563b096..fe33f8031 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -3486,76 +3486,56 @@ bool reader_get_selection(size_t *start, size_t *len) { /// Read non-interactively. Read input from stdin without displaying the prompt, using syntax /// highlighting. This is used for reading scripts and init files. +/// The file is not closed. static int read_ni(parser_t &parser, int fd, const io_chain_t &io) { - FILE *in_stream; - std::vector acc; + // Read all data into a std::string. + std::string fd_contents; + for (;;) { + char buff[4096]; + size_t amt = read(fd, buff, sizeof buff); + if (amt > 0) { + fd_contents.append(buff, amt); + } else if (amt == 0) { + // EOF. + break; + } else { + int err = errno; + if (err == EINTR) { + continue; + } else if ((err == EAGAIN || err == EWOULDBLOCK) && make_fd_blocking(fd)) { + // We succeeded in making the fd blocking, keep going. + continue; + } else { + // Fatal error. + FLOGF(error, _(L"Unable to read input file: %s"), strerror(err)); + // Reset buffer on error. We won't evaluate incomplete files. + fd_contents.clear(); + } + } + } - int des = (fd == STDIN_FILENO ? dup(STDIN_FILENO) : fd); - int res = 0; + wcstring str = str2wcstring(fd_contents); - if (des == -1) { - wperror(L"dup"); + // Eagerly deallocate to save memory. + fd_contents.clear(); + fd_contents.shrink_to_fit(); + + // Swallow a BOM (issue #1518). + if (!str.empty() && str.at(0) == UTF8_BOM_WCHAR) { + str.erase(0, 1); + } + + parse_error_list_t errors; + parsed_source_ref_t pstree; + if (!parse_util_detect_errors(str, &errors, false /* do not accept incomplete */, &pstree)) { + parser.eval(pstree, io); + return 0; + } else { + wcstring sb; + parser.get_backtrace(str, errors, sb); + std::fwprintf(stderr, L"%ls", sb.c_str()); return 1; } - - in_stream = fdopen(des, "r"); - if (in_stream != nullptr) { - while (!feof(in_stream)) { - char buff[4096]; - size_t c = fread(buff, 1, 4096, in_stream); - - if (ferror(in_stream)) { - if (errno == EINTR) { - // We got a signal, just keep going. Be sure that we call insert() below because - // we may get data as well as EINTR. - clearerr(in_stream); - } else if ((errno == EAGAIN || errno == EWOULDBLOCK) && - make_fd_blocking(des) == 0) { - // We succeeded in making the fd blocking, keep going. - clearerr(in_stream); - } else { - // Fatal error. - FLOGF(error, _(L"Unable to read input file: %s"), strerror(errno)); - // Reset buffer on error. We won't evaluate incomplete files. - acc.clear(); - break; - } - } - - acc.insert(acc.end(), buff, buff + c); - } - - wcstring str = acc.empty() ? wcstring() : str2wcstring(&acc.at(0), acc.size()); - acc.clear(); - - if (fclose(in_stream)) { - FLOGF(warning, _(L"Error while closing input stream")); - wperror(L"fclose"); - res = 1; - } - - // Swallow a BOM (issue #1518). - if (!str.empty() && str.at(0) == UTF8_BOM_WCHAR) { - str.erase(0, 1); - } - - parse_error_list_t errors; - parsed_source_ref_t pstree; - if (!parse_util_detect_errors(str, &errors, false /* do not accept incomplete */, - &pstree)) { - parser.eval(pstree, io); - } else { - wcstring sb; - parser.get_backtrace(str, errors, sb); - std::fwprintf(stderr, L"%ls", sb.c_str()); - res = 1; - } - } else { - FLOGF(warning, _(L"Error while opening input stream")); - wperror(L"fdopen"); - res = 1; - } - return res; } int reader_read(parser_t &parser, int fd, const io_chain_t &io) { @@ -3582,7 +3562,7 @@ int reader_read(parser_t &parser, int fd, const io_chain_t &io) { scoped_push interactive_push{&parser.libdata().is_interactive, interactive}; signal_set_handlers_once(interactive); - res = parser.is_interactive() ? read_i(parser) : read_ni(parser, fd, io); + res = interactive ? read_i(parser) : read_ni(parser, fd, io); // If the exit command was called in a script, only exit the script, not the program. reader_set_end_loop(false); diff --git a/src/reader.h b/src/reader.h index 5f3127d56..a60883711 100644 --- a/src/reader.h +++ b/src/reader.h @@ -49,6 +49,7 @@ class editable_line_t { }; /// Read commands from \c fd until encountering EOF. +/// The fd is not closed. int reader_read(parser_t &parser, int fd, const io_chain_t &io); /// Tell the shell whether it should exit after the currently running command finishes.