From 38161162e24b24ed53e33b100bfd5938eeb2b133 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 13:39:56 +0200 Subject: [PATCH 1/3] hts_rename_over() can lose its destination when the retried rename fails The unlink-then-rename fallback leaves nothing in place of dst between the unlink and the retry, so a retry that fails too loses it. Park dst under a free scratch name instead, drop it once the move succeeded, and put it back otherwise. Closes #790 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htsselftest.c | 56 ++++++++++++++++++++++++++++++++- src/htstools.c | 52 ++++++++++++++++++++++++++---- src/htstools.h | 9 ++++-- tests/01_engine-renameover.test | 6 ++-- tests/renamefail.c | 2 +- 5 files changed, 111 insertions(+), 14 deletions(-) diff --git a/src/htsselftest.c b/src/htsselftest.c index 0ae3a3dd..18f68fa8 100644 --- a/src/htsselftest.c +++ b/src/htsselftest.c @@ -6137,7 +6137,7 @@ static hts_boolean ro_is(const char *path, const char *data) { } // -#test=renameover : 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; @@ -6208,6 +6208,60 @@ static int st_renameover(httrackp *opt, int argc, char **argv) { 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(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). dst must come back, not vanish. */ + (void) UNLINK(src); + ro_put(dst, "old"); + if (hts_rename_over_aside_selftest(src, dst)) { + fprintf(stderr, "renameover: a failed aside retry reported success\n"); + err++; + } + if (!ro_is(dst, "old")) { + fprintf(stderr, "renameover: a failed aside retry lost dst\n"); + err++; + } + if (fexist_utf8(aside)) { + fprintf(stderr, + "renameover: a failed aside retry left the parked 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(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); + } + + (void) UNLINK(src); (void) UNLINK(dst); printf("renameover: %s\n", err ? "FAIL" : "OK"); return err; diff --git a/src/htstools.c b/src/htstools.c index 61de0300..e37c4430 100644 --- a/src/htstools.c +++ b/src/htstools.c @@ -1438,6 +1438,39 @@ HTSEXT_API hts_boolean hts_findissystem(find_handle find) { return 0; } +/* Free scratch name beside dst to park it under. */ +static hts_boolean rename_aside_name(char *dest, size_t size, const char *dst) { + int i; + + for (i = 0; i < 16; i++) { + if ((size_t) snprintf(dest, size, "%s.hts-old%d", dst, i) >= size) + return HTS_FALSE; + if (!fexist(dest)) + return HTS_TRUE; + } + return HTS_FALSE; +} + +/* dst 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(const char *csrc, const char *cdst) { + char caside[CATBUFF_SIZE]; + int err; + + if (!rename_aside_name(caside, sizeof(caside), cdst)) + return HTS_FALSE; + if (RENAME(cdst, caside) != 0) + return HTS_FALSE; + if (RENAME(csrc, cdst) == 0) { + (void) UNLINK(caside); + return HTS_TRUE; + } + err = errno; + (void) RENAME(caside, cdst); + errno = err; + return HTS_FALSE; +} + hts_boolean hts_rename_over(const char *src, const char *dst) { char csrc[CATBUFF_SIZE], cdst[CATBUFF_SIZE]; @@ -1445,14 +1478,21 @@ hts_boolean hts_rename_over(const char *src, const char *dst) { 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(csrc, cdst); +} + +hts_boolean hts_rename_over_aside_selftest(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(csrc, cdst); } diff --git a/src/htstools.h b/src/htstools.h index 7756a79e..9f79181d 100644 --- a/src/htstools.h +++ b/src/htstools.h @@ -138,11 +138,14 @@ 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). */ + paths are fconv()'d. A dst in the way is moved aside rather than removed, so + a failure at any point leaves dst as it was. */ hts_boolean hts_rename_over(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(const char *src, const char *dst); + #endif #endif diff --git a/tests/01_engine-renameover.test b/tests/01_engine-renameover.test index 90b1057f..327a2a71 100644 --- a/tests/01_engine-renameover.test +++ b/tests/01_engine-renameover.test @@ -4,8 +4,8 @@ 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 +# must leave dst alone when the rename failed for a reason moving dst aside +# cannot fix (#779, #790). 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 'set +e; rm -rf "$dir"' EXIT @@ -42,7 +42,7 @@ 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") echo "$out" | grep -q "renameover: OK" diff --git a/tests/renamefail.c b/tests/renamefail.c index ac169bee..d10ed0ea 100644 --- a/tests/renamefail.c +++ b/tests/renamefail.c @@ -1,5 +1,5 @@ /* Borrows Windows' rename() for 01_engine-renameover.test, since POSIX cannot - reach hts_rename_over()'s unlink fallback: an existing target is refused with + reach hts_rename_over()'s aside 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. */ From ea6aafb210f816f049954f7754d13c99bb6cbc17 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 14:32:36 +0200 Subject: [PATCH 2/3] Review fixes: check the restore, probe in UTF-8, state the honest guarantee The move back out of the parked name was unchecked, so a retry that failed for a reason that still applied left dst absent with the content orphaned under a name nothing reported. Check it, retry once, and name the parked copy in the log; hts_rename_over() takes an httrackp for that. The aside probe used fexist(), which is not UTF-8 and consults the ANSI codepage on Windows while the renames beside it are wide. It also reads a directory as a free name, so the park now skips a name whose rename refuses rather than giving up on it. The header claimed a failure leaves dst as it was, which the crash window between the two renames does not give. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htsback.c | 6 ++-- src/htscache.c | 1 + src/htsselftest.c | 54 +++++++++++++++++++++++---------- src/htssinglefile.c | 2 +- src/htstools.c | 38 ++++++++++++++--------- src/htstools.h | 13 +++++--- src/htswarc.c | 4 +-- tests/01_engine-renameover.test | 20 ++++++++++-- tests/renamefail.c | 14 +++++++-- 9 files changed, 107 insertions(+), 45 deletions(-) diff --git a/src/htsback.c b/src/htsback.c index d8ace590..a4cc0dbc 100644 --- a/src/htsback.c +++ b/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); diff --git a/src/htscache.c b/src/htscache.c index 9d1b86e0..d8398089 100644 --- a/src/htscache.c +++ b/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), diff --git a/src/htsselftest.c b/src/htsselftest.c index 18f68fa8..ba60a8d7 100644 --- a/src/htsselftest.c +++ b/src/htsselftest.c @@ -6140,7 +6140,6 @@ static hts_boolean ro_is(const char *path, const char *data) { // 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++; } @@ -6192,7 +6191,7 @@ static int st_renameover(httrackp *opt, int argc, char **argv) { /* 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,7 +6202,7 @@ 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++; } @@ -6219,7 +6218,7 @@ static int st_renameover(httrackp *opt, int argc, char **argv) { (void) UNLINK(keep); ro_put(src, "new"); ro_put(dst, "old"); - if (!hts_rename_over_aside_selftest(src, dst)) { + if (!hts_rename_over_aside_selftest(opt, src, dst)) { fprintf(stderr, "renameover: the aside fallback failed: %s\n", strerror(errno)); err++; @@ -6228,27 +6227,30 @@ static int st_renameover(httrackp *opt, int argc, char **argv) { err++; } - /* #790: the retry fails (no src). dst must come back, not vanish. */ + /* #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(src, dst)) { + 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")) { - fprintf(stderr, "renameover: a failed aside retry lost dst\n"); - err++; - } - if (fexist_utf8(aside)) { - fprintf(stderr, - "renameover: a failed aside retry left the parked copy\n"); + 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(src, dst)) { + if (!hts_rename_over_aside_selftest(opt, src, dst)) { fprintf(stderr, "renameover: a taken aside name failed the move: %s\n", strerror(errno)); err++; @@ -6259,6 +6261,26 @@ static int st_renameover(httrackp *opt, int argc, char **argv) { } (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); diff --git a/src/htssinglefile.c b/src/htssinglefile.c index a65b9de2..ea83050a 100644 --- a/src/htssinglefile.c +++ b/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); diff --git a/src/htstools.c b/src/htstools.c index e37c4430..68bd0ade 100644 --- a/src/htstools.c +++ b/src/htstools.c @@ -1438,40 +1438,49 @@ HTSEXT_API hts_boolean hts_findissystem(find_handle find) { return 0; } -/* Free scratch name beside dst to park it under. */ -static hts_boolean rename_aside_name(char *dest, size_t size, 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 ((size_t) snprintf(dest, size, "%s.hts-old%d", dst, i) >= size) + if (!slprintfbuff(caside, size, "%s.hts-old%d", cdst, i)) return HTS_FALSE; - if (!fexist(dest)) + /* 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; } -/* dst is in the way of the move: park it, retry, and put it back if the retry +/* 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(const char *csrc, const char *cdst) { +static hts_boolean rename_over_aside(httrackp *opt, const char *csrc, + const char *cdst) { char caside[CATBUFF_SIZE]; int err; - if (!rename_aside_name(caside, sizeof(caside), cdst)) - return HTS_FALSE; - if (RENAME(cdst, caside) != 0) + if (!rename_park_aside(caside, sizeof(caside), cdst)) return HTS_FALSE; if (RENAME(csrc, cdst) == 0) { (void) UNLINK(caside); return HTS_TRUE; } err = errno; - (void) RENAME(caside, cdst); + /* 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(const char *src, const char *dst) { +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); @@ -1486,13 +1495,14 @@ hts_boolean hts_rename_over(const char *src, const char *dst) { if (err != EEXIST || !fexist_utf8(src)) return HTS_FALSE; - return rename_over_aside(csrc, cdst); + return rename_over_aside(opt, csrc, cdst); } -hts_boolean hts_rename_over_aside_selftest(const char *src, const char *dst) { +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(csrc, cdst); + return rename_over_aside(opt, csrc, cdst); } diff --git a/src/htstools.h b/src/htstools.h index 9f79181d..484bfa97 100644 --- a/src/htstools.h +++ b/src/htstools.h @@ -137,14 +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. A dst in the way is moved aside rather than removed, so - a failure at any point leaves dst as it was. */ -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(const char *src, const char *dst); +hts_boolean hts_rename_over_aside_selftest(httrackp *opt, const char *src, + const char *dst); #endif diff --git a/src/htswarc.c b/src/htswarc.c index c3cb1c45..33851f50 100644 --- a/src/htswarc.c +++ b/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; diff --git a/tests/01_engine-renameover.test b/tests/01_engine-renameover.test index 327a2a71..03027a71 100644 --- a/tests/01_engine-renameover.test +++ b/tests/01_engine-renameover.test @@ -5,8 +5,8 @@ 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 moving dst aside -# cannot fix (#779, #790). The selftest prints the regime it detected; pin it per platform so -# a leg cannot pass having tested the other half. +# 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") echo "$out" | grep -q "renameover: OK" echo "$out" | grep -q "renameover: regime $want" +echo "$out" | grep -q "renameover: restore back" if [ "$(uname -s)" != "Linux" ]; then echo "renameover: LD_PRELOAD interposition is Linux-only here, skipping" @@ -47,6 +48,21 @@ 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" +echo "$out" | grep -q "renameover: restore back" + +# #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") +echo "$out" | grep -q "renameover: OK" +echo "$out" | grep -q "renameover: restore back" + +# 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") +echo "$out" | grep -q "renameover: OK" +echo "$out" | grep -q "renameover: restore parked" # 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 \ diff --git a/tests/renamefail.c b/tests/renamefail.c index d10ed0ea..ae35e7c2 100644 --- a/tests/renamefail.c +++ b/tests/renamefail.c @@ -1,7 +1,9 @@ /* Borrows Windows' rename() for 01_engine-renameover.test, since POSIX cannot reach hts_rename_over()'s aside 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. */ + 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). */ #define _GNU_SOURCE #include @@ -18,7 +20,9 @@ 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; + 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; struct stat st; @@ -26,7 +30,13 @@ SHIM_EXPORT int rename(const char *oldpath, const char *newpath) { errno = EACCES; return -1; } - if (stat(newpath, &st) == 0) { + if (aside_fails != NULL && strstr(oldpath, ".hts-old") != NULL && + aside_failures < atoi(aside_fails)) { + aside_failures++; + errno = EACCES; + return -1; + } + if (aside_fails == NULL && stat(newpath, &st) == 0) { errno = EEXIST; return -1; } From 1b3f7918b6911fb734af1785a1f4623c8eaa588c Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 16:03:13 +0200 Subject: [PATCH 3/3] The aside fallback parked a directory that stood in the way Windows refuses every rename onto an existing target, so the fallback is production code there rather than the rare path it is on POSIX. A directory at the destination was renamed aside like a file, the move then succeeded, and UNLINK could not drop the parked directory, so the call reported success where master had reported failure and left an orphan behind. 101_local-update-stale-bak plants exactly that shape and caught it on both Windows legs. Park a regular file only. A directory in the way is refused, as before. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htsselftest.c | 26 ++++++++++++++++++++++++++ src/htstools.c | 4 ++++ 2 files changed, 30 insertions(+) diff --git a/src/htsselftest.c b/src/htsselftest.c index ba60a8d7..7fc3ffee 100644 --- a/src/htsselftest.c +++ b/src/htsselftest.c @@ -6188,6 +6188,32 @@ 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"); diff --git a/src/htstools.c b/src/htstools.c index 68bd0ade..b13c166a 100644 --- a/src/htstools.c +++ b/src/htstools.c @@ -1463,6 +1463,10 @@ static hts_boolean rename_over_aside(httrackp *opt, const char *csrc, 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) {