fix(shada): repair damaged shada file, write atomically #42618

Problem:
A truncated shada file (crash, or conflicting Nvim sibling) fails with
E576 on every startup and exit, forever: the merge treats it as "not
a shada file" and refuses to replace it. And when main.shada is missing
or unreadable, or with `:wshada!`, shada is written in-place, not
atomically.

Solution:
- If a valid shada _header_ is found, we know it is a (damaged) shada
  file, so we shouldn't just throw away the entire thing. Keep the
  entries gathered so far and replace the file.
- Always write a temp file and rename it.
- Flush (and fsync) the temp file before renaming it. Check the final
  flush.

runtime/doc/news.txt

@@ -510,6 +510,8 @@ STARTUP
   dismissed instead of waiting (indefinitely).
 • |--embed| with |--listen| waits for a UI on the listen address if stdin is
   not a pipe (e.g. tty or /dev/null), instead of exiting.
+• A damaged |shada| file (e.g. truncated by a crash) is repaired on the next
+  write and keeps any valid entries. |shada-error-handling|
 
 TERMINAL
 

runtime/doc/starting.txt

@@ -1148,7 +1148,9 @@ include:
   non-ShaDa files are not overwritten for safety reasons to avoid accidentally
   destroying an unrelated file.  This could happen e.g. when typing "nvim -i
   file" in place of "nvim -R file" (yes, somebody did that at least with Vim).
-  Such errors are listed at |shada-critical-contents-errors|.
+  Such errors are listed at |shada-critical-contents-errors|.  If they occur
+  after a valid ShaDa header (e.g. file was truncated by a crash), Nvim keeps
+  the entries before the damage and replaces the file.
 - If writing to the temporary file failed: e.g. because of the insufficient
   space left.
 - If renaming file failed: e.g. because of insufficient permissions.

runtime/doc/vim_diff.txt

@@ -622,6 +622,8 @@ Additional differences:
 - Some errors make ShaDa code keep temporary file in-place for user to decide
   what to do with it.  Vim deletes temporary file in these cases.
   |shada-error-handling|
+- A damaged ShaDa file (e.g. truncated by a crash) is repaired on the next
+  write and keeps any valid entries.  |shada-error-handling|
 - ShaDa file keeps search direction (|v:searchforward|), viminfo does not.
 
 |printf()| returns something meaningful when used with `%p` argument: in Vim

src/nvim/os/fileio.c

@@ -144,7 +144,7 @@ void file_open_buffer(FileDescriptor *ret_fp, char *data, size_t len)
   ret_fp->bytes_read = 0;
 }
 
-/// Close file and free its buffer
+/// Close file and free its buffer. Idempotent: closing it again does nothing.
 ///
 /// @param[in,out]  fp  File to close.
 /// @param[in]  do_fsync  If true, use fsync() to write changes to disk.
@@ -159,6 +159,7 @@ int file_close(FileDescriptor *const fp, const bool do_fsync)
 
   const int flush_error = (do_fsync ? file_fsync(fp) : file_flush(fp));
   const int close_error = os_close(fp->fd);
+  fp->fd = -1;  // No-op if called again.
   free_block(fp->buffer);
   if (close_error != 0) {
     return close_error;

src/nvim/shada.c

@@ -558,13 +558,14 @@ static ShaDaReadResult sd_reader_skip(FileDescriptor *const sd_reader, const siz
 }
 
 /// Wrapper for closing file descriptors
-static void close_file(FileDescriptor *cookie)
+static int close_file(FileDescriptor *cookie)
 {
   const int error = file_close(cookie, !!p_fs);
   if (error != 0) {
     semsg(_(SERR "System error while closing ShaDa file: %s"),
           os_strerror(error));
   }
+  return error;
 }
 
 /// Read ShaDa file
@@ -1731,6 +1732,7 @@ static inline ShaDaWriteResult shada_read_when_writing(FileDescriptor *const sd_
   ShaDaWriteResult ret = kSDWriteSuccessful;
   ShadaEntry entry;
   ShaDaReadResult srni_ret;
+  bool has_header = false;
 
 #define COMPARE_WITH_ENTRY(wms_entry_, entry) \
   do { \
@@ -1755,8 +1757,9 @@ static inline ShaDaWriteResult shada_read_when_writing(FileDescriptor *const sd_
       // Should be handled by the while condition.
       abort();
     case kSDReadStatusNotShaDa:
-      ret = kSDWriteReadNotShada;
-      FALLTHROUGH;
+      // Damage after a valid header (e.g. truncated by a crash), so we know it is a (partial) ShaDa
+      // file. Keep the entries gathered so far and replace the file. #6875 #11876
+      return has_header ? ret : kSDWriteReadNotShada;
     case kSDReadStatusReadError:
       return ret;
     case kSDReadStatusMalformed:
@@ -1766,6 +1769,9 @@ static inline ShaDaWriteResult shada_read_when_writing(FileDescriptor *const sd_
     case kSDItemMissing:
       break;
     case kSDItemHeader:
+      has_header = true;
+      shada_free_shada_entry(&entry);
+      break;
     case kSDItemBufferList:
       abort();
     case kSDItemUnknown:
@@ -2279,7 +2285,8 @@ static ShaDaWriteResult shada_write(FileDescriptor *const sd_writer,
     }
   }
 
-  const unsigned srni_flags = (unsigned)(kSDReadUndisableableData
+  const unsigned srni_flags = (unsigned)(kSDReadHeader
+                                         | kSDReadUndisableableData
                                          | kSDReadUnknown
                                          | (dump_history ? kSDReadHistory : 0)
                                          | (dump_registers ? kSDReadRegisters : 0)
@@ -2686,6 +2693,9 @@ shada_write_exit:
   map_destroy(cstr_t, &wms->file_marks);
   set_destroy(ptr_t, &removable_bufs);
   packer.packer_flush(&packer);
+  if (packer.anyint != 0) {
+    ret = kSDWriteFailed;
+  }
   set_destroy(cstr_t, &wms->dumped_variables);
   xfree(wms);
   return ret;
@@ -2755,71 +2765,36 @@ int shada_write_file(const char *const file, bool nomerge)
     return FAIL;
   }
 
-  char *tempname = NULL;
   FileDescriptor sd_writer;
   FileDescriptor sd_reader;
-  bool did_open_writer = false;
   bool did_open_reader = false;
 
   if (!nomerge) {
-    int error;
-    if ((error = file_open(&sd_reader, fname, kFileReadOnly, 0)) != 0) {
-      if (error != UV_ENOENT) {
-        semsg(_(SERR "System error while opening ShaDa file %s for reading "
-                "to merge before writing it: %s"),
-              fname, os_strerror(error));
-        // Try writing the file even if opening it emerged any issues besides
-        // file not existing: maybe writing will succeed nevertheless.
-      }
-      nomerge = true;
-      goto shada_write_file_nomerge;
-    } else {
+    const int error = file_open(&sd_reader, fname, kFileReadOnly, 0);
+    if (error == 0) {
       did_open_reader = true;
-    }
-
-    // Save permissions from the original file, with modifications:
-    int perm = (int)os_getperm(fname);
-    perm = (perm >= 0) ? ((perm & 0777) | 0600) : 0600;
-    //                 ^3         ^1       ^2      ^2,3
-    // 1: Strip SUID bit if any.
-    // 2: Make sure that user can always read and write the result.
-    // 3: If somebody happened to delete the file after it was opened for
-    //    reading use u=rw permissions.
-    tempname = shada_open_tmp(fname, &sd_writer, perm);
-    did_open_writer = tempname != NULL;
-  }
-  if (nomerge) {
-shada_write_file_nomerge: {}
-    char *const tail = path_tail_with_sep(fname);
-    if (tail != fname) {
-      const char tail_save = *tail;
-      *tail = NUL;
-      if (!os_isdir(fname)) {
-        int ret;
-        char *failed_dir;
-        if ((ret = os_mkdir_recurse(fname, 0700, &failed_dir, NULL)) != 0) {
-          semsg(_(SERR "Failed to create directory %s "
-                  "for writing ShaDa file: %s"),
-                failed_dir, os_strerror(ret));
-          xfree(fname);
-          xfree(failed_dir);
-          return FAIL;
-        }
-      }
-      *tail = tail_save;
-    }
-    int error = file_open(&sd_writer, fname, kFileCreate|kFileTruncate, 0600);
-    if (error) {
-      semsg(_(SERR "System error while opening ShaDa file %s for writing: %s"),
+    } else if (error != UV_ENOENT) {
+      semsg(_(SERR "System error while opening ShaDa file %s for reading "
+              "to merge before writing it: %s"),
             fname, os_strerror(error));
-    } else {
-      did_open_writer = true;
+      // Try writing the file even if opening it emerged any issues besides
+      // file not existing: maybe writing will succeed nevertheless.
     }
   }
 
-  if (!did_open_writer) {
+  // Save permissions from the original file, with modifications:
+  int perm = (int)os_getperm(fname);
+  perm = (perm >= 0) ? ((perm & 0777) | 0600) : 0600;
+  //                 ^3         ^1       ^2      ^2,3
+  // 1: Strip SUID bit if any.
+  // 2: Make sure that user can always read and write the result.
+  // 3: If the file does not exist use u=rw permissions.
+  char *tempname = NULL;
+  if (os_file_mkdir(fname, 0700) == 0) {
+    tempname = shada_open_tmp(fname, &sd_writer, perm);
+  }
+  if (tempname == NULL) {
     xfree(fname);
-    xfree(tempname);
     if (did_open_reader) {
       close_file(&sd_reader);
     }
@@ -2832,17 +2807,16 @@ shada_write_file_nomerge: {}
     verbose_leave();
   }
 
-  const ShaDaWriteResult sw_ret = shada_write(&sd_writer, (nomerge ? NULL : &sd_reader));
+  const ShaDaWriteResult sw_ret = shada_write(&sd_writer, did_open_reader ? &sd_reader : NULL);
   assert(sw_ret != kSDWriteIgnError);
-  if (!nomerge) {
-    if (did_open_reader) {
-      close_file(&sd_reader);
-    }
-    bool did_remove = false;
-    if (sw_ret == kSDWriteSuccessful) {
-      FileInfo old_info;
-      if (!os_fileinfo(fname, &old_info)
-          || S_ISDIR(old_info.stat.st_mode)
+  if (did_open_reader) {
+    close_file(&sd_reader);
+  }
+  bool did_remove = false;
+  if (sw_ret == kSDWriteSuccessful) {
+    FileInfo old_info;
+    if (os_fileinfo(fname, &old_info)) {
+      if (S_ISDIR(old_info.stat.st_mode)
 #ifdef UNIX
           // For Unix we check the owner of the file.  It's not very nice
           // to overwrite a user's viminfo file after a "su root", with a
@@ -2874,29 +2848,33 @@ shada_write_file_nomerge: {}
         }
       }
 #endif
-      if (vim_rename(tempname, fname) == -1) {
-        semsg(_(RNERR "Can't rename ShaDa file from %s to %s!"),
-              tempname, fname);
-      } else {
-        did_remove = true;
-        os_remove(tempname);
-      }
-    } else {
-      if (sw_ret == kSDWriteReadNotShada) {
-        semsg(_(RNERR "Did not rename %s because %s "
-                "does not look like a ShaDa file"), tempname, fname);
-      } else {
-        semsg(_(RNERR "Did not rename %s to %s because there were errors "
-                "during writing it"), tempname, fname);
-      }
     }
-    if (!did_remove) {
-shada_write_file_did_not_remove:
-      semsg(_(RNERR "Do not forget to remove %s or rename it manually to %s."),
+    // Flush (+fsync) before rename, so a crash cannot partially replace the old file.
+    if (close_file(&sd_writer) != 0) {
+      goto shada_write_file_did_not_remove;
+    }
+    if (vim_rename(tempname, fname) == -1) {
+      semsg(_(RNERR "Can't rename ShaDa file from %s to %s!"),
             tempname, fname);
+    } else {
+      did_remove = true;
+      os_remove(tempname);
+    }
+  } else {
+    if (sw_ret == kSDWriteReadNotShada) {
+      semsg(_(RNERR "Did not rename %s because %s "
+              "does not look like a ShaDa file"), tempname, fname);
+    } else {
+      semsg(_(RNERR "Did not rename %s to %s because there were errors "
+              "during writing it"), tempname, fname);
     }
-    xfree(tempname);
   }
+  if (!did_remove) {
+shada_write_file_did_not_remove:
+    semsg(_(RNERR "Do not forget to remove %s or rename it manually to %s."),
+          tempname, fname);
+  }
+  xfree(tempname);
   close_file(&sd_writer);
 
   xfree(fname);
@@ -3258,9 +3236,11 @@ shada_read_next_item_start:
   entry->data = sd_default_values[type_u64].data;
   switch ((ShadaEntryType)type_u64) {
   case kSDItemHeader:
-    // TODO(bfredl): header is written to file and provides useful debugging
-    // info. It is never read by nvim (earlier we parsed it back to a
-    // Dict, but that value was never used)
+    // Check that header is well-formed. shada_read_when_writing() uses it to detect "damaged ShaDa"
+    // vs "non-ShaDa" file.
+    if (unpack_skip(&read_ptr, &read_size) != MPACK_OK) {
+      goto shada_read_next_item_error;
+    }
     break;
   case kSDItemSearchPattern: {
     Dict(_shada_search_pat) *it = &entry->data.search_pattern;

test/functional/shada/errors_spec.lua

@@ -667,17 +667,21 @@ $
     nvim_command('wshada! ' .. shada_fname)
   end)
 
-  it('fails on invalid ShaDa file (failing skip in second item)', function()
-    wshada('\001\000\001\128#!/')
+  it('replaces ShaDa file damaged after the header, keeps valid entries', function()
+    -- Header, history entry, truncated entry.
+    wshada('\001\000\001\128\004\000\008\146\000\196\004kept#!/')
     eq(
       'Vim(rshada):E576: Reading ShaDa file: last entry specified that it occupies 47 bytes, but file ended earlier',
       t.pcall_err(nvim_command, sdrcmd())
     )
+    n.fn.histdel(':')
     eq(
       'Vim(wshada):E576: Error while reading ShaDa file: last entry specified that it occupies 47 bytes, but file ended earlier',
       t.pcall_err(nvim_command, 'wshada ' .. shada_fname)
     )
-    nvim_command('wshada! ' .. shada_fname)
+    eq(0, n.fn.filereadable(shada_fname .. '.tmp.a'))
+    nvim_command(sdrcmd())
+    eq('kept', n.fn.histget(':', -1))
   end)
 
   it('errors with too large items', function()