hts_rename_over() can lose its destination when the retried rename fails

src/htsback.c

@@ -646,7 +646,7 @@ void back_refetch_backup(httrackp *opt, lien_back *const back) {
       if (fexist_utf8(back->tmpfile))
         hts_log_print(opt, LOG_WARNING, "replacing leftover backup %s",
                       back->tmpfile);
-      saved = hts_rename_over(back->url_sav, back->tmpfile);
+      saved = hts_rename_over(opt, back->url_sav, back->tmpfile);
     }
     if (!saved) {
       hts_log_print(opt, LOG_WARNING | LOG_ERRNO,
@@ -687,7 +687,7 @@ static void back_finalize_backup(httrackp *opt, lien_back *const back,
     }
     /* On failure keep the backup: an orphaned temp beats losing the good copy.
      */
-    if (!hts_rename_over(back->tmpfile, back->url_sav))
+    if (!hts_rename_over(opt, back->tmpfile, back->url_sav))
       hts_log_print(opt, LOG_WARNING | LOG_ERRNO,
                     "could not restore %s; previous copy kept as %s",
                     back->url_sav, back->tmpfile);
@@ -818,7 +818,7 @@ int back_finalize(httrackp * opt, cache_back * cache, struct_back * sback,
                                  "Read error when decompressing");
                     }
                     UNLINK(unpacked);
-                  } else if (hts_rename_over(unpacked, back[p].url_sav)) {
+                  } else if (hts_rename_over(opt, unpacked, back[p].url_sav)) {
                     /* The temp bypassed filecreate(), which is what chmods. */
 #ifndef _WIN32
                     chmod(back[p].url_sav, HTS_ACCESS_FILE);

src/htscache.c

@@ -985,6 +985,7 @@ void cache_init(cache_back * cache, httrackp * opt) {
               StringBuff(opt->path_log),
               "hts-cache/new.zip")))) { // a previous cache exists.. rename it
         if (!hts_rename_over(
+                opt,
                 fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
                         StringBuff(opt->path_log), "hts-cache/new.zip"),
                 fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),

src/htsselftest.c

@@ -6137,10 +6137,9 @@ static hts_boolean ro_is(const char *path, const char *data) {
 }
 
 // -#test=renameover <dir>: hts_rename_over() must replace an existing dst and
-// never delete one it did not replace (#779). Which half is live depends on
+// never lose one it did not replace (#779, #790). Which half is live depends on
 // what rename() does to an existing target, so probe that and name the regime.
 static int st_renameover(httrackp *opt, int argc, char **argv) {
-  (void) opt;
   if (argc < 1) {
     fprintf(stderr, "renameover: needs a writable base dir\n");
     return 1;
@@ -6169,7 +6168,7 @@ static int st_renameover(httrackp *opt, int argc, char **argv) {
   ro_put(dst, "old");
   if (replaceable) {
     /* An existing dst must still be replaced: the unlink is for this. */
-    if (!hts_rename_over(src, dst)) {
+    if (!hts_rename_over(opt, src, dst)) {
       fprintf(stderr, "renameover: replacing an existing dst failed: %s\n",
               strerror(errno));
       err++;
@@ -6179,7 +6178,7 @@ static int st_renameover(httrackp *opt, int argc, char **argv) {
     }
   } else {
     /* A failure the unlink cannot fix must leave dst as it was. */
-    if (hts_rename_over(src, dst)) {
+    if (hts_rename_over(opt, src, dst)) {
       fprintf(stderr, "renameover: an unfixable failure reported success\n");
       err++;
     }
@@ -6189,10 +6188,36 @@ static int st_renameover(httrackp *opt, int argc, char **argv) {
     }
   }
 
+  /* A directory in the way is not something the caller asked to replace: it
+     must be refused, never parked aside and orphaned. */
+  (void) UNLINK(dst);
+  ro_put(src, "new");
+  if (MKDIR(dst) == 0) {
+    char parked[sizeof(dst) + 16];
+
+    snprintf(parked, sizeof(parked), "%s.hts-old0", dst);
+    if (hts_rename_over(opt, src, dst)) {
+      fprintf(stderr, "renameover: a directory at dst reported success\n");
+      err++;
+    }
+    if (!ro_is(src, "new")) {
+      fprintf(stderr, "renameover: a directory at dst consumed src\n");
+      err++;
+    }
+    /* RMDIR only succeeds on a directory that is there, so it doubles as the
+       probe: the parked name must not exist at all. */
+    if (RMDIR(parked) == 0 || fexist_utf8(parked)) {
+      fprintf(stderr, "renameover: a directory at dst was parked aside\n");
+      err++;
+    }
+    (void) RMDIR(dst);
+  }
+  (void) UNLINK(src);
+
   /* A missing src must leave dst alone and report failure. */
   (void) UNLINK(src);
   ro_put(dst, "keep");
-  if (hts_rename_over(src, dst)) {
+  if (hts_rename_over(opt, src, dst)) {
     fprintf(stderr, "renameover: a missing src reported success\n");
     err++;
   }
@@ -6203,11 +6228,88 @@ static int st_renameover(httrackp *opt, int argc, char **argv) {
 
   /* Same, with dst absent too: nothing to lose, still a failure. */
   (void) UNLINK(dst);
-  if (hts_rename_over(src, dst)) {
+  if (hts_rename_over(opt, src, dst)) {
     fprintf(stderr, "renameover: a missing src and dst reported success\n");
     err++;
   }
 
+  /* The aside fallback, driven directly: a clobbering rename() never reaches
+     it. Skipped in the refused regime, where no rename at all succeeds. */
+  if (replaceable) {
+    char aside[sizeof(dst) + 16], keep[sizeof(dst) + 16];
+
+    snprintf(aside, sizeof(aside), "%s.hts-old0", dst);
+    snprintf(keep, sizeof(keep), "%s.hts-old1", dst);
+    (void) UNLINK(aside);
+    (void) UNLINK(keep);
+    ro_put(src, "new");
+    ro_put(dst, "old");
+    if (!hts_rename_over_aside_selftest(opt, src, dst)) {
+      fprintf(stderr, "renameover: the aside fallback failed: %s\n",
+              strerror(errno));
+      err++;
+    } else if (!ro_is(dst, "new") || fexist_utf8(src) || fexist_utf8(aside)) {
+      fprintf(stderr, "renameover: the aside fallback did not replace dst\n");
+      err++;
+    }
+
+    /* #790: the retry fails (no src). The old content must survive, back at dst
+       or, when the move back fails too, under the parked name it is logged as.
+       Name the outcome so a leg cannot pass having tested the other one. */
+    (void) UNLINK(src);
+    ro_put(dst, "old");
+    if (hts_rename_over_aside_selftest(opt, src, dst)) {
+      fprintf(stderr, "renameover: a failed aside retry reported success\n");
+      err++;
+    }
+    if (ro_is(dst, "old") && !fexist_utf8(aside)) {
+      printf("renameover: restore back\n");
+    } else if (ro_is(aside, "old") && !fexist_utf8(dst)) {
+      printf("renameover: restore parked\n");
+      (void) UNLINK(aside);
+      ro_put(dst, "old");
+    } else {
+      fprintf(stderr, "renameover: a failed aside retry lost the old copy\n");
+      err++;
+    }
+
+    /* An unrelated file already sitting on the aside name must survive. */
+    ro_put(src, "new");
+    ro_put(aside, "mine");
+    if (!hts_rename_over_aside_selftest(opt, src, dst)) {
+      fprintf(stderr, "renameover: a taken aside name failed the move: %s\n",
+              strerror(errno));
+      err++;
+    } else if (!ro_is(dst, "new") || !ro_is(aside, "mine") ||
+               fexist_utf8(keep)) {
+      fprintf(stderr, "renameover: a taken aside name was not skipped\n");
+      err++;
+    }
+    (void) UNLINK(aside);
+    (void) UNLINK(keep);
+
+    /* A directory there reads as free to the probe, so the park must skip it
+       on the refusal rather than give up. */
+    ro_put(src, "new");
+    ro_put(dst, "old");
+    if (MKDIR(aside) == 0) {
+      if (!hts_rename_over_aside_selftest(opt, src, dst)) {
+        fprintf(stderr,
+                "renameover: a directory on the aside name blocked the "
+                "move: %s\n",
+                strerror(errno));
+        err++;
+      } else if (!ro_is(dst, "new") || fexist_utf8(keep)) {
+        fprintf(stderr, "renameover: a directory on the aside name was not "
+                        "skipped\n");
+        err++;
+      }
+      (void) RMDIR(aside);
+    }
+    (void) UNLINK(keep);
+  }
+
+  (void) UNLINK(src);
   (void) UNLINK(dst);
   printf("renameover: %s\n", err ? "FAIL" : "OK");
   return err;

src/htssinglefile.c

@@ -1109,7 +1109,7 @@ hts_boolean singlefile_rewrite_file(httrackp *opt, const char *root,
                    HTS_ACCESS_FILE);
 #endif
     if (ok)
-      ok = hts_rename_over(StringBuff(tmp), page_path);
+      ok = hts_rename_over(opt, StringBuff(tmp), page_path);
     if (!ok) {
       hts_log_print(opt, LOG_ERROR, "single-file: could not rewrite %s",
                     page_path);

src/htstools.c

@@ -1438,21 +1438,75 @@ HTSEXT_API hts_boolean hts_findissystem(find_handle find) {
   return 0;
 }
 
-hts_boolean hts_rename_over(const char *src, const char *dst) {
+/* Park cdst under a free sibling name; caside receives it. */
+static hts_boolean rename_park_aside(char *caside, size_t size,
+                                     const char *cdst) {
+  int i;
+
+  for (i = 0; i < 16; i++) {
+    if (!slprintfbuff(caside, size, "%s.hts-old%d", cdst, i))
+      return HTS_FALSE;
+    /* Skip a name the mirror already holds: POSIX rename() would clobber it
+       (#774). A non-regular entry reads as free and the rename refuses it. */
+    if (fexist_utf8(caside))
+      continue;
+    if (RENAME(cdst, caside) == 0)
+      return HTS_TRUE;
+  }
+  return HTS_FALSE;
+}
+
+/* cdst is in the way of the move: park it, retry, and put it back if the retry
+   fails too. Unlinking it instead would leave nothing at all (#790). */
+static hts_boolean rename_over_aside(httrackp *opt, const char *csrc,
+                                     const char *cdst) {
+  char caside[CATBUFF_SIZE];
+  int err;
+
+  /* Only a regular file may be parked: a directory in the way is not what the
+     caller asked to replace, and parking it orphans it (UNLINK cannot drop). */
+  if (!fexist_utf8(cdst))
+    return HTS_FALSE;
+  if (!rename_park_aside(caside, sizeof(caside), cdst))
+    return HTS_FALSE;
+  if (RENAME(csrc, cdst) == 0) {
+    (void) UNLINK(caside);
+    return HTS_TRUE;
+  }
+  err = errno;
+  /* Retry once, then name the parked copy: nothing else on disk or in the log
+     points at it, and an --update purge would delete it unnoticed. */
+  if (RENAME(caside, cdst) != 0 && RENAME(caside, cdst) != 0)
+    hts_log_print(opt, LOG_WARNING | LOG_ERRNO,
+                  "could not put %s back; its previous content is now %s", cdst,
+                  caside);
+  errno = err;
+  return HTS_FALSE;
+}
+
+hts_boolean hts_rename_over(httrackp *opt, const char *src, const char *dst) {
   char csrc[CATBUFF_SIZE], cdst[CATBUFF_SIZE];
 
   fconv(csrc, sizeof(csrc), src);
   fconv(cdst, sizeof(cdst), dst);
   if (RENAME(csrc, cdst) == 0)
     return HTS_TRUE;
-  /* Only a dst in the way is something the unlink can clear, and the CRT maps
-     that to EEXIST; it keeps EACCES for a src another process holds, where
-     removing dst would lose a file the retry cannot replace (#790). The src
-     check covers a CRT that reports neither. */
+  /* Only a dst in the way is something the fallback can clear, and the CRT maps
+     that to EEXIST; it keeps EACCES for a src another process holds, where the
+     retry would fail the same way. The src check covers a CRT that reports
+     neither. */
   const int err = errno;
 
   if (err != EEXIST || !fexist_utf8(src))
     return HTS_FALSE;
-  (void) UNLINK(cdst);
-  return RENAME(csrc, cdst) == 0 ? HTS_TRUE : HTS_FALSE;
+  return rename_over_aside(opt, csrc, cdst);
+}
+
+hts_boolean hts_rename_over_aside_selftest(httrackp *opt, const char *src,
+                                           const char *dst) {
+  char csrc[CATBUFF_SIZE], cdst[CATBUFF_SIZE];
+
+  fconv(csrc, sizeof(csrc), src);
+  fconv(cdst, sizeof(cdst), dst);
+  return rename_over_aside(opt, csrc, cdst);
 }

src/htstools.h

@@ -137,11 +137,17 @@ HTSEXT_API hts_boolean hts_findisdir(find_handle find);
 HTSEXT_API hts_boolean hts_findisfile(find_handle find);
 HTSEXT_API hts_boolean hts_findissystem(find_handle find);
 
-/* Move src onto dst, replacing an existing dst; HTS_TRUE on success. Both
-   paths are fconv()'d. dst is removed only to make room for a src that exists,
-   so a caller whose src was never written keeps its dst; a retry that still
-   fails does not (#790). */
-hts_boolean hts_rename_over(const char *src, const char *dst);
+/* Move src onto dst, replacing an existing dst; HTS_TRUE on success. Both paths
+   are fconv()'d. A dst in the way is parked under a sibling name rather than
+   removed, so the old content survives a failure: back at dst, or under that
+   sibling (named in the log) when the move back failed too. Not atomic: a crash
+   between the two renames leaves dst absent and its content beside it. */
+hts_boolean hts_rename_over(httrackp *opt, const char *src, const char *dst);
+
+/* Selftest hook: run the aside fallback directly, on a platform whose rename()
+   never reaches it. Both paths are fconv()'d. */
+hts_boolean hts_rename_over_aside_selftest(httrackp *opt, const char *src,
+                                           const char *dst);
 
 #endif
 

src/htswarc.c

@@ -1114,7 +1114,7 @@ static void warc_wacz_package(warc_writer *w) {
     hts_log_print(w->opt, LOG_WARNING,
                   "WACZ: packaging failed, kept existing %s untouched",
                   waczpath);
-  } else if (!hts_rename_over(tmppath, waczpath)) {
+  } else if (!hts_rename_over(w->opt, tmppath, waczpath)) {
     (void) UNLINK(fconv(catbuff, sizeof(catbuff), tmppath));
     hts_log_print(w->opt, LOG_WARNING | LOG_ERRNO,
                   "WACZ: could not finalize %s", waczpath);
@@ -1589,7 +1589,7 @@ static hts_boolean warc_commit(warc_writer *w) {
     for (s = 0; s < nseg; s++) {
       const char *final = warc_seg_path(w, s, finalbuf, sizeof(finalbuf));
       snprintf(tmpbuf, sizeof(tmpbuf), "%s" WARC_TMP_SUFFIX, final);
-      if (!hts_rename_over(tmpbuf, final)) {
+      if (!hts_rename_over(w->opt, tmpbuf, final)) {
         hts_log_print(w->opt, LOG_ERROR | LOG_ERRNO,
                       "WARC: could not replace %s", final);
         return HTS_FALSE;

tests/01_engine-renameover.test

@@ -4,9 +4,9 @@
 set -euo pipefail
 
 # Drives -#test=renameover: hts_rename_over() must replace an existing dst, and
-# must leave dst alone when the rename failed for a reason removing dst cannot
-# fix (#779). The selftest prints the regime it detected; pin it per platform so
-# a leg cannot pass having tested the other half.
+# must leave dst alone when the rename failed for a reason moving dst aside
+# cannot fix (#779, #790). The selftest prints the regime and the restore
+# outcome it took; pin both per leg so none can pass having tested another.
 dir=$(mktemp -d)
 trap 'set +e; rm -rf "$dir"' EXIT
 
@@ -18,6 +18,7 @@ esac
 out=$(httrack -O /dev/null -#test=renameover "$dir")
 grep -q "renameover: OK" <<<"$out"
 grep -q "renameover: regime $want" <<<"$out"
+grep -q "renameover: restore back" <<<"$out"
 
 if [ "$(uname -s)" != "Linux" ]; then
     echo "renameover: LD_PRELOAD interposition is Linux-only here, skipping"
@@ -42,11 +43,26 @@ fi
 # refuses by default. The shim allocates nothing, so the ordering is harmless.
 export ASAN_OPTIONS="${ASAN_OPTIONS:+$ASAN_OPTIONS:}verify_asan_link_order=0"
 
-# The unlink fallback is dead code on POSIX, so borrow Windows' rename().
+# The aside fallback is dead code on POSIX, so borrow Windows' rename().
 out=$(LD_PRELOAD="$RENAMEFAIL_LIB" httrack -O /dev/null \
     -#test=renameover "$dir")
 grep -q "renameover: OK" <<<"$out"
 grep -q "renameover: regime fallback" <<<"$out"
+grep -q "renameover: restore back" <<<"$out"
+
+# #790: the move back out of the parked name fails once. Without the retry the
+# old copy stays parked and dst is left absent.
+out=$(RENAMEFAIL_ASIDE_FAILS=1 LD_PRELOAD="$RENAMEFAIL_LIB" httrack -O /dev/null \
+    -#test=renameover "$dir")
+grep -q "renameover: OK" <<<"$out"
+grep -q "renameover: restore back" <<<"$out"
+
+# It keeps failing: the old copy must survive under the parked name, never be
+# deleted, and the call must still report failure.
+out=$(RENAMEFAIL_ASIDE_FAILS=9 LD_PRELOAD="$RENAMEFAIL_LIB" httrack -O /dev/null \
+    -#test=renameover "$dir")
+grep -q "renameover: OK" <<<"$out"
+grep -q "renameover: restore parked" <<<"$out"
 
 # A source another process holds fails with EACCES, which dst had no part in.
 out=$(RENAMEFAIL_MODE=locked LD_PRELOAD="$RENAMEFAIL_LIB" httrack -O /dev/null \