hts_rename_over() deletes its destination when the source is missing

src/htslib.c

@@ -6648,9 +6648,12 @@ int hts_rename_utf8(const char *oldpath, const char *newpath) {
   LPWSTR wnewpath = hts_pathToUCS2(newpath);
   if (woldpath != NULL && wnewpath != NULL) {
     const int result = _wrename(woldpath, wnewpath);
+    /* Save errno: callers key off it (#779) and free() may clobber it. */
+    const int err = errno;
 
     free(woldpath);
     free(wnewpath);
+    errno = err;
     return result;
   } else {
     if (woldpath != NULL)

src/htsselftest.c

@@ -5955,6 +5955,104 @@ static int st_mirrorio(httrackp *opt, int argc, char **argv) {
   return 0;
 }
 
+static void ro_put(const char *path, const char *data) {
+  FILE *const fp = FOPEN(path, "wb");
+
+  assertf(fp != NULL);
+  assertf(fwrite(data, 1, strlen(data), fp) == strlen(data));
+  fclose(fp);
+}
+
+/* HTS_TRUE if path holds exactly data. */
+static hts_boolean ro_is(const char *path, const char *data) {
+  char buf[64];
+  FILE *const fp = FOPEN(path, "rb");
+  size_t n;
+
+  if (fp == NULL)
+    return HTS_FALSE;
+  n = fread(buf, 1, sizeof(buf), fp);
+  fclose(fp);
+  return n == strlen(data) && memcmp(buf, data, n) == 0 ? HTS_TRUE : HTS_FALSE;
+}
+
+// -#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
+// 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;
+  }
+  char src[HTS_URLMAXSIZE * 2], dst[HTS_URLMAXSIZE * 2];
+  int err = 0;
+
+  fconcat(src, sizeof(src), argv[0], "renameover-src.bin");
+  fconcat(dst, sizeof(dst), argv[0], "renameover-dst.bin");
+
+  (void) UNLINK(src);
+  (void) UNLINK(dst);
+  ro_put(src, "probe");
+  ro_put(dst, "probe");
+
+  const int probe = RENAME(src, dst) == 0 ? 0 : errno;
+  /* Only a target in the way is something the unlink can clear. */
+  const hts_boolean replaceable = probe == 0 || probe == EEXIST;
+
+  printf("renameover: regime %s\n",
+         probe == 0 ? "clobber" : (probe == EEXIST ? "fallback" : "refused"));
+
+  (void) UNLINK(src);
+  (void) UNLINK(dst);
+  ro_put(src, "new");
+  ro_put(dst, "old");
+  if (replaceable) {
+    /* An existing dst must still be replaced: the unlink is for this. */
+    if (!hts_rename_over(src, dst)) {
+      fprintf(stderr, "renameover: replacing an existing dst failed: %s\n",
+              strerror(errno));
+      err++;
+    } else if (!ro_is(dst, "new") || fexist_utf8(src)) {
+      fprintf(stderr, "renameover: dst was not replaced by src\n");
+      err++;
+    }
+  } else {
+    /* A failure the unlink cannot fix must leave dst as it was. */
+    if (hts_rename_over(src, dst)) {
+      fprintf(stderr, "renameover: an unfixable failure reported success\n");
+      err++;
+    }
+    if (!ro_is(dst, "old")) {
+      fprintf(stderr, "renameover: an unfixable failure destroyed dst\n");
+      err++;
+    }
+  }
+
+  /* A missing src must leave dst alone and report failure. */
+  (void) UNLINK(src);
+  ro_put(dst, "keep");
+  if (hts_rename_over(src, dst)) {
+    fprintf(stderr, "renameover: a missing src reported success\n");
+    err++;
+  }
+  if (!ro_is(dst, "keep")) {
+    fprintf(stderr, "renameover: a missing src destroyed dst\n");
+    err++;
+  }
+
+  /* Same, with dst absent too: nothing to lose, still a failure. */
+  (void) UNLINK(dst);
+  if (hts_rename_over(src, dst)) {
+    fprintf(stderr, "renameover: a missing src and dst reported success\n");
+    err++;
+  }
+
+  (void) UNLINK(dst);
+  printf("renameover: %s\n", err ? "FAIL" : "OK");
+  return err;
+}
+
 // -#test=direnum <dir>: enumerate a long+non-ASCII directory via the
 // opendir/readdir wrappers; children must round-trip as UTF-8 (#133,#630).
 static int st_direnum(httrackp *opt, int argc, char **argv) {
@@ -6603,6 +6701,10 @@ static const struct selftest_entry {
     {"mirrorio", "<dir>",
      "round-trip a long+non-ASCII path through the mirror I/O wrappers",
      st_mirrorio},
+    {"renameover", "<dir>",
+     "hts_rename_over(): replace dst, but never delete a dst it did not "
+     "replace",
+     st_renameover},
     {"direnum", "<dir>",
      "enumerate a long+non-ASCII directory through opendir/readdir",
      st_direnum},

src/htstools.c

@@ -1451,7 +1451,14 @@ hts_boolean hts_rename_over(const char *src, const char *dst) {
   fconv(cdst, sizeof(cdst), dst);
   if (RENAME(csrc, cdst) == 0)
     return HTS_TRUE;
-  /* RENAME does not clobber an existing target on Windows. */
+  /* 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. */
+  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;
 }

src/htstools.h

@@ -138,7 +138,9 @@ 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. */
+   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);
 
 #endif

src/htswarc.c

@@ -1552,8 +1552,8 @@ static hts_boolean warc_commit(warc_writer *w) {
   if (!w->protect_prev)
     return HTS_TRUE; /* nothing was there to lose: written in place */
 
-  /* hts_rename_over unlinks its destination when the source is missing, so
-     every segment has to be on disk before the first rename. */
+  /* All or nothing: a swap stopping halfway would mix this run's segments with
+     the previous one's, so require every temp before renaming any. */
   swap = w->opened && !w->failed && w->unbacked_revisits == 0;
   for (s = 0; s < nseg && swap; s++) {
     snprintf(tmpbuf, sizeof(tmpbuf), "%s" WARC_TMP_SUFFIX,

tests/01_engine-renameover.test

@@ -0,0 +1,55 @@
+#!/bin/bash
+#
+
+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.
+dir=$(mktemp -d)
+trap 'rm -rf "$dir"' EXIT
+
+case "$(uname -s)" in
+MINGW* | MSYS_NT*) want=fallback ;; # native rename() refuses an existing target
+*) want=clobber ;;
+esac
+
+out=$(httrack -O /dev/null -#test=renameover "$dir")
+echo "$out" | grep -q "renameover: OK"
+echo "$out" | grep -q "renameover: regime $want"
+
+if [ "$(uname -s)" != "Linux" ]; then
+    echo "renameover: LD_PRELOAD interposition is Linux-only here, skipping"
+    exit 0
+fi
+# A --disable-shared build has nothing to preload; anything else missing is a
+# build problem, not a skip, or the interposed legs would pass vacuously.
+if [ ! -r "${RENAMEFAIL_LA:-}" ]; then
+    echo "renameover: ${RENAMEFAIL_LA:-\$RENAMEFAIL_LA} was not built" >&2
+    exit 1
+fi
+if grep -q "^dlname=''" "$RENAMEFAIL_LA"; then
+    echo "renameover: static-only build, skipping the interposed legs"
+    exit 0
+fi
+if [ ! -r "${RENAMEFAIL_LIB:-}" ]; then
+    echo "renameover: ${RENAMEFAIL_LIB:-\$RENAMEFAIL_LIB} was not built" >&2
+    exit 1
+fi
+
+# An LD_PRELOAD library loads ahead of the executable's own libasan, which ASan
+# 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().
+out=$(LD_PRELOAD="$RENAMEFAIL_LIB" httrack -O /dev/null \
+    -#test=renameover "$dir")
+echo "$out" | grep -q "renameover: OK"
+echo "$out" | grep -q "renameover: regime fallback"
+
+# 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 \
+    -#test=renameover "$dir")
+echo "$out" | grep -q "renameover: OK"
+echo "$out" | grep -q "renameover: regime refused"

tests/Makefile.am

@@ -1,7 +1,7 @@
 # Committed binary fixture read by 01_zlib-cache-golden.test. List it
 # explicitly: automake does not expand wildcards in EXTRA_DIST, so a glob would
 # silently drop it from the dist tarball and break "make distcheck".
-EXTRA_DIST = $(TESTS) crawl-test.sh run-all-tests.sh check-network.sh \
+EXTRA_DIST = $(TESTS) renamefail.c crawl-test.sh run-all-tests.sh check-network.sh \
 	proxy-https-server.py socks5-server.py proxy-connect-server.py \
 	proxytestlib.py tls-stall-server.py warc-validate.py wacz-validate.py \
 	pty-resize.py \
@@ -21,6 +21,15 @@ TESTS_ENVIRONMENT += BROTLI_ENABLED=$(BROTLI_ENABLED)
 TESTS_ENVIRONMENT += ZSTD_ENABLED=$(ZSTD_ENABLED)
 TESTS_ENVIRONMENT += V6_SUPPORT=$(V6_SUPPORT)
 TESTS_ENVIRONMENT += top_srcdir=$(top_srcdir)
+TESTS_ENVIRONMENT += RENAMEFAIL_LA=$(abs_builddir)/librenamefail.la
+TESTS_ENVIRONMENT += RENAMEFAIL_LIB=$(abs_builddir)/$(LT_CV_OBJDIR)/librenamefail.so
+
+# rename() interposer for 01_engine-renameover.test; -rpath is what makes
+# libtool build a shared module rather than a static-only convenience library.
+check_LTLIBRARIES = librenamefail.la
+librenamefail_la_SOURCES = renamefail.c
+librenamefail_la_LDFLAGS = -module -avoid-version -rpath $(abs_builddir)
+librenamefail_la_LIBADD = $(DL_LIBS)
 
 TEST_EXTENSIONS = .test
 # Run each .test through bash instead of execve()ing it. This lets "make check"
@@ -74,6 +83,7 @@ TESTS = \
 	01_engine-direnum.test \
 	01_engine-cookieimport.test \
 	01_engine-relative.test \
+	01_engine-renameover.test \
 	01_engine-robots.test \
 	01_engine-savename.test \
 	01_engine-selftest-dispatch.test \

tests/renamefail.c

@@ -0,0 +1,41 @@
+/* Borrows Windows' rename() for 01_engine-renameover.test, since POSIX cannot
+   reach hts_rename_over()'s unlink fallback: an existing target is refused with
+   EEXIST, and RENAMEFAIL_MODE=locked reports EACCES instead, as the CRT does
+   for a source another process holds. */
+
+#define _GNU_SOURCE
+#include <dlfcn.h>
+#include <errno.h>
+#include <stddef.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+
+/* The tree builds with -fvisibility=hidden, which would hide the interposer. */
+#define SHIM_EXPORT __attribute__((visibility("default")))
+
+SHIM_EXPORT int rename(const char *oldpath, const char *newpath);
+
+SHIM_EXPORT int rename(const char *oldpath, const char *newpath) {
+  static int (*real_rename)(const char *, const char *) = NULL;
+  const char *const mode = getenv("RENAMEFAIL_MODE");
+  const int locked = mode != NULL && strcmp(mode, "locked") == 0;
+  struct stat st;
+
+  if (locked) {
+    errno = EACCES;
+    return -1;
+  }
+  if (stat(newpath, &st) == 0) {
+    errno = EEXIST;
+    return -1;
+  }
+  if (real_rename == NULL) {
+    *(void **) &real_rename = dlsym(RTLD_NEXT, "rename");
+    if (real_rename == NULL) {
+      errno = ENOSYS;
+      return -1;
+    }
+  }
+  return real_rename(oldpath, newpath);
+}