Clean up reader_read

Stop having reader_read close the input file descriptor. Make other
modernizations.

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();

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++));
+            const char *file = *(argv + (my_optind++));
-            int fd = open(file, O_RDONLY);
+            autoclose_fd_t fd(open_cloexec(file, O_RDONLY));
-            if (fd == -1) {
+            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<const wchar_t *> 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"));

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;
+    // Read all data into a std::string.
-    std::vector<char> acc;
+    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);
+    wcstring str = str2wcstring(fd_contents);
-    int res = 0;
 
-    if (des == -1) {
+    // Eagerly deallocate to save memory.
-        wperror(L"dup");
+    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<bool> 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);

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.