From 3d47f042ac59156e64f14056ea14c826ab45cee1 Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Wed, 29 Jan 2020 13:55:10 -0800
Subject: [PATCH] Be more consistent about using autoclose_fd_t and exec_close

Simplifying and improving file descriptor handling discipline.
---
 src/common.cpp               | 14 +++++++++---
 src/common.h                 |  3 +++
 src/env_universal_common.cpp | 42 +++++++++++++++---------------------
 src/env_universal_common.h   |  2 +-
 src/exec.cpp                 | 14 ------------
 5 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/src/common.cpp b/src/common.cpp
index a23880e3e..aeab343a8 100644
--- a/src/common.cpp
+++ b/src/common.cpp
@@ -2071,12 +2071,20 @@ void exit_without_destructors(int code) { _exit(code); }
 
 void autoclose_fd_t::close() {
     if (fd_ < 0) return;
-    if (::close(fd_) == -1) {
-        wperror(L"close");
-    }
+    exec_close(fd_);
     fd_ = -1;
 }
 
+void exec_close(int fd) {
+    assert(fd >= 0 && "Invalid fd");
+    while (close(fd) == -1) {
+        if (errno != EINTR) {
+            wperror(L"close");
+            break;
+        }
+    }
+}
+
 extern "C" {
 [[gnu::noinline]] void debug_thread_error(void) {
     // Wait for a SIGINT. We can't use sigsuspend() because the signal may be delivered on another
diff --git a/src/common.h b/src/common.h
index f476f1d76..a3fc728c6 100644
--- a/src/common.h
+++ b/src/common.h
@@ -569,6 +569,9 @@ class autoclose_fd_t {
     ~autoclose_fd_t() { close(); }
 };
 
+/// Close a file descriptor \p fd, retrying on EINTR.
+void exec_close(int fd);
+
 wcstring format_string(const wchar_t *format, ...);
 wcstring vformat_string(const wchar_t *format, va_list va_orig);
 void append_format(wcstring &str, const wchar_t *format, ...);
diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp
index 6e9ae8620..8c29894b3 100644
--- a/src/env_universal_common.cpp
+++ b/src/env_universal_common.cpp
@@ -414,11 +414,10 @@ bool env_universal_t::load_from_path(const std::string &path, callback_data_list
     }
 
     bool result = false;
-    int fd = open_cloexec(path.c_str(), O_RDONLY);
-    if (fd >= 0) {
+    autoclose_fd_t fd{open_cloexec(path.c_str(), O_RDONLY)};
+    if (fd.valid()) {
         FLOGF(uvar_file, L"universal log reading from file");
-        this->load_from_fd(fd, callbacks);
-        close(fd);
+        this->load_from_fd(fd.fd(), callbacks);
         result = true;
     }
     return result;
@@ -571,7 +570,7 @@ static bool lock_uvar_file(int fd) {
     return check_duration(start_time);
 }
 
-bool env_universal_t::open_and_acquire_lock(const std::string &path, int *out_fd) {
+bool env_universal_t::open_and_acquire_lock(const std::string &path, autoclose_fd_t *out_fd) {
     // Attempt to open the file for reading at the given path, atomically acquiring a lock. On BSD,
     // we can use O_EXLOCK. On Linux, we open the file, take a lock, and then compare fstat() to
     // stat(); if they match, it means that the file was not replaced before we acquired the lock.
@@ -589,11 +588,11 @@ bool env_universal_t::open_and_acquire_lock(const std::string &path, int *out_fd
     }
 #endif
 
-    int fd = -1;
-    while (fd == -1) {
+    autoclose_fd_t fd{};
+    while (!fd.valid()) {
         double start_time = timef();
-        fd = open_cloexec(path, flags, 0644);
-        if (fd == -1) {
+        fd = autoclose_fd_t{open_cloexec(path, flags, 0644)};
+        if (!fd.valid()) {
             if (errno == EINTR) continue;  // signaled; try again
 #ifdef O_EXLOCK
             if (do_locking && (errno == ENOTSUP || errno == EOPNOTSUPP)) {
@@ -611,7 +610,7 @@ bool env_universal_t::open_and_acquire_lock(const std::string &path, int *out_fd
             break;
         }
 
-        assert(fd >= 0);  // if we get here, we must have a valid fd
+        assert(fd.valid() && "Should have a valid fd here");
         if (!needs_lock && do_locking) {
             do_locking = check_duration(start_time);
         }
@@ -619,20 +618,19 @@ bool env_universal_t::open_and_acquire_lock(const std::string &path, int *out_fd
         // Try taking the lock, if necessary. If we failed, we may be on lockless NFS, etc.; in that
         // case we pretend we succeeded. See the comment in save_to_path for the rationale.
         if (needs_lock && do_locking) {
-            do_locking = lock_uvar_file(fd);
+            do_locking = lock_uvar_file(fd.fd());
         }
 
         // Hopefully we got the lock. However, it's possible the file changed out from under us
         // while we were waiting for the lock. Make sure that didn't happen.
-        if (file_id_for_fd(fd) != file_id_for_path(path)) {
+        if (file_id_for_fd(fd.fd()) != file_id_for_path(path)) {
             // Oops, it changed! Try again.
-            close(fd);
-            fd = -1;
+            fd.close();
         }
     }
 
-    *out_fd = fd;
-    return fd >= 0;
+    *out_fd = std::move(fd);
+    return out_fd->valid();
 }
 
 // Returns true if modified variables were written, false if not. (There may still be variable
@@ -692,7 +690,7 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) {
 
     const wcstring directory = wdirname(explicit_vars_path);
     bool success = true;
-    int vars_fd = -1;
+    autoclose_fd_t vars_fd{};
 
     FLOGF(uvar_file, L"universal log performing full sync");
 
@@ -704,19 +702,13 @@ bool env_universal_t::sync(callback_data_list_t &callbacks) {
 
     // Read from it.
     if (success) {
-        assert(vars_fd >= 0);
-        this->load_from_fd(vars_fd, callbacks);
+        assert(vars_fd.valid());
+        this->load_from_fd(vars_fd.fd(), callbacks);
     }
 
     if (success && ok_to_save) {
         success = this->save(directory, explicit_vars_path);
     }
-
-    // Clean up.
-    if (vars_fd >= 0) {
-        close(vars_fd);
-    }
-
     return success;
 }
 
diff --git a/src/env_universal_common.h b/src/env_universal_common.h
index ae4bd3691..f7441e9dd 100644
--- a/src/env_universal_common.h
+++ b/src/env_universal_common.h
@@ -65,7 +65,7 @@ class env_universal_t {
     bool remove_internal(const wcstring &key);
 
     // Functions concerned with saving.
-    bool open_and_acquire_lock(const std::string &path, int *out_fd);
+    bool open_and_acquire_lock(const std::string &path, autoclose_fd_t *out_fd);
     bool open_temporary_file(const wcstring &directory, wcstring *out_path, int *out_fd);
     bool write_to_fd(int fd, const wcstring &path);
     bool move_new_vars_file_into_place(const wcstring &src, const wcstring &dst);
diff --git a/src/exec.cpp b/src/exec.cpp
index c20c32cd2..5273adaf4 100644
--- a/src/exec.cpp
+++ b/src/exec.cpp
@@ -49,23 +49,9 @@
 #include "trace.h"
 #include "wutil.h"  // IWYU pragma: keep
 
-/// File descriptor redirection error message.
-#define FD_ERROR _(L"An error occurred while redirecting file descriptor %d")
-
 /// Number of calls to fork() or posix_spawn().
 static relaxed_atomic_t<int> s_fork_count{0};
 
-void exec_close(int fd) {
-    assert(fd >= 0 && "Invalid fd");
-    while (close(fd) == -1) {
-        if (errno != EINTR) {
-            FLOGF(warning, FD_ERROR, fd);
-            wperror(L"close");
-            break;
-        }
-    }
-}
-
 /// This function is executed by the child process created by a call to fork(). It should be called
 /// after \c child_setup_process. It calls execve to replace the fish process image with the command
 /// specified in \c p. It never returns. Called in a forked child! Do not allocate memory, etc.