diff --git a/man/httrack.1 b/man/httrack.1 index 96c3f3a0..33f729b6 100644 --- a/man/httrack.1 +++ b/man/httrack.1 @@ -3,7 +3,7 @@ .\" .\" This file is generated by man/makeman.sh; do not edit by hand. .\" SPDX-License-Identifier: GPL-3.0-or-later -.TH httrack 1 "14 July 2026" "httrack website copier" +.TH httrack 1 "16 July 2026" "httrack website copier" .SH NAME httrack \- offline browser : copy websites to a local directory .SH SYNOPSIS @@ -165,7 +165,7 @@ pause transfer if N bytes reached, and wait until lock file is deleted (\-\-max\ .IP \-cN number of multiple connections (*c8) (\-\-sockets[=N]) .IP \-TN -timeout, number of seconds after a non\-responding link is shutdown (\-\-timeout[=N]) +timeout, number of seconds after a non\-responding link is shutdown; also bounds host name resolution (\-\-timeout[=N]) .IP \-RN number of retries, in case of timeout or non\-fatal errors (*R1) (\-\-retries[=N]) .IP \-JN diff --git a/src/htsdns_selftest.c b/src/htsdns_selftest.c index 101eb2e9..3673fe4e 100644 --- a/src/htsdns_selftest.c +++ b/src/htsdns_selftest.c @@ -63,9 +63,14 @@ typedef struct mock_host { int gai_err; /* non-zero: getaddrinfo returns this */ int naddr; mock_addr addr[6]; - int calls; /* times the backend resolved this host */ + int calls; /* times the backend resolved this host */ + int slow_ms; /* non-zero: block this long, as a black-hole resolver would */ } mock_host; +/* Long enough to outlast the 1s --timeout the bounded resolve is checked + against, short enough to keep the self-test quick. */ +#define MOCK_SLOW_MS 3000 + static mock_host mock_hosts[] = { {"v4only.test", 0, 1, {{AF_INET, {1, 2, 3, 4}}}, 0}, {"v6only.test", 0, 1, {{AF_INET6, {0x20, 0x01, 0x0d, 0xb8, [15] = 1}}}, 0}, @@ -95,8 +100,15 @@ static mock_host mock_hosts[] = { {AF_INET, {10, 0, 0, 6}}}, 0}, {"nodns.test", EAI_NONAME, 0, {{0}}, 0}, + /* resolves, but only well after --timeout: the #606 wedge */ + {"slow.test", 0, 1, {{AF_INET, {127, 0, 0, 9}}}, 0, MOCK_SLOW_MS}, }; +/* Serializes mock_host bookkeeping: a timed-out resolve is abandoned, so its + worker is still inside the backend while the test reads the counters. */ +static htsmutex mock_lock = HTSMUTEX_INIT; +static int mock_finished = 0; /* backend calls that have returned */ + static mock_host *mock_find(const char *name) { for (size_t i = 0; i < sizeof(mock_hosts) / sizeof(mock_hosts[0]); i++) { if (strcmp(mock_hosts[i].name, name) == 0) @@ -106,8 +118,34 @@ static mock_host *mock_find(const char *name) { } static void mock_reset_calls(void) { + hts_mutexlock(&mock_lock); for (size_t i = 0; i < sizeof(mock_hosts) / sizeof(mock_hosts[0]); i++) mock_hosts[i].calls = 0; + mock_finished = 0; + hts_mutexrelease(&mock_lock); +} + +static int mock_read_calls(const char *name) { + int calls; + + hts_mutexlock(&mock_lock); + calls = mock_find(name)->calls; + hts_mutexrelease(&mock_lock); + return calls; +} + +/* Wait for n backend calls to return, ordering their writes against ours. */ +static void mock_wait_finished(int n) { + for (;;) { + hts_boolean done; + + hts_mutexlock(&mock_lock); + done = (mock_finished >= n) ? HTS_TRUE : HTS_FALSE; + hts_mutexrelease(&mock_lock); + if (done) + break; + Sleep(10); + } } /* Build one addrinfo node owning its sockaddr (freed by mock_freeaddrinfo). */ @@ -133,10 +171,10 @@ static struct addrinfo *mock_mkai(const mock_addr *a) { return ai; } -static int HTS_RESOLVER_CALL mock_getaddrinfo(const char *node, - const char *service, - const struct addrinfo *hints, - struct addrinfo **res) { +static int HTS_RESOLVER_CALL mock_getaddrinfo_(const char *node, + const char *service, + const struct addrinfo *hints, + struct addrinfo **res) { mock_host *const h = mock_find(node); const int want = (hints != NULL) ? hints->ai_family : PF_UNSPEC; struct addrinfo *head = NULL, *tail = NULL; @@ -145,7 +183,11 @@ static int HTS_RESOLVER_CALL mock_getaddrinfo(const char *node, *res = NULL; if (h == NULL) return EAI_NONAME; + hts_mutexlock(&mock_lock); h->calls++; /* a real backend hit; a cached host skips this */ + hts_mutexrelease(&mock_lock); + if (h->slow_ms != 0) + Sleep(h->slow_ms); if (h->gai_err != 0) return h->gai_err; for (int i = 0; i < h->naddr; i++) { @@ -165,6 +207,18 @@ static int HTS_RESOLVER_CALL mock_getaddrinfo(const char *node, return 0; } +static int HTS_RESOLVER_CALL mock_getaddrinfo(const char *node, + const char *service, + const struct addrinfo *hints, + struct addrinfo **res) { + const int ret = mock_getaddrinfo_(node, service, hints, res); + + hts_mutexlock(&mock_lock); + mock_finished++; + hts_mutexrelease(&mock_lock); + return ret; +} + static void HTS_RESOLVER_CALL mock_freeaddrinfo(struct addrinfo *res) { while (res != NULL) { struct addrinfo *const next = res->ai_next; @@ -370,6 +424,91 @@ int dns_selftests(httrackp *opt) { return failures; } +/* Probes how long acquiring opt->state.lock takes while a resolve is in + flight. hts_has_stopped() takes that same lock and mutates nothing, and the + API promises it stays callable from another thread during a mirror. */ +typedef struct lock_probe { + httrackp *opt; + htsmutex lock; + hts_boolean done; + TStamp blocked_ms; +} lock_probe; + +static void lock_probe_thread(void *arg) { + lock_probe *const p = (lock_probe *) arg; + TStamp start; + + Sleep(MOCK_SLOW_MS / 10); /* let the resolve get under way first */ + start = mtime_local(); + (void) hts_has_stopped(p->opt); + hts_mutexlock(&p->lock); + p->blocked_ms = mtime_local() - start; + p->done = HTS_TRUE; + hts_mutexrelease(&p->lock); +} + +int dns_timeout_selftests(httrackp *opt) { + SOCaddr addrs[HTS_MAXADDRNUM]; + const char *err = NULL; + lock_probe probe; + TStamp start, elapsed; + int count; + + failures = 0; + hts_dns_set_resolver_backend(&mock_backend); + IPV6_resolver = 0; + mock_reset_calls(); + opt->timeout = 1; /* the bound under test */ + + memset(&probe, 0, sizeof(probe)); + probe.opt = opt; + probe.lock = HTSMUTEX_INIT; + hts_mutexinit(&probe.lock); + CHECK(hts_newthread(lock_probe_thread, &probe) == 0); + + start = mtime_local(); + count = hts_dns_resolve_all(opt, "slow.test", addrs, HTS_MAXADDRNUM, &err); + elapsed = mtime_local() - start; + + /* the resolve returns on opt->timeout, not when the resolver deigns to + answer: this is what lets --max-time and --timeout fire (#606). The bound + is derived from opt->timeout, never from the mock's sleep, or a resolve + that ignored opt->timeout would still pass under the mock. */ + CHECK(elapsed < (TStamp) opt->timeout * 1000 + 500); + CHECK(count == 0); /* a timeout is reported as "does not resolve" */ + + /* state.lock is not held across the resolve; a concurrent stop query, which + the mirror API promises stays live, is not blocked behind it */ + for (;;) { + hts_boolean done; + TStamp blocked; + + hts_mutexlock(&probe.lock); + done = probe.done; + blocked = probe.blocked_ms; + hts_mutexrelease(&probe.lock); + if (done) { + CHECK(blocked < 500); + break; + } + Sleep(20); + } + hts_mutexfree(&probe.lock); + + /* a timeout is not an answer, so it must not be negative-cached: the host is + resolved again rather than written off for the rest of the crawl */ + CHECK(mock_read_calls("slow.test") == 1); + count = hts_dns_resolve_all(opt, "slow.test", addrs, HTS_MAXADDRNUM, &err); + CHECK(count == 0); + CHECK(mock_read_calls("slow.test") == 2); /* re-resolved, not cached */ + + /* Both resolves were abandoned mid-backend; wait for their workers to leave + it before returning. The backend stays installed: an abandoned worker + still reads it (to free its addrinfo) after the last call returns. */ + mock_wait_finished(2); + return failures; +} + #else int dns_selftests(httrackp *opt) { diff --git a/src/htsdns_selftest.h b/src/htsdns_selftest.h index 72b34d8c..74a6062b 100644 --- a/src/htsdns_selftest.h +++ b/src/htsdns_selftest.h @@ -46,6 +46,12 @@ typedef struct httrackp httrackp; Returns the number of failed checks (0 == success). */ int dns_selftests(httrackp *opt); +/* Drive a deliberately slow (mock) resolver, asserting that a resolve is + bounded by opt->timeout, does not hold opt->state.lock while it runs, and + does not cache a timeout as an answer (#606). Takes a few seconds. + Returns the number of failed checks (0 == success). */ +int dns_timeout_selftests(httrackp *opt); + #endif #endif diff --git a/src/htshelp.c b/src/htshelp.c index 9d3db084..e48921ee 100644 --- a/src/htshelp.c +++ b/src/htshelp.c @@ -496,8 +496,8 @@ void help(const char *app, int more) { infomsg(""); infomsg("Flow control:"); infomsg(" cN number of multiple connections (*c8)"); - infomsg - (" TN timeout, number of seconds after a non-responding link is shutdown"); + infomsg(" TN timeout, number of seconds after a non-responding link is" + " shutdown; also bounds host name resolution"); infomsg (" RN number of retries, in case of timeout or non-fatal errors (*R1)"); infomsg diff --git a/src/htslib.c b/src/htslib.c index 55899b26..8687152a 100644 --- a/src/htslib.c +++ b/src/htslib.c @@ -5084,50 +5084,164 @@ HTSEXT_API int check_hostname_dns(const char *const hostname) { return hts_dns_resolve_nocache(hostname, &buffer) != NULL; } -// Needs locking -// Internal DNS cache. Fill out[0..count-1] with up to max addresses for _iadr, -// resolving (and caching the full list) on a miss. Returns the count. -static int hts_dns_resolve_list_(httrackp *opt, const char *_iadr, - SOCaddr *const out, const int max, - const char **error) { - char BIGSTK iadr[HTS_URLMAXSIZE * 2]; - coucal cache = hts_cache(opt); // le cache dns +/* A resolve in flight. Refcounted: a timed-out resolve is abandoned, not + cancelled, so the last of caller/worker to leave frees the job. */ +typedef struct dns_resolve_job { + htsmutex lock; + int refcount; + hts_boolean done; + char *hostname; + SOCaddr addr[HTS_MAXADDRNUM]; int count; + const char *error; +} dns_resolve_job; + +/* Copy the first min(count, max) addresses of src into dest. */ +static void dns_copy_addrs(SOCaddr *dest, SOCaddr *src, int count, int max) { + int i; + + for (i = 0; i < count && i < max; i++) + SOCaddr_copy_SOCaddr(dest[i], src[i]); +} + +static void dns_job_release(dns_resolve_job *job) { + hts_boolean last; + + hts_mutexlock(&job->lock); + last = (--job->refcount == 0) ? HTS_TRUE : HTS_FALSE; + hts_mutexrelease(&job->lock); + if (last) { + hts_mutexfree(&job->lock); + freet(job->hostname); + freet(job); + } +} + +/* Outlives a timed-out resolve, so it writes only the job: never opt (freed + before the thread wait at exit) nor the DNS cache. */ +static void dns_resolve_thread(void *arg) { + dns_resolve_job *const job = (dns_resolve_job *) arg; + SOCaddr resolved[HTS_MAXADDRNUM]; + const char *error = NULL; + const int count = hts_dns_resolve_nocache_list(job->hostname, resolved, + HTS_MAXADDRNUM, &error); + + hts_mutexlock(&job->lock); + dns_copy_addrs(job->addr, resolved, count, HTS_MAXADDRNUM); + job->count = count; + job->error = error; + job->done = HTS_TRUE; /* published last: gates the caller's read of addr[] */ + hts_mutexrelease(&job->lock); + dns_job_release(job); +} + +/* Resolve hostname on a worker thread, giving up after timeout seconds. + Returns the address count, or -1 on timeout -- distinct from 0 ("does not + resolve"), which is a real answer and gets negative-cached. */ +static int hts_dns_resolve_nocache_list_bounded(const char *hostname, + SOCaddr *const out, + const int max, + const int timeout, + const char **error) { + dns_resolve_job *job; + TStamp deadline; + int count = -1; + int poll_ms = 1; + + if (timeout <= 0) /* no bound asked for (--timeout 0) */ + return hts_dns_resolve_nocache_list(hostname, out, max, error); + + job = calloct(1, sizeof(*job)); + assertf(job != NULL); + hts_mutexinit(&job->lock); + job->hostname = strdupt(hostname); + job->refcount = 2; /* this caller + the worker */ + if (hts_newthread(dns_resolve_thread, job) != 0) { + job->refcount = 1; /* no worker: fall back to resolving inline */ + dns_job_release(job); + return hts_dns_resolve_nocache_list(hostname, out, max, error); + } + + deadline = mtime_local() + (TStamp) timeout * 1000; + for (;;) { + hts_boolean done; + + hts_mutexlock(&job->lock); + done = job->done; + if (done) { + count = job->count; + dns_copy_addrs(out, job->addr, count, max); + if (error != NULL) + *error = job->error; + } + hts_mutexrelease(&job->lock); + if (done || mtime_local() >= deadline) + break; + Sleep(poll_ms); + if (poll_ms < 50) /* short first polls keep a fast resolve fast */ + poll_ms *= 2; + } + dns_job_release(job); + return count; +} + +int hts_dns_resolve_all(httrackp *opt, const char *iadr, SOCaddr *out, int max, + const char **error) { + char BIGSTK host[HTS_URLMAXSIZE * 2]; + SOCaddr resolved[HTS_MAXADDRNUM]; + coucal cache; + int count, i; assertf(opt != NULL); - assertf(_iadr != NULL); assertf(out != NULL); + if (!strnotempty(iadr) || max <= 0) { + return 0; + } - strcpybuff(iadr, jump_identification_const(_iadr)); - // couper éventuel : + /* cache key and resolver input: identification and any ":port" stripped */ + strcpybuff(host, jump_identification_const(iadr)); { char *a; - if ((a = jump_toport(iadr))) + if ((a = jump_toport(host))) *a = '\0'; } - /* get IP from the dns cache */ - count = hts_ghbn_all(cache, iadr, out, max); + hts_mutexlock(&opt->state.lock); + cache = hts_cache(opt); +#if HTS_INET6 != 0 + hts_resolver_check_env(); /* settle the backend before a worker reads it */ +#endif + count = hts_ghbn_all(cache, host, out, max); + hts_mutexrelease(&opt->state.lock); if (count >= 0) { // cache hit (0 == negative-cached) return count; - } else { // non présent dans le cache dns, tester - SOCaddr resolved[HTS_MAXADDRNUM]; - t_dnscache *record; - int i; + } #if DEBUGDNS - printf("resolving (not cached) %s\n", iadr); + printf("resolving (not cached) %s\n", host); #endif - count = hts_dns_resolve_nocache_list(iadr, resolved, HTS_MAXADDRNUM, error); + /* Resolve with no lock held: getaddrinfo can block for a long time, and + state.lock also gates the stop request (#606). */ + count = hts_dns_resolve_nocache_list_bounded(host, resolved, HTS_MAXADDRNUM, + opt->timeout, error); #if HTS_WIDE_DEBUG - DEBUG_W("gethostbyname done\n"); + DEBUG_W("gethostbyname done\n"); #endif - /* attempt to store new entry (coucal owns it and dups the host key) */ - record = malloct(sizeof(t_dnscache)); + if (count < 0) { /* timed out: no answer to cache, and none to report */ + if (error != NULL) + *error = "host name resolution timed out"; + return 0; + } + + hts_mutexlock(&opt->state.lock); + { /* store the full list (coucal owns the record and dups the host key; a + concurrent resolve of the same host replaces, and frees, this one) */ + t_dnscache *const record = malloct(sizeof(t_dnscache)); + if (record != NULL) { memset(record, 0, sizeof(*record)); record->host_count = count; @@ -5137,28 +5251,13 @@ static int hts_dns_resolve_list_(httrackp *opt, const char *_iadr, memcpy(record->host_addr[i], &SOCaddr_sockaddr(resolved[i]), record->host_length[i]); } - coucal_add_pvoid(cache, iadr, record); + coucal_add_pvoid(cache, host, record); } - - /* copy result to caller (cache store may have failed; result still valid) - */ - for (i = 0; i < count && i < max; i++) { - SOCaddr_copy_SOCaddr(out[i], resolved[i]); - } - return count; - } // retour hp du cache -} - -int hts_dns_resolve_all(httrackp *opt, const char *iadr, SOCaddr *out, int max, - const char **error) { - int count; - - if (!strnotempty(iadr) || max <= 0) { - return 0; } - hts_mutexlock(&opt->state.lock); - count = hts_dns_resolve_list_(opt, iadr, out, max, error); hts_mutexrelease(&opt->state.lock); + + /* copy result to caller (cache store may have failed; result still valid) */ + dns_copy_addrs(out, resolved, count, max); return count; } diff --git a/src/htslib.h b/src/htslib.h index b059dd75..6bf928c6 100644 --- a/src/htslib.h +++ b/src/htslib.h @@ -222,7 +222,8 @@ LLint http_xfread1(htsblk * r, int bufl); /* Cached resolver: fill out[0..count-1] with up to max addresses for iadr (in resolver order), returning the count (0 = does not resolve, negative-cached). Resolves once per host; later calls read the DNS cache. Must hold no lock - (brackets opt->state.lock itself). */ + (brackets opt->state.lock itself, never across the resolve). A miss resolves + on a worker thread bounded by opt->timeout; a timeout reports 0, uncached. */ int hts_dns_resolve_all(httrackp *opt, const char *iadr, SOCaddr *out, int max, const char **error); HTS_INLINE SOCaddr *hts_dns_resolve2(httrackp *opt, const char *iadr, diff --git a/src/htsselftest.c b/src/htsselftest.c index d8de32f8..5abba57c 100644 --- a/src/htsselftest.c +++ b/src/htsselftest.c @@ -2109,6 +2109,15 @@ static int st_dns(httrackp *opt, int argc, char **argv) { return err; } +static int st_dnstimeout(httrackp *opt, int argc, char **argv) { + const int err = dns_timeout_selftests(opt); + + (void) argc; + (void) argv; + printf("dns-timeout-selftest: %s\n", err ? "FAIL" : "OK"); + return err; +} + static int st_cookies(httrackp *opt, int argc, char **argv) { static t_cookie cookie; char hdr[1024]; @@ -3133,6 +3142,8 @@ static const struct selftest_entry { {"cache-corrupt", "", "cache read-side corruption self-test", st_cache_corrupt}, {"dns", "", "DNS resolver/cache self-test", st_dns}, + {"dnstimeout", "", "a slow DNS resolve is bounded and holds no lock", + st_dnstimeout}, {"cookies", "", "cookie request-header self-test", st_cookies}, {"useragent", "", "default User-Agent self-test", st_useragent}, {"makeindex", "[dir]", "hts_finish_makeindex footer/refresh self-test", diff --git a/tests/01_engine-dnstimeout.test b/tests/01_engine-dnstimeout.test new file mode 100644 index 00000000..165761d9 --- /dev/null +++ b/tests/01_engine-dnstimeout.test @@ -0,0 +1,15 @@ +#!/bin/bash +# + +set -euo pipefail + +# A resolve against a deliberately slow mock getaddrinfo (no network) must +# return on --timeout, must not hold opt->state.lock meanwhile, and must not +# cache the timeout as an answer (#606). +# 'run' is an ignored placeholder argument. +out=$(httrack -#test=dnstimeout run) + +test "$out" = "dns-timeout-selftest: OK" || { + echo "expected 'dns-timeout-selftest: OK', got: $out" >&2 + exit 1 +} diff --git a/tests/Makefile.am b/tests/Makefile.am index 39c04bfe..217a7caf 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -35,6 +35,7 @@ TESTS = \ 01_engine-copyopt.test \ 01_engine-crange.test \ 01_engine-dns.test \ + 01_engine-dnstimeout.test \ 01_engine-doitlog.test \ 01_engine-entities.test \ 01_engine-filelist.test \