The re-fetch backup can destroy the copy it was taken to protect - #818
Merged
Conversation
The re-fetch backup, the .z content-coding spool and its .u decode target were all named by appending an extension to the local save name, which is a name the mirror can hold. A site serving <path>.bak had its copy renamed over as the backup and then unlinked on commit, and the run reported success. Build them in an hts-tmp directory beside the file instead, and remove it once the last slot sharing it is done. Closes #774 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Both zip-repair paths removed the damaged cache, moved repair.zip onto its name without looking at the result, and announced a successful recovery either way. A refused move left the entries under a name nothing reads and no cache at all. Go through hts_rename_over(), which keeps the destination when it cannot replace it, and report the failure instead of claiming success. Closes #786 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
…p away The re-fetch backup was dropped on commit without checking that the new copy exists. filecreate() can fail after the backup was taken (EMFILE, ENOSPC, EROFS), and back_finalize()'s incomplete-transfer guard is gated on a known Content-Length, so a chunked response went straight to the commit and lost both copies. Restore the backup instead when there is nothing to commit to. Closes #775 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The Windows job iterates a fixed glob, so a 106_engine-* test never ran there; name it in the list. The move it exercises is the one whose failure mode is Windows-specific. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The Windows job iterates a fixed glob, so a 108_engine-* test never ran there. The rename semantics this drives are exactly what differs on the CRT, which reserves EACCES for a locked source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
…rantee 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) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
…hing On Windows the rename destination always exists, so every cache repair takes hts_rename_over()'s fallback; without #790 that fallback still removes the cache before a retry that can fail. Stacking the two keeps the repair path away from the destructive shape. Carries the #824 fix in the same block: unzRepair() reports Z_OK with no entries for an input that never had a local file header, and its output can be short on a full disk, so both call sites committed an empty or truncated archive over the cache. Both now go through one cache_repair() that refuses a recovery holding nothing or failing to open. Closes #824 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Moving the temporaries into an hts-tmp directory only pushed the collision one level down: hts-tmp is a path segment a URL can name, so a site serving /d/hts-tmp/a.bin.bak still landed on the backup path for /d/a.bin and was consumed. url_savename now escapes hts-tmp and hts-cache as whole path components, the way it already escapes the DOS device names, which also stops a URL from naming hts-cache/new.zip. The three tests now drive the hostile shape rather than the pre-fix name: a savename assertion on the escape, a crawl serving /bakname/hts-tmp/a.bin.bak, and a selftest pinning the temporary's name. Also: retry the backup once when another slot removed the shared directory between structcheck and the rename; report a commit that had to restore, so the caller does not cache the new validators against the old body; drop the coded body on the too-long branch as its sibling does; and scope the leftover scan to plants inside hts-tmp only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
This was referenced Jul 27, 2026
#822 converted 01_engine-renameover.test's assertions to the here-string form, since grep -q SIGPIPEs the echo feeding it and an assertion that held reports 141. This branch had added its restore-outcome and retry legs in the old pipe form. Resolved on master's form, with those legs ported into it rather than kept alongside. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Carries the here-string reconciliation with #822 and the aside directory-refusal fix. TESTS tail keeps both new entries in order. Signed-off-by: Xavier Roche <roche@httrack.com>
Carries the here-string reconciliation with #822 and the aside directory-refusal fix. TESTS tail keeps all four new entries in order. Signed-off-by: Xavier Roche <roche@httrack.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
xroche
enabled auto-merge (squash)
July 27, 2026 18:07
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Resolves the tests/tests-list.mk collision by keeping every entry from both sides; the list is one TESTS += line per file, so a union is the correct resolution here (unlike Makefile.am's continued list, see #844). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Keeps every tests-list.mk entry from both sides; the list is one TESTS += line per test, so a union is the correct resolution (unlike Makefile.am's continued list, see #844). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Keeps every tests-list.mk entry from both sides; the list is one TESTS += line per test, so a union is the correct resolution (unlike Makefile.am's continued list, see #844). 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
* A URL can still be saved onto the engine's own temporary files The re-fetch backup and the content-coding temporaries built their names inside the mirror, so a site serving the matching path had its mirrored copy used as scratch and then deleted. #818 escaped hts-cache and hts-tmp as reserved path components, but three shapes still got through (#842): a trailing space that cleanEndingSpaceOrDot() stripped back off after the check, and the first path component, which the table could not see because it anchors on a leading '/' that url_savename has already removed. The latter covers a single-label hostname too, and left the DOS device names equally exposed in that position. The temporaries now live in ~hts-tmp/. url_savename maps '~' to '_' in every name it builds, so nothing a site can serve resolves inside it whatever the escape does; the escape is still fixed, as defence for hts-cache/ itself. The frozen-slot spool (<save>.tmp) shared the collision, plus a heap buffer sized from url_sav that the path_html branch overran, and moves to the same helper. The .delayed placeholder does not: it goes through url_savename's collision detection, so a competing link gets a suffix rather than the file. Closes #774 Closes #842 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Scope the reserved-name escape and fix the review blockers Restrict the new first-component match so a hostname label is not rewritten: only a trailing run cleanEndingSpaceOrDot() strips may end the name there, so aux.example.com keeps its name while a single-label hts-tmp host and a path-first hts-cache/ are still escaped. Renaming a host directory would have moved an existing mirror out from under --update --purge-old. Drop the frozen-slot spool rewrite: it carried a pre-existing heap overflow that belongs in its own change. Clear back->tmpfile when structcheck() fails in the named branch, as the unnamed one does. Test 132 now plants a leftover at the exact temporary path, so both a reverted HTS_TMPDIR and a no-op back_refetch_backup fail it; 131 pins the hostname labels. Signed-off-by: Xavier Roche <roche@httrack.com> 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs in the re-fetch backup path. The second is fixed; the first is only mitigated, and the issue stays open.
#775, fixed. The backup was dropped on commit without checking that the new copy exists.
filecreate()can fail after the backup was taken (EMFILE under a high-c, ENOSPC, EROFS), andback_finalize()'s incomplete-transfer guard only runs on a known Content-Length, so a chunked response went straight to the commit and lost both copies. The commit is now conditional on the new copy existing. When it restores instead it says so, because the caller must not cache this response's validators against the body it just put back. Existence is not completeness, though: a chunked response cut at a chunk boundary still looks complete to the engine, which is #840.#774, mitigated only. The re-fetch backup, the
.zcontent-coding spool and its.udecode target were all named by appending an extension to the local save name, which is a name the mirror itself can hold, so a site serving<path>.bakhad that copy renamed over to become the backup and then unlinked when the transfer committed. They now live in anhts-tmpdirectory beside the file, andurl_savenameescapeshts-tmpandhts-cacheas path components the way it has always escaped the DOS device names,/nulbecomingnul_.That covers the ordinary case and nothing more. Three shapes still land on the temporary's name: a trailing space on the component, a component in first position under a structure such as
-N100, and a single-label hostname. Reproductions are in #842, which also records that the first-position and hostname holes are inherited from the DOS-device escaping and so predate this PR. Do not read the escaping here as closing #774.The user-visible cost of the escaping is deliberate: a site that genuinely serves
/hts-tmp/...or/hts-cache/...mirrors ashts-tmp_/hts-cache_, with links rewritten to match.Tests, each checked against the unfixed build: a savename assertion that the reserved segments escape, a crawl serving
/bakname/hts-tmp/a.bin.bak, and an engine selftest pinning the temporary's name so it cannot move out of the reserved directory without the reservation moving with it. They cover the shapes that are fixed, not the three in #842.Refs #774
Closes #775