From 0e6285c87b66412f081d68d283905483f4b80ed4 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 10:25:08 +0200 Subject: [PATCH 1/5] hts_rename_over() deleted its destination when the source was missing The unlink-then-rename fallback exists for Windows, whose rename() refuses an existing target. It fired on any failed rename, so a caller asking to move a temp file it never managed to write lost the destination and got HTS_FALSE back, which reads as "nothing happened". Gate the unlink on the failure it was added for: EACCES/EEXIST, and a source that exists. The CRT does not promise ENOENT over EACCES when both hold, so the source check is not redundant. hts_rename_utf8() now preserves errno across its free() calls, which the gate depends on. Closes #779 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htslib.c | 4 ++ src/htsselftest.c | 89 +++++++++++++++++++++++++++++++++ src/htstools.c | 9 +++- src/htstools.h | 3 +- tests/01_engine-renameover.test | 29 +++++++++++ tests/Makefile.am | 11 +++- tests/renamefail.c | 32 ++++++++++++ 7 files changed, 174 insertions(+), 3 deletions(-) create mode 100644 tests/01_engine-renameover.test create mode 100644 tests/renamefail.c diff --git a/src/htslib.c b/src/htslib.c index 34a7e2d2..8462f646 100644 --- a/src/htslib.c +++ b/src/htslib.c @@ -6648,9 +6648,13 @@ 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); + /* Callers decide from errno whether the target was in the way (#779), and + free() is not required to leave it alone. */ + const int err = errno; free(woldpath); free(wnewpath); + errno = err; return result; } else { if (woldpath != NULL) diff --git a/src/htsselftest.c b/src/htsselftest.c index 371e1e64..5b1731c4 100644 --- a/src/htsselftest.c +++ b/src/htsselftest.c @@ -5955,6 +5955,91 @@ 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 : hts_rename_over() must replace an existing dst, and +// must never delete a dst it did not replace (#779). POSIX rename() clobbers on +// its own, so the unlink fallback is only reached under the LD_PRELOAD shim the +// test also runs this under. +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"); + + /* Tell the harness whether the unlink fallback is reachable at all here: it + is dead unless rename() refuses an existing target, as Windows' does. */ + (void) UNLINK(src); + (void) UNLINK(dst); + ro_put(src, "probe"); + ro_put(dst, "probe"); + if (RENAME(src, dst) != 0) + printf("renameover: fallback exercised, rename refuses an existing " + "target\n"); + + /* An existing dst must still be replaced: that is what the unlink is for. */ + (void) UNLINK(src); + (void) UNLINK(dst); + ro_put(src, "new"); + ro_put(dst, "old"); + 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++; + } + + /* 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 : 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 +6688,10 @@ static const struct selftest_entry { {"mirrorio", "", "round-trip a long+non-ASCII path through the mirror I/O wrappers", st_mirrorio}, + {"renameover", "", + "hts_rename_over(): replace dst, but never delete a dst it did not " + "replace", + st_renameover}, {"direnum", "", "enumerate a long+non-ASCII directory through opendir/readdir", st_direnum}, diff --git a/src/htstools.c b/src/htstools.c index 8cb159e2..30f888e1 100644 --- a/src/htstools.c +++ b/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. */ + /* Unlink only for the failure the fallback exists for: a dst in the way, + which Windows' rename() reports as EACCES (POSIX replaces it silently). A + src that was never written must leave dst alone, whatever errno says -- + the CRT does not promise ENOENT over EACCES when both hold. */ + const int err = errno; + + if ((err != EACCES && err != EEXIST) || !fexist_utf8(src)) + return HTS_FALSE; (void) UNLINK(cdst); return RENAME(csrc, cdst) == 0 ? HTS_TRUE : HTS_FALSE; } diff --git a/src/htstools.h b/src/htstools.h index 0548bfbc..9135a71c 100644 --- a/src/htstools.h +++ b/src/htstools.h @@ -138,7 +138,8 @@ 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 never removed unless it is replaced, so a caller + whose src was never written keeps its dst. */ hts_boolean hts_rename_over(const char *src, const char *dst); #endif diff --git a/tests/01_engine-renameover.test b/tests/01_engine-renameover.test new file mode 100644 index 00000000..678b8737 --- /dev/null +++ b/tests/01_engine-renameover.test @@ -0,0 +1,29 @@ +#!/bin/bash +# + +set -euo pipefail + +# Drives -#test=renameover: hts_rename_over() must replace an existing dst, and +# must leave dst alone when src is missing (#779). The second run interposes a +# rename() with the Windows shape (refuses an existing target with EACCES), +# which is the only way to reach the unlink fallback on POSIX. +dir=$(mktemp -d) +trap 'rm -rf "$dir"' EXIT + +httrack -O /dev/null -#test=renameover "$dir" | grep -q "renameover: OK" + +if [ "$(uname -s)" != "Linux" ]; then + echo "renameover: LD_PRELOAD interposition is Linux-only here, skipping" + exit 0 +fi +# Not a skip on Linux: a missing module would make the leg vacuous. +if [ ! -r "${RENAMEFAIL_LIB:-}" ]; then + echo "renameover: ${RENAMEFAIL_LIB:-\$RENAMEFAIL_LIB} was not built" >&2 + exit 1 +fi + +out=$(LD_PRELOAD="$RENAMEFAIL_LIB" httrack -O /dev/null \ + -#test=renameover "$dir") +echo "$out" | grep -q "renameover: OK" +# A shim that never fired would make the leg vacuous. +echo "$out" | grep -q "renameover: fallback exercised" diff --git a/tests/Makefile.am b/tests/Makefile.am index cff2c58a..1441fa41 100644 --- a/tests/Makefile.am +++ b/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,14 @@ 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_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 +82,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 \ diff --git a/tests/renamefail.c b/tests/renamefail.c new file mode 100644 index 00000000..0b2e96ef --- /dev/null +++ b/tests/renamefail.c @@ -0,0 +1,32 @@ +/* LD_PRELOAD shim giving POSIX rename() the Windows shape: it refuses an + existing target with EACCES, and reports that ahead of a missing source. + Without it hts_rename_over()'s unlink fallback is unreachable on POSIX. */ + +#define _GNU_SOURCE +#include +#include +#include +#include + +/* 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; + struct stat st; + + if (stat(newpath, &st) == 0) { + errno = EACCES; + 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); +} From e20f906900ae3313ecf9912c1c1e182a3c5c2ab7 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 10:36:26 +0200 Subject: [PATCH 2/5] Do not promise more than the unlink fallback can deliver The contract said dst is never removed unless it is replaced. The retry after the unlink can still fail, so say what actually holds and point at #790. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htstools.c | 6 +++--- src/htstools.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/htstools.c b/src/htstools.c index 30f888e1..1e9681a9 100644 --- a/src/htstools.c +++ b/src/htstools.c @@ -1452,9 +1452,9 @@ hts_boolean hts_rename_over(const char *src, const char *dst) { if (RENAME(csrc, cdst) == 0) return HTS_TRUE; /* Unlink only for the failure the fallback exists for: a dst in the way, - which Windows' rename() reports as EACCES (POSIX replaces it silently). A - src that was never written must leave dst alone, whatever errno says -- - the CRT does not promise ENOENT over EACCES when both hold. */ + which Windows' rename() reports as EACCES. The errno test alone is not + enough, the CRT does not promise ENOENT over EACCES when src is missing + too. */ const int err = errno; if ((err != EACCES && err != EEXIST) || !fexist_utf8(src)) diff --git a/src/htstools.h b/src/htstools.h index 9135a71c..7756a79e 100644 --- a/src/htstools.h +++ b/src/htstools.h @@ -138,8 +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. dst is never removed unless it is replaced, so a caller - whose src was never written keeps its dst. */ + 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 From ad9030f6133579c7c7c004fa03c8c045b4ab964b Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 10:40:08 +0200 Subject: [PATCH 3/5] Gate the unlink on EEXIST, and test all three rename regimes Review of the first commit found the gate too wide: the CRT maps ERROR_ALREADY_EXISTS to EEXIST and reserves EACCES for a source another process holds, so accepting EACCES would remove dst for a failure dst had no part in, and the retry would then fail with dst already gone. The selftest now probes what rename() does to an existing target and asserts against the regime it finds, so the interposed legs cover the fallback and a failure the unlink cannot fix. Follow-up in #791. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htslib.c | 3 +-- src/htsselftest.c | 47 +++++++++++++++++++++------------ src/htstools.c | 10 +++---- src/htstools.h | 4 +-- tests/01_engine-renameover.test | 27 ++++++++++++++----- tests/renamefail.c | 17 +++++++++--- 6 files changed, 71 insertions(+), 37 deletions(-) diff --git a/src/htslib.c b/src/htslib.c index 8462f646..ada29e86 100644 --- a/src/htslib.c +++ b/src/htslib.c @@ -6648,8 +6648,7 @@ 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); - /* Callers decide from errno whether the target was in the way (#779), and - free() is not required to leave it alone. */ + /* Save errno: callers key off it (#779) and free() may clobber it. */ const int err = errno; free(woldpath); diff --git a/src/htsselftest.c b/src/htsselftest.c index 5b1731c4..0576cb79 100644 --- a/src/htsselftest.c +++ b/src/htsselftest.c @@ -5976,10 +5976,9 @@ static hts_boolean ro_is(const char *path, const char *data) { return n == strlen(data) && memcmp(buf, data, n) == 0 ? HTS_TRUE : HTS_FALSE; } -// -#test=renameover : hts_rename_over() must replace an existing dst, and -// must never delete a dst it did not replace (#779). POSIX rename() clobbers on -// its own, so the unlink fallback is only reached under the LD_PRELOAD shim the -// test also runs this under. +// -#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 +// 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) { @@ -5992,28 +5991,42 @@ static int st_renameover(httrackp *opt, int argc, char **argv) { fconcat(src, sizeof(src), argv[0], "renameover-src.bin"); fconcat(dst, sizeof(dst), argv[0], "renameover-dst.bin"); - /* Tell the harness whether the unlink fallback is reachable at all here: it - is dead unless rename() refuses an existing target, as Windows' does. */ (void) UNLINK(src); (void) UNLINK(dst); ro_put(src, "probe"); ro_put(dst, "probe"); - if (RENAME(src, dst) != 0) - printf("renameover: fallback exercised, rename refuses an existing " - "target\n"); - /* An existing dst must still be replaced: that is what the unlink is for. */ + 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 (!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++; + 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. */ diff --git a/src/htstools.c b/src/htstools.c index 30f888e1..5e55e673 100644 --- a/src/htstools.c +++ b/src/htstools.c @@ -1451,13 +1451,13 @@ hts_boolean hts_rename_over(const char *src, const char *dst) { fconv(cdst, sizeof(cdst), dst); if (RENAME(csrc, cdst) == 0) return HTS_TRUE; - /* Unlink only for the failure the fallback exists for: a dst in the way, - which Windows' rename() reports as EACCES (POSIX replaces it silently). A - src that was never written must leave dst alone, whatever errno says -- - the CRT does not promise ENOENT over EACCES when both hold. */ + /* Only a dst standing in the way is something the unlink can clear, and the + CRT reports that as EEXIST (ERROR_ALREADY_EXISTS); it keeps EACCES for a + src another process holds, where removing dst would destroy a file the + retry then cannot replace. The src check covers a CRT that disagrees. */ const int err = errno; - if ((err != EACCES && err != EEXIST) || !fexist_utf8(src)) + if (err != EEXIST || !fexist_utf8(src)) return HTS_FALSE; (void) UNLINK(cdst); return RENAME(csrc, cdst) == 0 ? HTS_TRUE : HTS_FALSE; diff --git a/src/htstools.h b/src/htstools.h index 9135a71c..d5aebd9d 100644 --- a/src/htstools.h +++ b/src/htstools.h @@ -138,8 +138,8 @@ 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 never removed unless it is replaced, so a caller - whose src was never written keeps its dst. */ + paths are fconv()'d. dst is only ever removed to make room for src, so a + failed call leaves it as it was. */ hts_boolean hts_rename_over(const char *src, const char *dst); #endif diff --git a/tests/01_engine-renameover.test b/tests/01_engine-renameover.test index 678b8737..b5fbf8f1 100644 --- a/tests/01_engine-renameover.test +++ b/tests/01_engine-renameover.test @@ -4,26 +4,39 @@ set -euo pipefail # Drives -#test=renameover: hts_rename_over() must replace an existing dst, and -# must leave dst alone when src is missing (#779). The second run interposes a -# rename() with the Windows shape (refuses an existing target with EACCES), -# which is the only way to reach the unlink fallback on POSIX. +# 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 -httrack -O /dev/null -#test=renameover "$dir" | grep -q "renameover: OK" +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 -# Not a skip on Linux: a missing module would make the leg vacuous. +# Not a skip on Linux: a missing module would make the interposed legs vacuous. if [ ! -r "${RENAMEFAIL_LIB:-}" ]; then echo "renameover: ${RENAMEFAIL_LIB:-\$RENAMEFAIL_LIB} was not built" >&2 exit 1 fi +# 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" -# A shim that never fired would make the leg vacuous. -echo "$out" | grep -q "renameover: fallback exercised" +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" diff --git a/tests/renamefail.c b/tests/renamefail.c index 0b2e96ef..ac169bee 100644 --- a/tests/renamefail.c +++ b/tests/renamefail.c @@ -1,11 +1,14 @@ -/* LD_PRELOAD shim giving POSIX rename() the Windows shape: it refuses an - existing target with EACCES, and reports that ahead of a missing source. - Without it hts_rename_over()'s unlink fallback is unreachable on POSIX. */ +/* 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 #include #include +#include +#include #include /* The tree builds with -fvisibility=hidden, which would hide the interposer. */ @@ -15,12 +18,18 @@ 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 (stat(newpath, &st) == 0) { + 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) { From e0f1334b41e269fb433c500350680c5f7b008b66 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 10:43:35 +0200 Subject: [PATCH 4/5] warc_swap_segments: state the real reason the temps are checked first The all-or-nothing precheck was documented as a workaround for hts_rename_over deleting its destination, which it no longer does. The loop still earns its place: a swap stopping halfway mixes two runs' segments. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htswarc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/htswarc.c b/src/htswarc.c index 3d7952d7..4cf3efe7 100644 --- a/src/htswarc.c +++ b/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, From be28791c44d367713581f364ef1ee957bb350f99 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 10:50:29 +0200 Subject: [PATCH 5/5] Make the interposed test legs survive ASan and a static build Two ways the LD_PRELOAD legs broke outside a plain Linux build: ASan aborts when a preloaded library loads ahead of its runtime, and --disable-shared (what the MSan job uses) builds no module to preload at all. The first is safe to waive here, the shim allocates nothing. The second is a real skip rather than a failure, told apart from a missing module by libtool's own dlname, so a build that should have one still fails loudly. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- tests/01_engine-renameover.test | 15 ++++++++++++++- tests/Makefile.am | 1 + 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/01_engine-renameover.test b/tests/01_engine-renameover.test index b5fbf8f1..dcf6a7c5 100644 --- a/tests/01_engine-renameover.test +++ b/tests/01_engine-renameover.test @@ -23,12 +23,25 @@ if [ "$(uname -s)" != "Linux" ]; then echo "renameover: LD_PRELOAD interposition is Linux-only here, skipping" exit 0 fi -# Not a skip on Linux: a missing module would make the interposed legs vacuous. +# 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") diff --git a/tests/Makefile.am b/tests/Makefile.am index 8a98ce28..3168695d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -21,6 +21,7 @@ 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