Implement atomic loading/saving of fishd file within fish, eventually

permitting removal of fishd. Universal variables test passes, others
fail.

1 files changed, 318 insertions, 63 deletions

diff --git a/env_universal_common.cpp b/env_universal_common.cpp
index f3cc6cf0..f7fa394f 100644
--- a/env_universal_common.cpp
+++ b/env_universal_common.cpp
@@ -89,10 +89,32 @@
 */
 #define ENV_UNIVERSAL_EOF   0x102
 
+/** Small note about not editing ~/.fishd manually. Inserted at the top of all .fishd files. */
+#define SAVE_MSG "# This file is automatically generated by the fish.\n# Do NOT edit it directly, your changes will be overwritten.\n"
+
+static wcstring fishd_get_config();
+static std::string get_variables_file_path(const std::string &dir, const std::string &identifier);
+
+static wcstring default_vars_path()
+{
+    wcstring wdir = fishd_get_config();
+    const std::string dir = wcs2string(wdir);
+    if (dir.empty())
+        return L"";
+    
+    const std::string machine_id = get_machine_identifier();
+    const std::string machine_id_path = get_variables_file_path(dir, machine_id);
+    return str2wcstring(machine_id_path);
+}
+
 /**
    The table of all universal variables
 */
-static env_universal_t s_env_universal_var;
+static env_universal_t &default_universal_vars()
+{
+    static env_universal_t s_default_vars(L"");
+    return s_default_vars;
+}
 
 /**
    Callback function, should be called on all events
@@ -131,7 +153,7 @@ void env_universal_common_init(void (*cb)(fish_message_type_t type, const wchar_
 
 void read_message(connection_t *conn)
 {
-    return s_env_universal_var.read_message(conn);
+    return default_universal_vars().read_message(conn);
 }
 
 /**
@@ -181,7 +203,7 @@ static int read_byte(connection_t *src)
 */
 void env_universal_common_remove(const wcstring &name)
 {
-    s_env_universal_var.remove(name);
+    default_universal_vars().remove(name);
 }
 
 /**
@@ -204,7 +226,7 @@ void env_universal_common_set(const wchar_t *key, const wchar_t *val, bool expor
     CHECK(key,);
     CHECK(val,);
     
-    s_env_universal_var.set(key, val, exportv);
+    default_universal_vars().set(key, val, exportv);
 
     if (callback)
     {
@@ -292,7 +314,7 @@ static bool is_universal_safe_to_encode_directly(wchar_t c)
     if (c < 32 || c > 128)
         return false;
 
-    return iswalnum(c) || wcschr(L"/", c);
+    return iswalnum(c) || wcschr(L"/_", c);
 }
 
 /**
@@ -430,25 +452,24 @@ message_t *create_message(fish_message_type_t type,
 */
 void env_universal_common_get_names(wcstring_list_t &lst, bool show_exported, bool show_unexported)
 {
-    wcstring_list_t names = s_env_universal_var.get_names(show_exported, show_unexported);
+    wcstring_list_t names = default_universal_vars().get_names(show_exported, show_unexported);
     lst.insert(lst.end(), names.begin(), names.end());
 }
 
 
 env_var_t env_universal_common_get(const wcstring &name)
 {
-    return s_env_universal_var.get(name);
+    return default_universal_vars().get(name);
 }
 
 bool env_universal_common_get_export(const wcstring &name)
 {
-    return s_env_universal_var.get_export(name);
+    return default_universal_vars().get_export(name);
 }
 
 void enqueue_all(connection_t *c)
 {
-    s_env_universal_var.enqueue_all(c);
-    try_send_all(c);
+    default_universal_vars().enqueue_all(c);
 }
 
 connection_t::connection_t(int input_fd) :
@@ -473,7 +494,7 @@ void connection_destroy(connection_t *c)
     }
 }
 
-env_universal_t::env_universal_t() : tried_renaming(false)
+env_universal_t::env_universal_t(const wcstring &path) : explicit_vars_path(path), tried_renaming(false), last_read_file(kInvalidFileID)
 {
     VOMIT_ON_FAILURE(pthread_mutex_init(&lock, NULL));
 }
@@ -506,30 +527,51 @@ bool env_universal_t::get_export(const wcstring &name) const
 }
 
 
-void env_universal_t::set_internal(const wcstring &key, const wcstring &val, bool exportv)
+void env_universal_t::set_internal(const wcstring &key, const wcstring &val, bool exportv, bool overwrite)
 {
     ASSERT_IS_LOCKED(lock);
+    if (! overwrite && this->modified.find(key) != this->modified.end())
+    {
+        /* This value has been modified and we're not overwriting it. Skip it. */
+        return;
+    }
+    
     var_entry_t *entry = &vars[key];
     entry->val = val;
     entry->exportv = exportv;
+    
+    /* If we are overwriting, then this is now modified */
+    if (overwrite)
+    {
+        this->modified.insert(key);
+    }
 }
 
 void env_universal_t::set(const wcstring &key, const wcstring &val, bool exportv)
 {
     scoped_lock locker(lock);
-    this->set_internal(key, val, exportv);
+    this->set_internal(key, val, exportv, true /* overwrite */);
 }
 
-void env_universal_t::remove_internal(const wcstring &key)
+void env_universal_t::remove_internal(const wcstring &key, bool overwrite)
 {
-    scoped_lock locker(lock);
-    vars.erase(key);
+    ASSERT_IS_LOCKED(lock);
+    if (! overwrite && this->modified.find(key) != modified.end())
+    {
+        /* This value has been modified and we're not overwriting it. Skip it. */
+        return;
+    }
+    this->vars.erase(key);
+    if (overwrite)
+    {
+        this->modified.insert(key);
+    }
 }
 
 void env_universal_t::remove(const wcstring &key)
 {
-    ASSERT_IS_LOCKED(lock);
-    this->remove(key);
+    scoped_lock locker(lock);
+    this->remove_internal(key, true);
 }
 
 wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexported) const
@@ -549,9 +591,9 @@ wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexpor
     return result;
 }
 
-void env_universal_t::enqueue_all(connection_t *c) const
+void env_universal_t::enqueue_all_internal(connection_t *c) const
 {
-    scoped_lock locker(lock);
+    ASSERT_IS_LOCKED(lock);
     var_table_t::const_iterator iter;
     for (iter = vars.begin(); iter != vars.end(); ++iter)
     {
@@ -562,6 +604,28 @@ void env_universal_t::enqueue_all(connection_t *c) const
         msg->count=1;
         c->unsent.push(msg);
     }
+    try_send_all(c);
+}
+
+void env_universal_t::enqueue_all(connection_t *c) const
+{
+    scoped_lock locker(lock);
+    enqueue_all_internal(c);
+}
+
+void env_universal_t::load_from_fd(int fd)
+{
+    ASSERT_IS_LOCKED(lock);
+    assert(fd >= 0);
+    /* Get the dev / inode */
+    file_id_t current_file = file_id_for_fd(fd);
+    if (current_file != last_read_file)
+    {
+        connection_t c(fd);
+        /* Read from the file. Do not destroy the connection; the caller is responsible for closing the fd. */
+        this->read_message_internal(&c);
+        last_read_file = current_file;
+    }
 }
 
 bool env_universal_t::load_from_path(const wcstring &path)
@@ -569,18 +633,38 @@ bool env_universal_t::load_from_path(const wcstring &path)
     ASSERT_IS_LOCKED(lock);
     /* OK to not use CLO_EXEC here because fishd is single threaded */
     bool result = false;
-    int fd = wopen_cloexec(path, O_RDONLY | O_CLOEXEC, 0600);
+    int fd = wopen_cloexec(path, O_RDONLY);
     if (fd >= 0)
     {
-        /* Success */
+        this->load_from_fd(fd);
+        close(fd);
         result = true;
-        connection_t c(fd);
-        /* Read from the file */
-        this->read_message(&c);
-        connection_destroy(&c);
     }
     return result;
+}
+
+void env_universal_t::write_to_fd(int fd)
+{
+    ASSERT_IS_LOCKED(lock);
+    assert(fd >= 0);
+    connection_t conn(fd);
+    write_loop(fd, SAVE_MSG, strlen(SAVE_MSG));
+    this->enqueue_all_internal(&conn);
+    
+    /* Since we just wrote out this file, it matches our internal state; pretend we read from it */
+    this->last_read_file = file_id_for_fd(fd);
+    
+    /* Do not destroy the connection; we don't close the file */
+}
 
+bool env_universal_t::move_new_vars_file_into_place(const wcstring &src, const wcstring &dst)
+{
+    int ret = wrename(src, dst);
+    if (ret != 0)
+    {
+        wperror(L"rename");
+    }
+    return ret == 0;
 }
 
 /**
@@ -639,62 +723,232 @@ static wcstring fishd_get_config()
     return result;
 }
 
-static std::string get_variables_file_path(const std::string &dir, const std::string &identifier);
 bool env_universal_t::load()
 {
     scoped_lock locker(lock);
-    wcstring wdir = fishd_get_config();
-    const std::string dir = wcs2string(wdir);
-    if (dir.empty())
-        return false;
-    
-    const std::string machine_id = get_machine_identifier();
-    const std::string machine_id_path = get_variables_file_path(dir, machine_id);
-    
-    const wcstring path = str2wcstring(machine_id_path);
-    bool success = load_from_path(path);
+    const wcstring vars_path = explicit_vars_path.empty() ? default_vars_path() : explicit_vars_path;
+    bool success = load_from_path(vars_path);
     if (! success && ! tried_renaming && errno == ENOENT)
     {
         /* We failed to load, because the file was not found. Older fish used the hostname only. Try *moving* the filename based on the hostname into place; if that succeeds try again. Silently "upgraded." */
         tried_renaming = true;
         std::string hostname_id;
-        if (get_hostname_identifier(&hostname_id) && hostname_id != machine_id)
+        if (get_hostname_identifier(&hostname_id))
         {
-            std::string hostname_path = get_variables_file_path(dir, hostname_id);
-            if (0 == rename(hostname_path.c_str(), machine_id_path.c_str()))
+            const wcstring hostname_path = wdirname(vars_path) + L'/' + str2wcstring(hostname_id);
+            if (0 == wrename(hostname_path, vars_path))
             {
                 /* We renamed - try again */
-                success = load_from_path(path);
+                success = this->load();
             }
         }
     }
     return success;
 }
 
-bool env_universal_t::save()
+bool env_universal_t::open_temporary_file(const wcstring &directory, wcstring *out_path, int *out_fd)
 {
-    /* Writes variables */
-    scoped_lock locker(lock);
-    wcstring wdir = fishd_get_config();
-    const std::string dir = wcs2string(wdir);
-    if (dir.empty())
-        return false;
+    /* Create and open a temporary file for writing within the given directory */
+    /* Try to create a temporary file, up to 10 times. We don't use mkstemps because we want to open it CLO_EXEC. This should almost always succeed on the first try. */
+    assert(! string_suffixes_string(L"/", directory));
     
-    const std::string machine_id = get_machine_identifier();
-    const std::string machine_id_path = get_variables_file_path(dir, machine_id);
-    
-    const wcstring path = str2wcstring(machine_id_path);
-    return save_to_path(path);
+    bool success = false;
+    const wcstring tmp_name_template = directory + L"/fishd.tmp.XXXXXX";
+    wcstring tmp_name;
+    for (size_t attempt = 0; attempt < 10 && ! success; attempt++)
+    {
+        int result_fd = -1;
+        char *narrow_str = wcs2str(tmp_name_template.c_str());
+        if (narrow_str && mktemp(narrow_str))
+        {
+            /* It was successfully templated; try opening it atomically */
+            tmp_name = str2wcstring(narrow_str);
+            result_fd = wopen_cloexec(tmp_name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, 0644);
+        }
+        
+        if (result_fd >= 0)
+        {
+            /* Success */
+            *out_fd = result_fd;
+            *out_path = str2wcstring(narrow_str);
+            success = true;
+        }
+        free(narrow_str);
+    }
+    if (! success)
+    {
+        wperror(L"open");
+    }
+    return success;
 }
 
-bool env_universal_t::save_to_path(const wcstring &path)
+bool env_universal_t::open_and_acquire_lock(const wcstring &path, int *out_fd)
 {
-    return false;
+    /* 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.
+    
+    We pass O_RDONLY with O_CREAT; this creates a potentially empty file. We do this so that we have something to lock on.
+    */
+    int result_fd = -1;
+    bool needs_lock = true;
+    int flags = O_RDONLY | O_CREAT;
+#ifdef O_EXLOCK
+    flags |= O_EXLOCK;
+    needs_lock = false;
+#endif
+    for (;;)
+    {
+        int fd = wopen_cloexec(path, flags, 0644);
+        if (fd < 0)
+        {
+            int err = errno;
+            if (err == EINTR)
+            {
+                /* Signal; try again */
+                continue;
+            }
+#ifdef O_EXLOCK
+            else if (err == EOPNOTSUPP)
+            {
+                /* Filesystem probably does not support locking. Clear the flag and try again. Note that we try taking the lock via flock anyways. */
+                flags &= ~O_EXLOCK;
+                needs_lock = true;
+                continue;
+            }
+#endif
+            else
+            {
+                wperror(L"open");
+                break;
+            }
+        }
+        
+        /* If we get here, we must have a valid fd */
+        assert(fd >= 0);
+        
+        /* 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)
+        {
+            while (flock(fd, LOCK_EX) < 0)
+            {
+                /* error */
+                if (errno != EINTR)
+                {
+                    wperror(L"flock");
+                    break;
+                }
+            }
+        }
+        
+        /* 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))
+        {
+            /* Oops, it changed! Try again */
+            close(fd);
+            continue;
+        }
+
+        
+        /* Finally, we have an fd that's valid and hopefully locked. We're done */
+        assert(fd >= 0);
+        result_fd = fd;
+        break;
+    }
+    
+    *out_fd = result_fd;
+    
+    return result_fd >= 0;
 }
 
-void env_universal_t::read_message(connection_t *src)
+bool env_universal_t::save()
 {
     scoped_lock locker(lock);
+    /* Our saving strategy:
+    
+    1. Open the file, producing an fd.
+    2. Lock the file (may be combined with step 1 on systems with O_EXLOCK)
+    3. After taking the lock, check if the file at the given path is different from what we opened. If so, start over.
+    4. Read from the file. This can be elided if its dev/inode is unchanged since the last read
+    5. Open an adjacent temporary file
+    6. Write our changes to an adjacent file
+    7. Move the adjacent file into place via rename. This is assumed to be atomic.
+    8. Release the lock and close the file
+    
+    Consider what happens if Process 1 and 2 both do this simultaneously. Can there be data loss? Process 1 opens the file and then attempts to take the lock. Now, either process 1 will see the original file, or process 2's new file. If it sees the new file, we're OK: it's going to read from the new file, and so there's no data loss. If it sees the old file, then process 2 must have locked it (if process 1 locks it, switch their roles). The lock will block until process 2 reaches step 7; at that point process 1 will reach step 2, notice that the file has changed, and then start over.
+    
+    It's possible that the underlying filesystem does not support locks (lockless NFS). In this case, we risk data loss if two shells try to write their universal variables simultaneously. In practice this is unlikely, since uvars are usually written interactively.
+    
+    Prior versions of fish used a hard link scheme to support file locking on lockless NFS. The risk here is that if the process crashes or is killed while holding the lock, future instances of fish will not be able to obtain it. This seems to be a greater risk than that of data loss on lockless NFS. Users who put their home directory on lockless NFS are playing with fire anyways.
+    
+    It's worth discussing error handling on the initial open (#1):
+       File doesn't exist: attempt to create an empty file, then repeat
+       Permission denied / other errors: log to the console (once) and then give up
+    */
+    const wcstring vars_path = explicit_vars_path.empty() ? default_vars_path() : explicit_vars_path;
+    const wcstring directory = wdirname(vars_path);
+    bool success = false;
+    int vars_fd = -1;
+    int private_fd = -1;
+    wcstring private_file_path;
+    do
+    {
+        /* Open the file */
+        if (! this->open_and_acquire_lock(vars_path, &vars_fd))
+        {
+            break;
+        }
+        
+        /* Read from it */
+        assert(vars_fd >= 0);
+        this->load_from_fd(vars_fd);
+        
+        /* Open adjacent temporary file */
+        if (! this->open_temporary_file(directory, &private_file_path, &private_fd))
+        {
+            break;
+        }
+        
+        /* Write to it */
+        assert(private_fd >= 0);
+        this->write_to_fd(private_fd);
+        
+        /* Apply new file */
+        if (! this->move_new_vars_file_into_place(private_file_path, vars_path))
+        {
+            break;
+        }
+        
+        /* Since we moved the new file into place, clear the path so we don't try to unlink it */
+        private_file_path.clear();
+        success = true;
+        break;
+    } while (false);
+    
+    /* Clean up */
+    if (vars_fd >= 0)
+    {
+        close(vars_fd);
+    }
+    if (private_fd >= 0)
+    {
+        close(private_fd);
+    }
+    if (! private_file_path.empty())
+    {
+        wunlink(private_file_path);
+    }
+    
+    if (success)
+    {
+        /* All of our modified variables have now been written out. */
+        modified.clear();
+    }
+    
+    return success;
+}
+
+void env_universal_t::read_message_internal(connection_t *src)
+{
+    ASSERT_IS_LOCKED(lock);
     while (1)
     {
         
@@ -770,6 +1024,12 @@ void env_universal_t::read_message(connection_t *src)
     }
 }
 
+void env_universal_t::read_message(connection_t *src)
+{
+    scoped_lock locker(lock);
+    return read_message_internal(src);
+}
+
 /**
  Parse message msg
  */
@@ -799,7 +1059,7 @@ void env_universal_t::parse_message_internal(wchar_t *msg, connection_t *src)
             wcstring val;
             if (unescape_string(tmp + 1, &val, 0))
             {
-                this->set_internal(key, val, exportv);
+                this->set_internal(key, val, exportv, false);
             }
         }
         else
@@ -826,7 +1086,7 @@ void env_universal_t::parse_message_internal(wchar_t *msg, connection_t *src)
             debug(1, PARSE_ERR, msg);
         }
         
-        this->remove_internal(name);
+        this->remove_internal(name, false);
         
 #warning We're locked when this is invoked - bad news!
         if (callback)
@@ -866,11 +1126,6 @@ static std::string get_variables_file_path(const std::string &dir, const std::st
     return name;
 }
 
-/**
- Small not about not editing ~/.fishd manually. Inserted at the top of all .fishd files.
- */
-#define SAVE_MSG "# This file is automatically generated by the fish.\n# Do NOT edit it directly, your changes will be overwritten.\n"
-
 static bool load_or_save_variables_at_path(bool save, const std::string &path)
 {
     bool result = false;