save: write states, SRAM and disk index through a temporary file (#19629)

content.h

@@ -46,6 +46,11 @@ bool content_load_state_from_ram(void);
 /* Save a state to memory. */
 bool content_save_state_to_ram(void);
 
+/* Finishes a save written to filestream_atomic_temp_name(@path): moved
+ * over @path if @ok, deleted otherwise. A temporary file that could not
+ * be moved is kept only when it holds the one complete copy left. */
+bool content_finish_tmp(const char *path, bool ok);
+
 /* Save a ram state from memory to disk. */
 bool content_ram_state_to_file(const char *path);
 

disk_index_file.c

@@ -429,7 +429,9 @@ bool disk_index_file_save(disk_index_file_t *disk_index_file)
    /* NULL means the writer hit an error while serialising */
    buf = rjsonwriter_get_memory_buffer(writer, &_len);
 
-   if (!buf || !filestream_write_file(file_path, buf, _len))
+   /* Atomic write: a process killed mid-save would otherwise leave a
+    * truncated disk index, which makes multi-disc games revert to disc 1. */
+   if (!buf || !filestream_write_file_atomic(file_path, buf, _len))
    {
       RARCH_ERR("[Disk index file] Error writing disk index file: \"%s\".\n", file_path);
       rjsonwriter_free(writer);

libretro-common/include/streams/file_stream.h

@@ -337,6 +337,28 @@ RFILE *filestream_open_atomic(const char *path);
  */
 int filestream_commit_atomic(RFILE *file, const char *path, bool ok);
 
+/**
+ * The name of the sibling temporary file an atomic write of \c path
+ * goes through, for a writer that opens files by name.
+ *
+ * @param s Where the name is written.
+ * @param len The size of \c s.
+ * @param path Path of the file that will be replaced.
+ * @return \c false if the name does not fit in \c s.
+ */
+bool filestream_atomic_temp_name(char *s, size_t len, const char *path);
+
+/**
+ * Finishes an atomic write of \c path whose temporary file, named by
+ * filestream_atomic_temp_name(), was written and closed by other means:
+ * as filestream_commit_atomic() does after its close.
+ *
+ * @param path Path of the file that will be replaced.
+ * @param ok Whether the temporary file holds everything it should.
+ * @return As filestream_commit_atomic().
+ */
+int filestream_finish_atomic(const char *path, bool ok);
+
 /**
  * Writes a single character to the given file.
  *

libretro-common/streams/file_stream.c

@@ -2253,18 +2253,23 @@ RFILE *filestream_open_atomic(const char *path)
    return file;
 }
 
+bool filestream_atomic_temp_name(char *s, size_t len, const char *path)
+{
+   size_t path_len;
+   if (!s || !path || !*path)
+      return false;
+   path_len = strlen(path);
+   if (path_len + sizeof(".tmp") > len)
+      return false;
+   memcpy(s, path, path_len);
+   memcpy(s + path_len, ".tmp", sizeof(".tmp"));
+   return true;
+}
+
 int filestream_commit_atomic(RFILE *file, const char *path, bool ok)
 {
-   char *temp_path;
-
    if (!file)
       return -1;
-   if (!path || !*path || !(temp_path = filestream_atomic_temp_path(path)))
-   {
-      if (filestream_close(file) != 0)
-         free(file);
-      return -1;
-   }
 
    /* A buffered write reports a full disk at close, not at write,
     * so both have to agree before the rename goes ahead. */
@@ -2274,6 +2279,16 @@ int filestream_commit_atomic(RFILE *file, const char *path, bool ok)
       ok = false;
    }
 
+   return filestream_finish_atomic(path, ok);
+}
+
+int filestream_finish_atomic(const char *path, bool ok)
+{
+   char *temp_path;
+
+   if (!path || !*path || !(temp_path = filestream_atomic_temp_path(path)))
+      return -1;
+
    if (!ok)
    {
       filestream_delete(temp_path);

samples/tasks/cloudsync/cloudsync_pacing_test.c

@@ -561,6 +561,7 @@ static void walk_lane(void)
    snprintf(path, sizeof(path), "%s/.hidden/h.srm", a);      write_file(path, 4, 1);
    snprintf(path, sizeof(path), "%s/.dot.srm", a);           write_file(path, 4, 1);
    snprintf(path, sizeof(path), "%s/core2/p.srm.rafetching", a); write_file(path, 4, 1);
+   snprintf(path, sizeof(path), "%s/core2/p.srm.tmp", a);        write_file(path, 4, 1);
    snprintf(path, sizeof(path), "%s/.DS_Store", b);          write_file(path, 4, 1);
 
    nkeys = reference_keys(a, "saves", keys, 0);
@@ -584,7 +585,8 @@ static void walk_lane(void)
          same = 0;
    check("walk: the same files dir_list_new() lists, hidden ones too", same);
    for (i = 0; same && i < nkeys; i++)
-      if (strstr(keys[i], ".rafetching") || strstr(keys[i], ".DS_Store"))
+      if (     strstr(keys[i], ".rafetching") || strstr(keys[i], ".DS_Store")
+            || strstr(keys[i], ".tmp"))
          same = 0;
    check("walk: ignored names left out", same);
    task_cloud_sync_cleanup(task);

samples/tasks/save_state/save_state_io_test.c

@@ -532,6 +532,75 @@ static void test_serialize_failure(void)
    filestream_delete(path);
 }
 
+/* -----------------------------------------------------------------
+ * A save is written beside its slot and moved in only once whole: a
+ * save that fails over an existing state leaves that state as it was,
+ * and none leaves its temporary file behind.
+ * ----------------------------------------------------------------- */
+static bool tmp_exists(const char *path)
+{
+   char tmp[256];
+   snprintf(tmp, sizeof(tmp), "%s.tmp", path);
+   return file_size(tmp) >= 0;
+}
+
+/* The slot no longer holds @old: a write into it has begun. */
+static bool written_into(const char *path, const void *old, int64_t n_old)
+{
+   void   *now = NULL;
+   int64_t n   = 0;
+   bool    changed;
+   if (!filestream_read_file(path, &now, &n))
+      return true;
+   changed = n != n_old || memcmp(now, old, (size_t)n_old);
+   free(now);
+   return changed;
+}
+
+static void test_failed_save_keeps_old(void)
+{
+   const char *path = "sst_keep.state";
+   size_t   sz      = 12 * TEST_SAVE_STATE_CHUNK;
+   void    *before  = NULL, *after = NULL;
+   int64_t  n_before = 0, n_after = 0;
+
+   frontend_reset();
+   core_fill(sz);
+   filestream_delete(path);
+   content_save_state(path, true);
+   pump(1000);
+   okf(!tmp_exists(path), "a save leaves nothing beside its slot");
+   if (!filestream_read_file(path, &before, &n_before))
+      n_before = -1;
+
+   /* A quantum a tick, cancelled two ticks into the write: whatever
+    * the save does first (keeping the old state for undo), the write
+    * has begun once the slot is open, beside it or in it. */
+   memset(core_mem, 0x5A, sz);
+   clock_step = TEST_TICK_BUDGET_US;
+   content_save_state(path, true);
+   {
+      int i;
+      for (i = 0; i < 1000 && !tmp_exists(path) && !written_into(path, before, n_before); i++)
+         task_queue_check();
+   }
+   task_queue_check();
+   task_queue_check();
+   task_queue_reset();
+   pump(1000);
+   clock_step = 0;
+
+   if (!filestream_read_file(path, &after, &n_after))
+      n_after = -2;
+   okf(n_before > 0 && n_after == n_before && !memcmp(before, after, (size_t)n_before),
+       "a save cancelled part-way over a state leaves that state as it was");
+   okf(!tmp_exists(path), "a cancelled save leaves nothing beside its slot");
+
+   free(before);
+   free(after);
+   filestream_delete(path);
+}
+
 /* -----------------------------------------------------------------
  * A background save defers only the core's serialize to the handler
  * - that deferral is the whole point of the core's
@@ -1181,6 +1250,7 @@ static int run_default_lane(void)
          "round-trip is byte-exact at one quantum per tick");
    test_close_waits_for_save();
    test_serialize_failure();
+   test_failed_save_keeps_old();
    test_background_serialize_timing();
    test_open_failure();
    test_truncated_state();

save.c

@@ -566,17 +566,21 @@ static bool content_save_ram_file(unsigned slot, bool compress)
          msg_hash_to_str(MSG_TO),
          ram.path);
 
+   /* Write via a temporary file, so a crash or power loss mid-save
+    * leaves the previous save intact instead of a truncated one. */
 #if defined(HAVE_COMPRESSION)
    if (compress)
    {
-      if (!rzipstream_write_file(
-            ram.path, mem_info.data, mem_info.size))
+      char tmp_path[PATH_MAX_LENGTH];
+      if (     !filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), ram.path)
+            || !content_finish_tmp(ram.path, rzipstream_write_file(
+                  tmp_path, mem_info.data, mem_info.size)))
          goto fail;
    }
    else
 #endif
    {
-      if (!filestream_write_file(
+      if (!filestream_write_file_atomic(
             ram.path, mem_info.data, mem_info.size))
          goto fail;
    }

tasks/task_cloudsync.c

@@ -507,6 +507,11 @@ static bool task_cloud_sync_should_ignore_file(const char *filename)
    if (string_ends_with(filename, ".rafetching"))
        return true;
 
+   /* a save, state or disk index still being written, or left by one
+    * that never finished (filestream_atomic_temp_name) */
+   if (string_ends_with(filename, ".tmp"))
+       return true;
+
    return false;
 }
 

tasks/task_save.c

@@ -412,6 +412,18 @@ static void undo_save_state_cb(retro_task_t *task,
    free(state);
 }
 
+bool content_finish_tmp(const char *path, bool ok)
+{
+   int ret = filestream_finish_atomic(path, ok);
+   if (ret == 1 && path_is_valid(path))
+   {
+      char tmp_path[PATH_MAX_LENGTH];
+      if (filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), path))
+         filestream_delete(tmp_path);
+   }
+   return ret == 0;
+}
+
 /**
  * task_save_handler_finished:
  * @task : the task to finish
@@ -430,7 +442,13 @@ static void task_save_handler_finished(retro_task_t *task,
     * (serialize failure, or the open itself). */
    if (state->file)
    {
-      intfstream_close(state->file);
+      /* rzip writes its last chunk at close, and a buffered write
+       * reports a full disk there, so a failed close is a failed save
+       * and the temporary file must not replace the old state. */
+      if (     intfstream_close(state->file) != 0
+            && !task_get_error(task))
+         task_set_error(task, strdup(
+               msg_hash_to_str(MSG_FAILED_TO_SAVE_STATE_TO)));
       free(state->file);
       state->file = NULL;
    }
@@ -440,6 +458,25 @@ static void task_save_handler_finished(retro_task_t *task,
    if (!task_get_error(task) && ((flg & RETRO_TASK_FLG_CANCELLED) > 0))
       task_set_error(task, strdup("Task canceled"));
 
+   /* The state was written to "<path>.tmp", so a crash, cancel or
+    * short write leaves the existing file intact. Move it into place
+    * on success, discard it otherwise. */
+   {
+      char tmp_path[PATH_MAX_LENGTH];
+
+      if (!filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), state->path))
+      {
+         /* Nothing was written: the open refused the same path. */
+         if (!task_get_error(task))
+            task_set_error(task, strdup(
+                  msg_hash_to_str(MSG_FAILED_TO_SAVE_STATE_TO)));
+      }
+      else if (!content_finish_tmp(state->path, !task_get_error(task))
+            && !task_get_error(task))
+         task_set_error(task, strdup(
+               msg_hash_to_str(MSG_FAILED_TO_SAVE_STATE_TO)));
+   }
+
    if (state->data)
    {
       if (     (state->flags & SAVE_TASK_FLAG_UNDO_SAVE)
@@ -838,12 +875,20 @@ static void task_save_handler(retro_task_t *task)
 
    if (!state->file)
    {
-      if (state->flags & SAVE_TASK_FLAG_COMPRESS_FILES)
+      char tmp_path[PATH_MAX_LENGTH];
+
+      /* Write to "<path>.tmp"; task_save_handler_finished moves it
+       * into place on success or deletes it on failure or cancel.
+       * A path too long to take the suffix is refused rather than
+       * written in place. */
+      if (!filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), state->path))
+         state->file   = NULL;
+      else if (state->flags & SAVE_TASK_FLAG_COMPRESS_FILES)
          state->file   = intfstream_open_rzip_file(
-               state->path, RETRO_VFS_FILE_ACCESS_WRITE);
+               tmp_path, RETRO_VFS_FILE_ACCESS_WRITE);
       else
          state->file   = intfstream_open_file(
-               state->path, RETRO_VFS_FILE_ACCESS_WRITE,
+               tmp_path, RETRO_VFS_FILE_ACCESS_WRITE,
                RETRO_VFS_FILE_ACCESS_HINT_NONE);
 
       if (!state->file)
@@ -1899,9 +1944,11 @@ static bool task_push_load_and_save_state(const char *path, void *data,
 bool content_auto_save_state(const char *path)
 {
    size_t _len;
+   int _close_ret;
    settings_t *settings = config_get_ptr();
    void *serial_data    = NULL;
    intfstream_t *file   = NULL;
+   char tmp_path[PATH_MAX_LENGTH];
 
    if (!core_info_current_supports_savestate())
    {
@@ -1918,12 +1965,20 @@ bool content_auto_save_state(const char *path)
    if (!serial_data)
       return false;
 
+   /* Write to "<path>.tmp" and move it into place, so a crash or power
+    * loss mid-save leaves the previous state intact. */
+   if (!filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), path))
+   {
+      free(serial_data);
+      return false;
+   }
+
 #if defined(HAVE_COMPRESSION)
    if (settings->bools.savestate_file_compression)
-      file = intfstream_open_rzip_file(path, RETRO_VFS_FILE_ACCESS_WRITE);
+      file = intfstream_open_rzip_file(tmp_path, RETRO_VFS_FILE_ACCESS_WRITE);
    else
 #endif
-      file = intfstream_open_file(path, RETRO_VFS_FILE_ACCESS_WRITE,
+      file = intfstream_open_file(tmp_path, RETRO_VFS_FILE_ACCESS_WRITE,
                                   RETRO_VFS_FILE_ACCESS_HINT_NONE);
 
    if (!file)
@@ -1937,13 +1992,18 @@ bool content_auto_save_state(const char *path)
       intfstream_close(file);
       free(serial_data);
       free(file);
+      filestream_delete(tmp_path);
       return false;
    }
 
-   intfstream_close(file);
+   /* A failed close (rzip's last chunk, a full disk) is a failed save. */
+   _close_ret = intfstream_close(file);
    free(serial_data);
    free(file);
 
+   if (!content_finish_tmp(path, _close_ret == 0))
+      return false;
+
 #ifdef HAVE_SCREENSHOTS
    if (settings->bools.savestate_thumbnail_enable)
    {
@@ -2424,21 +2484,25 @@ bool content_ram_state_to_file(const char *path)
          && ram_buf.state_buf.data
          && ram_buf.to_write_file)
    {
+      /* Write via a temporary file, so a crash or power loss mid-save
+       * leaves the previous file intact instead of a truncated one. */
+      bool written = false;
 #if defined(HAVE_COMPRESSION)
       settings_t *settings = config_get_ptr();
       if (settings->bools.save_file_compression)
       {
-         if (rzipstream_write_file(
-               path, ram_buf.state_buf.data, ram_buf.state_buf.size))
-            goto success;
+         char tmp_path[PATH_MAX_LENGTH];
+         if (filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), path))
+            written = content_finish_tmp(path, rzipstream_write_file(tmp_path,
+                  ram_buf.state_buf.data, ram_buf.state_buf.size));
       }
       else
 #endif
-      {
-         if (filestream_write_file(
-               path, ram_buf.state_buf.data, ram_buf.state_buf.size))
-            goto success;
-      }
+         written = filestream_write_file_atomic(path,
+               ram_buf.state_buf.data, ram_buf.state_buf.size);
+
+      if (written)
+         goto success;
    }
 
    return false;