Skip to content

An --update pass overwrites the previous WARC and leaves a page-less WACZ - #777

Merged
xroche merged 4 commits into
masterfrom
fix/warc-update-overwrite
Jul 27, 2026
Merged

An --update pass overwrites the previous WARC and leaves a page-less WACZ#777
xroche merged 4 commits into
masterfrom
fix/warc-update-overwrite

Conversation

@xroche

@xroche xroche commented Jul 27, 2026

Copy link
Copy Markdown
Owner

A second crawl into the same output reopened the WARC with "wb" and truncated it. On a cache-served pass nearly every URL comes back 304, so the file that replaced the archive held revisit records whose payloads had just been deleted, and the WACZ rebuilt beside it came out with zero page rows and nothing left to replay.

The writer now builds into a sibling .tmp whenever an archive is already there, and swaps it in at close only if the result can stand on its own. A pass that only revisited URLs it did not re-download keeps the previous .warc.gz, .cdx and .wacz, and logs why. A first crawl or a cacheless pass still writes in place.

Two related bugs turned up while building it. hts_rename_over() unlinks its destination when the source is missing, so a failed segment open under --warc-max-size deleted the previous run's segment; every temp is now checked present before the first rename, and a run that lost a record or a segment does not swap at all. warc_rotate() also advanced w->seg before the open that could fail, leaving the counter naming a file nobody created.

Tests 73 and 74 were pinning the old behaviour, 73 by requiring the update pass to have overwritten the archive and 74 by driving its second pass with -C0 to sidestep this bug. 73 now archives its second pass under a name of its own, which is the recovery the error message suggests; 74 keeps -C0 as the case that legitimately replaces. The two new tests fail against the pre-fix engine.

What --update should mean for WARC output is still open, and refusing has a cost: a pass that fetches new pages alongside its 304s loses those captures too, because there is nowhere valid to put them until that is settled. Options and evidence are in #759.

Closes #759

xroche and others added 4 commits July 27, 2026 09:42
A second crawl into the same output reopened the WARC with "wb" and
truncated it. On a cache-served pass nearly every URL comes back 304, so
the new file held revisit records whose payloads had just been deleted,
and the regenerated WACZ came out with zero page rows: a package that
replays nothing, in place of one that replayed fine.

The writer now builds into a sibling .tmp whenever an archive is already
there, and only swaps it in at close if the result can stand on its own.
A pass that only revisited URLs it did not re-download keeps the previous
.warc.gz, .cdx and .wacz untouched and says so.

What --update should ultimately mean for WARC output is still open; this
only stops the silent destruction in the meantime.

Closes #759

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
A run that lost a record or a segment must not replace a whole archive,
and hts_rename_over unlinks its destination when the source is missing,
so every segment has to be on disk before the first rename.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
# Conflicts:
#	tests/Makefile.am
--warc-max-size 2000 never rotated under the harness's --robots=0, so the
segment test was checking a single-file archive; a mutant that renamed
only segment 0 passed it. 600 rotates, and --archive-min-files fails the
test if a shrinking crawl ever stops producing the segments it checks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
Comment thread src/htswarc.c Dismissed
Comment thread src/htswarc.c Dismissed
@xroche

xroche commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

CodeQL's two cpp/path-injection alerts are the two master already carries, at the same call sites. They re-reported because the diff shifted the lines:

alert in this PR on master
251 htswarc.c:1292, the warc_rotate() FOPEN htswarc.c:1268
250 htswarc.c:1514, the warc_open() FOPEN htswarc.c:1477

Same two FOPEN calls, with one extra hop through warc_open_path(). No new call site, no new source.

The taint, traced rather than taken from the alert text. CodeQL names its sources as a command-line argument and an environment variable, and that holds up:

  • warc_open()'s path comes from StringBuff(opt->warc_file). The only writers of that field are htscoremain.c:1773 (argv[na]), the WARC_AUTONAME constant at 1801 and 1803, copy_htsopt() at htscore.c:3744, and the "" init at htslib.c:6031. None of them carries network data.
  • The auto-name and bare-basename cases prepend StringBuff(opt->path_html), which is -O. Its only non-test writers are the "" init and copies of another httrackp.
  • warc_rotate()'s name is w->seg_base (split from the same path), %05u of w->seg, and w->seg_ext, which is a static ".warc.gz" or ".warc".

One place remote content does reach this, worth stating because it is where the segment-per-run work would show a problem if there were one: w->seg advances when w->offset >= w->max_size, and w->offset is ftell(), so a hostile server can influence how many segments a run creates by sending large bodies. That influences the count, not the characters. %05u of an unsigned yields digits, so there is no way to reach a separator or a traversal component through it, and that holds for crawl-00001.warc.gz too.

Dismissing both as the same accepted class as the other twelve open cpp/path-injection alerts, with this trace as the recorded reason.

@xroche
xroche merged commit 5638910 into master Jul 27, 2026
22 checks passed
xroche added a commit that referenced this pull request Jul 27, 2026
…#788)

Every WARC header line is built with `wbuf_printf()`, which formatted into a 1024-byte stack buffer and returned `-1` instead of growing. `"WARC-Target-URI: %s\r\n"` costs 19 fixed bytes, so the line failed once a URL reached 1005 bytes, the `-1` reached the `goto done` in `warc_emit()`, and the record was abandoned. The page still got mirrored, the crawl still exited 0, and nothing was logged, so the only symptom was a URL missing from the archive.

`wbuf` reallocs already, so oversized output now formats straight into it after a `wbuf_reserve()`, which is the growth half of `wbuf_add()` split out. Every field benefits, not only the two carrying URLs. The second pass is bounded against what was reserved rather than trusted: advancing `len` by a return value larger than the reservation would push `len` past `cap` and corrupt the bounds check of every later append.

`-#test=warc-longurl` sweeps 100 to 9000 bytes across the boundary. Against the old formatter it fails at exactly 1005 and up, with 1003 and 1004 passing. Each record carries a distinct payload because identical ones dedupe into revisits, which would otherwise hide the response records the test counts.

One caveat on the sanitizer evidence, since it is easy to over-read. The buffer grows by doubling, so a small off-by-one lands in allocation slack where ASan cannot see it; that is why the second-pass bound is a logic check rather than something left to the sanitizer. The ASan+UBSan run over the sweep is clean, but only after planting a deliberate overflow in `wbuf_reserve` to confirm the probe actually fires. It did not, at first: libtool silently drops `-fsanitize` from the shared-library link, which produced no binary at all and a "clean" result that meant nothing.

Worth knowing for the segment work: `warc_emit()` returning `-1` also sets `w->failed`, which suppresses the archive swap added in #777. Before this fix a single over-long URL would make an `--update` pass throw away its whole archive and keep the previous one.

Closes #785

Signed-off-by: Xavier Roche <roche@httrack.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xroche
xroche deleted the fix/warc-update-overwrite branch July 27, 2026 18:26
xroche added a commit that referenced this pull request Jul 27, 2026
…s unbacked

Writing no WARC record at all for an engine-forced not-modified (the #839 fix
in the previous commit) dropped w->unbacked_revisits accounting along with it,
so an all-forced pass wrongly looked fully backed: warc_commit's swap guard let
it replace the previous archive with an (almost) empty one, reopening #777's
exact shape, and warc_cdx_flush then left a stale .cdx pointing into the
truncated file. Fix: emit a self-referencing identical-payload-digest revisit
instead of nothing, still counted as unbacked, so the guard keeps protecting
the previous archive exactly as it does for a real 304.

Classifying a notmodified htsblk by warc_resphdr's presence was too broad: it
also matched back_add()'s cache-priority branch, which never sent a request at
all and is unrelated to the back_wait() hacks #839 targets. Replaced it with an
explicit hts_boolean, warc_forced_notmodified, set only inside back_wait()'s
304-swap block from the already-computed server_sent_304, leaving every other
notmodified path (including back_add()'s) at its default and thus at its prior
behavior.

tests/134 now asserts the exact peer IP instead of mere presence, since a
zeroed-but-AF_INET address also satisfies a non-empty check. tests/135 gained
a second scenario: an all-forced-not-modified rerun against the same archive
name, which is the shape that lets the swap guard's bug actually destroy the
previous archive; the mixed-fixture scenario alone couldn't see it, since a
real 304 alongside it already keeps the guard's counter above zero.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
xroche added a commit that referenced this pull request Jul 27, 2026
…archived as one the server never sent (#849)

* A 304 during --update leaks the whole previous htsblk

back_wait() handles a 304 by replacing the response struct with the cache
entry, carrying only the socket and keep-alive members across via
back_connxfr(). The struct assignment drops every owned pointer the live
response still held without freeing any of them: the 8 KB header buffer on
every update, plus the two WARC header stashes when --warc-file is on. An
update over a 10k-page site drops roughly 80 MB in one run.

back_clear_entry() already knew how to tear those down, so the frees move into
a helper that both it and the 304 path call.

The new test runs the two-pass mini304 crawl with LeakSanitizer on, which the
sanitized CI job otherwise disables. The fresh first pass is the control: it
has no cache entry to read back and is clean either way. The update pass
reports 16 KB in 2 objects on master, one per unchanged URL, and nothing with
the fix.

Closes #782

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

* Trim the test header

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

* Review fixes: reuse deleteaddr(), and cover the WARC limb

back_free_response() was reimplementing deleteaddr(), which already frees adr
and headers and NULLs both; call it instead so the two cannot drift.

Test 114 never passed --warc-file, so the warc_free_request() limb ran with
both pointers NULL on every path it exercised and deleting it kept the test
green. A third pass turns the archive on, and it now fails with the 835 and 238
byte stashes when that call goes away.

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

* Register the new leak test as an expected Windows skip

The Win32/x64 job pins the exact set of tests allowed to skip, so an
all-skipped suite cannot report green. 114_local-update-304-leak needs a
LeakSanitizer build and MSVC has no equivalent, so it skips there and tripped
the gate with fail=0.

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

* A 304 revisit archives none of the exchange it stands for

The cache entry overwrites the whole htsblk in back_wait(), taking the
stashed request and response headers with it, so the revisit record gets
a synthesized "HTTP/1.1 200 OK" block and no request record. Both blocks
now move across the swap, but only for a real 304: the engine also forces
HTTP_NOT_MODIFIED by itself, and those have no 304 to carry. Since a 304
declares no Content-Type, warc_write_transaction() takes the resource type
from the caller so revisit CDXJ lines keep their mime.

cache_read_including_broken() returns the entry's response struct after
back_clear_entry() has freed it; adr, headers and location come back NULL
now, as the normal cache_readex() path already returns them.

Closes #826

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

* Do not expand an empty bodyargs array under macOS bash 3.2

Test 122 asks local-crawl.sh only for the revisit checks, so it is the
first caller to leave bodyargs empty. Bash before 4.4 calls that unbound
under set -u, which passes on Linux and fails the macOS leg.

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

* Restore the opt argument to hts_rename_over call sites

Lost when the master merge took this file wholesale; #816 gave the function an
httrackp parameter and the three call sites here reverted to the old form.

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

* A 304 revisit names no peer IP, and an engine-forced not-modified is archived as one the server never sent

back_connxfr() moves the socket and keep-alive members across the cache-entry
swap in the 304 path of back_wait(), but not the peer address, so every
revisit record came out with no WARC-IP-Address (#838). Carry it across the
same swap.

Separately, several engine hacks (the same-size heuristic, the #176
error-ignore-on-update path, ...) force HTTP_NOT_MODIFIED themselves when the
server actually said something else. That statuscode flows into the same 304
branch, and warc_write_backtransaction() turned it into a revisit record whose
WARC-Profile claims a server-not-modified 304 that was never exchanged (#839).
The fix reuses the existing server_sent_304 signal (already used to gate the
request/response header carry-over): its persisted proxy is
warc_resphdr != NULL, present only when the swap actually moved real 304
headers across. When it's absent, no server exchange happened this run to
archive, and there's no freshly re-fetched body either to dedup against a
same-run predecessor, so the fix skips the record entirely rather than
writing an identical-payload-digest revisit with nothing behind it to refer
to.

tests/134 and tests/135 extend the mini304/errmask fixtures into a two-pass
--warc-file crawl and assert against tests/warc-validate.py, which gained
--expect-ip and --no-record-for for these checks. Both reproduce their bug
when run against the pre-fix code.

Closes #838
Closes #839

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

* Fix review findings: an engine-forced not-modified must still count as unbacked

Writing no WARC record at all for an engine-forced not-modified (the #839 fix
in the previous commit) dropped w->unbacked_revisits accounting along with it,
so an all-forced pass wrongly looked fully backed: warc_commit's swap guard let
it replace the previous archive with an (almost) empty one, reopening #777's
exact shape, and warc_cdx_flush then left a stale .cdx pointing into the
truncated file. Fix: emit a self-referencing identical-payload-digest revisit
instead of nothing, still counted as unbacked, so the guard keeps protecting
the previous archive exactly as it does for a real 304.

Classifying a notmodified htsblk by warc_resphdr's presence was too broad: it
also matched back_add()'s cache-priority branch, which never sent a request at
all and is unrelated to the back_wait() hacks #839 targets. Replaced it with an
explicit hts_boolean, warc_forced_notmodified, set only inside back_wait()'s
304-swap block from the already-computed server_sent_304, leaving every other
notmodified path (including back_add()'s) at its default and thus at its prior
behavior.

tests/134 now asserts the exact peer IP instead of mere presence, since a
zeroed-but-AF_INET address also satisfies a non-empty check. tests/135 gained
a second scenario: an all-forced-not-modified rerun against the same archive
name, which is the shape that lets the swap guard's bug actually destroy the
previous archive; the mixed-fixture scenario alone couldn't see it, since a
real 304 alongside it already keeps the guard's counter above zero.

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

* Cover the cache-priority path and gate the digest revisit on OpenSSL

warc_forced_notmodified had exactly one assignment site, inside back_wait()'s
304-swap block. back_add()'s cache-priority branch (-C1, a lock-file resume,
or opt->state.stop) sets notmodified too but never sent a request, so it fell
through to the zero default and got classified as a genuine 304, emitting a
server-not-modified revisit around a synthesized "HTTP/1.1 200 OK" block. Set
the flag there as well: no request there ever means no server 304 to claim.

The identical-payload-digest revisit for an engine-forced not-modified also
assumed a digest was available, but payload_digest_b32() returns nothing
without OpenSSL. Gate the profile on have_pdig; write no record when it's
absent, since there's neither a digest to point a revisit at nor a real
exchange to record as a response, keeping w->unbacked_revisits incremented
either way so the archive-protection guard still holds.

tests/136 drives the cache-priority path with -C1 and checks the same
WARC-Profile assertion as tests/135. Both new branches (135 and 136) split on
HTTPS_SUPPORT so the no-OpenSSL CI leg exercises the no-digest path instead of
skipping it; 135's archive-kept scenario drops --wacz for the same reason, a
package needs OpenSSL for its SHA-256 digests and would otherwise skip the one
thing it exists to catch.

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

* An empty body looks digest-less too, and the profile tests never checked the real 304

has_payload requires body_len > 0, so a genuinely empty body (a real,
zero-byte file) fell into the same "no digest" branch as a build without
OpenSSL and got silently dropped from the archive instead of a proper
identical-payload-digest revisit. A zero-length body has a well-defined
digest (sha1 of nothing); compute it directly for that case instead of
reusing has_payload's gate.

tests/135 asserted mini304's pages were revisits but never which profile, so
forcing warc_forced_notmodified true unconditionally (a regression that would
mislabel or suppress a real 304) still passed. Added the server-not-modified
assertion for both mini304 pages, and a matching zero-length fixture
(errmask/empty.dat) plus its identical-payload-digest assertion. tests/136
gained a body-hex check on the fresh archive: its update-pass checks only
assert absence, so a warc_write_transaction stub that writes nothing at all
went undetected on the no-openssl leg.

The unbacked-revisits counter still increments even when nothing gets
written (load-bearing: it's what keeps the swap guard from letting an
all-unchanged pass overwrite a good archive with an empty one), so the
"this pass revisited N URL(s)" log line no longer claims those URLs got a
named body; it now says the archive doesn't hold their current content,
true whether a record was written or not.

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

---------

Signed-off-by: Xavier Roche <roche@httrack.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An --update pass overwrites the previous WARC and leaves a page-less WACZ

2 participants