diff --git a/src/htslib.c b/src/htslib.c index 34a7e2d2..ada29e86 100644 --- a/src/htslib.c +++ b/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) diff --git a/src/htsselftest.c b/src/htsselftest.c index 371e1e64..0576cb79 100644 --- a/src/htsselftest.c +++ b/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 : 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 : 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", "", "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..0d1e6f3a 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. */ + /* 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; } diff --git a/src/htstools.h b/src/htstools.h index 0548bfbc..7756a79e 100644 --- a/src/htstools.h +++ b/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 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, diff --git a/tests/01_engine-renameover.test b/tests/01_engine-renameover.test new file mode 100644 index 00000000..dcf6a7c5 --- /dev/null +++ b/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" diff --git a/tests/Makefile.am b/tests/Makefile.am index fe1bca09..3168695d 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,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 \ diff --git a/tests/renamefail.c b/tests/renamefail.c new file mode 100644 index 00000000..ac169bee --- /dev/null +++ b/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 +#include +#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; + 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); +}