Merge branch 'master' into fix/cache-repair-rename

Resolve tests/renamefail.c: keep the repair/repair-eexist narrowing
(#786) that master's simpler locked-only shim lacks; the aside-fails
and rename-over regimes carry through unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>

.github/workflows/windows-build.yml

@@ -211,7 +211,7 @@ jobs:
           pass=0 fail=0 skip=0 failed="" skipped="" deadline=0
           for t in 00_runnable.test 01_engine-*.test 01_zlib-*.test \
                    *_local-*.test 13_crawl_proxy_https.test 58_watchdog.test \
-                   60_crawl-log-salvage.test; do
+                   60_crawl-log-salvage.test 106_engine-repair-rename.test; do
             elapsed=$((SECONDS - started))
             if [ "$elapsed" -ge "$suite_deadline" ]; then
               echo "::error::suite deadline: ${elapsed}s elapsed, stopping before $t"

src/htscache.c

@@ -957,6 +957,35 @@ htsblk *cache_header(httrackp * opt, cache_back * cache, const char *adr,
     return NULL;
 }
 
+const char *cache_repair(httrackp *opt, const char *name,
+                         unsigned long *entries, unsigned long *bytes) {
+  char BIGSTK repairname[HTS_URLMAXSIZE * 2];
+  unzFile zip;
+
+  *entries = 0;
+  *bytes = 0;
+  if (!slprintfbuff(repairname, sizeof(repairname), "%s%s",
+                    StringBuff(opt->path_log), "hts-cache/repair.zip"))
+    return "the repair path is too long";
+  if (unzRepair(name, repairname,
+                fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
+                        StringBuff(opt->path_log), "hts-cache/repair.tmp"),
+                entries, bytes) != Z_OK)
+    return "could not repair the cache";
+  /* unzRepair writes an end-of-central-directory record whatever it found, so
+     an input holding no local file header at all yields a valid empty archive
+     and a short write yields a truncated one. Only an archive that holds
+     something and opens may replace the cache (#824). */
+  if (*entries == 0)
+    return "the repaired cache holds no entry, keeping the damaged one";
+  if ((zip = hts_unzOpen_utf8(repairname)) == NULL)
+    return "the repaired cache does not open, keeping the damaged one";
+  unzClose(zip);
+  if (!hts_rename_over(opt, repairname, name))
+    return "could not put the repaired cache in place";
+  return NULL;
+}
+
 // Initialisation du cache: créer nouveau, renomer ancien, charger..
 void cache_init(cache_back * cache, httrackp * opt) {
   // ---
@@ -1026,8 +1055,9 @@ void cache_init(cache_back * cache, httrackp * opt) {
       // Corrupted ZIP file ? Try to repair!
       if (cache->zipInput == NULL && !cache->ro) {
         char *name;
-        uLong repaired = 0;
-        uLong repairedBytes = 0;
+        const char *why;
+        unsigned long repaired = 0;
+        unsigned long repairedBytes = 0;
 
         if (!cache->ro) {
           name =
@@ -1040,25 +1070,16 @@ void cache_init(cache_back * cache, httrackp * opt) {
         }
         hts_log_print(opt, LOG_WARNING,
                       "Cache: damaged cache, trying to repair");
-        /* mztools has no UTF-8 hook, so repairing a corrupt cache under a
-           non-ASCII path_log fails cleanly (re-crawl), never forks a twin. */
-        if (unzRepair
-            (name,
-             fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log),
-                     "hts-cache/repair.zip"), fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                                                      StringBuff(opt->path_log),
-                                                      "hts-cache/repair.tmp"),
-             &repaired, &repairedBytes) == Z_OK) {
-          UNLINK(name);
-          RENAME(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                         StringBuff(opt->path_log), "hts-cache/repair.zip"),
-                 name);
-          cache->zipInput = hts_unzOpen_utf8(name);
+        why = cache_repair(opt, name, &repaired, &repairedBytes);
+        if (why != NULL) {
+          hts_log_print(opt, LOG_WARNING | LOG_ERRNO, "Cache: %s", why);
+        } else if ((cache->zipInput = hts_unzOpen_utf8(name)) != NULL) {
           hts_log_print(opt, LOG_WARNING,
                         "Cache: %d bytes successfully recovered in %d entries",
                         (int) repairedBytes, (int) repaired);
         } else {
-          hts_log_print(opt, LOG_WARNING, "Cache: could not repair the cache");
+          hts_log_print(opt, LOG_WARNING,
+                        "Cache: the repaired cache could not be reopened");
         }
       }
       // Opened ?

src/htscache.h

@@ -78,6 +78,14 @@ htsblk *cache_header(httrackp * opt, cache_back * cache, const char *adr,
                      const char *fil, htsblk * r);
 void cache_init(cache_back * cache, httrackp * opt);
 
+/* Recover the damaged cache at name into hts-cache/repair.zip and move it over
+   name, storing what was recovered in *entries and *bytes. Returns NULL on
+   success, else a reason the caller reports: a recovery that is empty or does
+   not open never replaces the cache, and neither does one that cannot be moved
+   into place (#786, #824). Note: utf-8. */
+const char *cache_repair(httrackp *opt, const char *name,
+                         unsigned long *entries, unsigned long *bytes);
+
 /* Which hts-cache/ generation (new.* vs old.*) is authoritative. */
 typedef enum {
   CACHE_RECONCILE_PROMOTE,     /* no new cache: promote the old generation */

src/htscoremain.c

@@ -2160,8 +2160,9 @@ static int hts_main_internal(int argc, char **argv, httrackp * opt) {
               case 'R':
                 {
                   char *name;
-                  uLong repaired = 0;
-                  uLong repairedBytes = 0;
+                  const char *why;
+                  unsigned long repaired = 0;
+                  unsigned long repairedBytes = 0;
 
                   if (fexist_utf8(fconcat(
                           OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
@@ -2184,24 +2185,15 @@ static int hts_main_internal(int argc, char **argv, httrackp * opt) {
                     return 1;
                   }
                   fprintf(stderr, "Cache: trying to repair %s\n", name);
-                  if (unzRepair
-                      (name,
-                       fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log),
-                               "hts-cache/repair.zip"),
-                       fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log),
-                               "hts-cache/repair.tmp"), &repaired,
-                       &repairedBytes) == Z_OK) {
-                    UNLINK(name);
-                    RENAME(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                                   StringBuff(opt->path_log),
-                                   "hts-cache/repair.zip"),
-                           name);
-                    fprintf(stderr,
-                            "Cache: %d bytes successfully recovered in %d entries\n",
-                            (int) repairedBytes, (int) repaired);
-                  } else {
-                    fprintf(stderr, "Cache: could not repair the cache\n");
+                  why = cache_repair(opt, name, &repaired, &repairedBytes);
+                  if (why != NULL) {
+                    fprintf(stderr, "Cache: %s\n", why);
+                    return 1;
                   }
+                  fprintf(
+                      stderr,
+                      "Cache: %d bytes successfully recovered in %d entries\n",
+                      (int) repairedBytes, (int) repaired);
                 }
                 return 0;
                 break;

tests/106_engine-repair-rename.test

@@ -0,0 +1,173 @@
+#!/bin/bash
+#
+# #786: both cache-repair paths moved the recovered zip onto the cache without
+# checking the move, and announced success either way. #824: they also committed
+# a recovery holding no entry at all. Neither may replace the cache, and both
+# must be reported.
+
+set -euo pipefail
+
+testdir=$(cd "$(dirname "$0")" && pwd)
+# shellcheck source=tests/testlib.sh
+. "${testdir}/testlib.sh"
+
+tmpdir=$(mktemp -d) # honors the Windows-shaped TMPDIR the MSYS suite exports
+cleanup() { rm -rf "$tmpdir"; }
+trap 'set +e; cleanup' EXIT
+trap 'set +e; cleanup; exit 1' HUP INT QUIT PIPE TERM
+
+# One local file header, no central directory: unzOpen fails, unzRepair
+# recovers the single entry. Same vector as -#test=zip-repair-shift.
+plant_damaged_cache() {
+    rm -rf "$1"
+    mkdir -p "$1/hts-cache"
+    printf '\120\113\003\004\024\000\000\000\000\000\000\000\000\000\000\000\350\212\000\000\000\000\000\000\000\000\001\000\000\000\141' \
+        >"$1/hts-cache/new.zip"
+}
+
+dead=http://127.0.0.1:1/
+
+# --- the CLI path (-#R), repair succeeding -----------------------------------
+printf '[-#R repairs in place] ..\t'
+proj="${tmpdir}/cli"
+plant_damaged_cache "$proj"
+out=$(httrack -O "$proj" -#R "$dead" 2>&1)
+grep -q 'successfully recovered' <<<"$out" || {
+    echo "FAIL: no recovery reported: $out"
+    exit 1
+}
+test ! -e "${proj}/hts-cache/repair.zip" || {
+    echo "FAIL: the recovery was left in repair.zip"
+    exit 1
+}
+test "$(wc -c <"${proj}/hts-cache/new.zip")" -gt 31 || {
+    echo "FAIL: new.zip is still the damaged one"
+    exit 1
+}
+echo "OK"
+
+# --- #824: nothing was recoverable, so nothing may be committed --------------
+printf '[-#R refuses an empty recovery] ..\t'
+proj="${tmpdir}/empty"
+rm -rf "$proj"
+mkdir -p "$proj/hts-cache"
+printf 'not a zip at all, some bytes' >"$proj/hts-cache/new.zip"
+rc=0
+out=$(httrack -O "$proj" -#R "$dead" 2>&1) || rc=$?
+test "$rc" -ne 0 || {
+    echo "FAIL: an empty recovery exited 0: $out"
+    exit 1
+}
+if grep -q 'successfully recovered' <<<"$out"; then
+    echo "FAIL: an empty recovery claimed success: $out"
+    exit 1
+fi
+test "$(wc -c <"${proj}/hts-cache/new.zip")" -eq 28 || {
+    echo "FAIL: the damaged cache was replaced by the empty recovery"
+    exit 1
+}
+echo "OK"
+
+if [ "$(uname -s)" != "Linux" ]; then
+    echo "repair-rename: 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 failure legs would pass vacuously.
+if [ ! -r "${RENAMEFAIL_LA:-}" ]; then
+    echo "repair-rename: ${RENAMEFAIL_LA:-\$RENAMEFAIL_LA} was not built" >&2
+    exit 1
+fi
+if grep -q "^dlname=''" "$RENAMEFAIL_LA"; then
+    echo "repair-rename: static-only build, skipping the interposed legs"
+    exit 0
+fi
+if [ ! -r "${RENAMEFAIL_LIB:-}" ]; then
+    echo "repair-rename: ${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"
+export RENAMEFAIL_MODE=repair
+export LD_PRELOAD="$RENAMEFAIL_LIB"
+
+# --- the CLI path, move refused ----------------------------------------------
+printf '[-#R reports a refused move] ..\t'
+proj="${tmpdir}/cli-fail"
+plant_damaged_cache "$proj"
+rc=0
+out=$(httrack -O "$proj" -#R "$dead" 2>&1) || rc=$?
+test "$rc" -ne 0 || {
+    echo "FAIL: a refused move exited 0: $out"
+    exit 1
+}
+if grep -q 'successfully recovered' <<<"$out"; then
+    echo "FAIL: a refused move still claimed success: $out"
+    exit 1
+fi
+test -s "${proj}/hts-cache/repair.zip" || {
+    echo "FAIL: the recovery was not kept for a retry"
+    exit 1
+}
+test "$(wc -c <"${proj}/hts-cache/new.zip")" -eq 31 || {
+    echo "FAIL: the damaged cache was destroyed anyway"
+    exit 1
+}
+echo "OK"
+
+# --- the engine path (cache_init), move refused ------------------------------
+printf '[cache_init reports a refused move] ..\t'
+proj="${tmpdir}/engine"
+plant_damaged_cache "$proj"
+httrack -O "$proj" "$dead" --timeout=2 --retries=0 -q >/dev/null 2>&1 || true
+log="${proj}/hts-log.txt"
+test -r "$log" || {
+    echo "FAIL: no crawl log"
+    exit 1
+}
+grep -q 'damaged cache' "$log" || {
+    echo "FAIL: the repair path never ran"
+    exit 1
+}
+grep -q 'could not put the repaired cache in place' "$log" || {
+    echo "FAIL: a refused move was not reported: $(grep -i cache "$log")"
+    exit 1
+}
+if grep -q 'successfully recovered' "$log"; then
+    echo "FAIL: a refused move still claimed success"
+    exit 1
+fi
+test -s "${proj}/hts-cache/repair.zip" || {
+    echo "FAIL: the recovery was not kept for a retry"
+    exit 1
+}
+echo "OK"
+
+# --- the move is refused the way Windows refuses it --------------------------
+# The destination always exists there, so every repair takes hts_rename_over()'s
+# fallback. Before #790 that fallback removed the cache and then failed.
+printf '[a refused fallback keeps the cache] ..\t'
+proj="${tmpdir}/eexist"
+plant_damaged_cache "$proj"
+rc=0
+out=$(RENAMEFAIL_MODE=repair-eexist httrack -O "$proj" -#R "$dead" 2>&1) || rc=$?
+test "$rc" -ne 0 || {
+    echo "FAIL: a refused fallback exited 0: $out"
+    exit 1
+}
+test "$(wc -c <"${proj}/hts-cache/new.zip")" -eq 31 || {
+    echo "FAIL: the fallback destroyed the cache"
+    exit 1
+}
+test -s "${proj}/hts-cache/repair.zip" || {
+    echo "FAIL: the recovery was not kept for a retry"
+    exit 1
+}
+leftover=$(find "${proj}/hts-cache" -name '*.hts-old*' | head -1)
+test -z "$leftover" || {
+    echo "FAIL: the parked copy was left behind: $leftover"
+    exit 1
+}
+echo "OK"

tests/Makefile.am

@@ -228,6 +228,7 @@ TESTS = \
 	103_teardown-status.test \
 	104_engine-warc-longurl.test \
 	105_suite-timeout.test \
+	106_engine-repair-rename.test \
 	110_local-ftp-parallel.test \
 	111_local-ftp-update-rest.test \
 	114_local-update-304-leak.test

tests/renamefail.c

@@ -1,9 +1,10 @@
-/* Borrows Windows' rename() for 01_engine-renameover.test, since POSIX cannot
-   reach hts_rename_over()'s aside fallback: an existing target is refused with
+/* Borrows Windows' rename() for the tests POSIX cannot otherwise reach:
+   hts_rename_over()'s aside fallback needs an existing target refused with
    EEXIST, and RENAMEFAIL_MODE=locked reports EACCES instead, as the CRT does
-   for a source another process holds. RENAMEFAIL_ASIDE_FAILS=N refuses the
-   first N moves back out of the parked ".hts-old" name, which is the only way
-   to reach hts_rename_over()'s restore retry (#790). */
+   for a source another process holds. =repair and =repair-eexist narrow the
+   refusal to the cache-repair move (#786), the latter through the fallback so
+   the retry inside it is the failing one. RENAMEFAIL_ASIDE_FAILS=N refuses the
+   first N moves back out of a parked ".hts-old" name (#790). */
 
 #define _GNU_SOURCE
 #include <dlfcn.h>
@@ -23,20 +24,29 @@ SHIM_EXPORT int rename(const char *oldpath, const char *newpath) {
   static int aside_failures = 0;
   const char *const mode = getenv("RENAMEFAIL_MODE");
   const char *const aside_fails = getenv("RENAMEFAIL_ASIDE_FAILS");
-  const int locked = mode != NULL && strcmp(mode, "locked") == 0;
+  const char *const slash = strrchr(oldpath, '/');
+  const char *const base = slash != NULL ? slash + 1 : oldpath;
+  const int repaired = strcmp(base, "repair.zip") == 0;
   struct stat st;
 
-  if (locked) {
+  if (mode != NULL && (strcmp(mode, "locked") == 0 ||
+                       (repaired && strcmp(mode, "repair") == 0))) {
     errno = EACCES;
     return -1;
   }
-  if (aside_fails != NULL && strstr(oldpath, ".hts-old") != NULL &&
+  if (repaired && mode != NULL && strcmp(mode, "repair-eexist") == 0) {
+    errno = stat(newpath, &st) == 0 ? EEXIST : EACCES;
+    return -1;
+  }
+  if (aside_fails != NULL && strstr(base, ".hts-old") != NULL &&
       aside_failures < atoi(aside_fails)) {
     aside_failures++;
     errno = EACCES;
     return -1;
   }
-  if (aside_fails == NULL && stat(newpath, &st) == 0) {
+  /* Only the bare shim emulates Windows for every rename; a narrowed mode
+     leaves the rest of the run on plain POSIX semantics. */
+  if (mode == NULL && aside_fails == NULL && stat(newpath, &st) == 0) {
     errno = EEXIST;
     return -1;
   }