From d5eb3f7e9fc762090aefa4c2c5806935210a52db Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Mon, 3 Aug 2026 11:42:16 +0200 Subject: [PATCH] fix DownloadPipeline.fetch_many() crashing on a missing chunk With replacement_chunk=False, fetch_many() is documented (and used) to yield None for a chunk that is missing in the repository - but the size check right before the yield then did len(None) and raised TypeError instead. borg webdav is currently the only caller that passes replacement_chunk=False for file content, so its "chunk missing" path (abort the connection instead of serving corrupted data) never actually ran: the TypeError ended up in the request error handler, which then tried to send a 500 for a response whose headers were already on the wire. Adds unit tests for both flavours of a missing chunk (the one without a replacement chunk fails without this fix) and an end-to-end webdav test for downloading a file with a chunk missing. Co-Authored-By: Claude Opus 5 --- src/borg/archive.py | 2 +- src/borg/testsuite/archive_test.py | 15 ++++++++ .../testsuite/archiver/webdav_cmd_test.py | 36 +++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 45377c2ec1..3299d07399 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -399,7 +399,7 @@ def fetch_many(self, chunks, ro_type=None, replacement_chunk=True): except KeyError: _, data = self.repo_objs.parse(id, cdata, ro_type=ro_type) self.parsed_cache[(id, ro_type)] = data - assert size is None or len(data) == size + assert data is None or size is None or len(data) == size yield data diff --git a/src/borg/testsuite/archive_test.py b/src/borg/testsuite/archive_test.py index a9ff6543ca..81214ba468 100644 --- a/src/borg/testsuite/archive_test.py +++ b/src/borg/testsuite/archive_test.py @@ -261,6 +261,21 @@ def counting_parse(id, cdata, **kw): assert len(set(parsed_ids)) == 3 +@pytest.mark.parametrize("replacement_chunk", [False, True]) +def test_download_pipeline_missing_chunk(replacement_chunk): + # a chunk missing in the repository is either replaced by all-zero data of the + # correct size, or reported as None - and never blows up on the size check. + key = PlaintextKey(None) + repo_objs = RepoObj(key) + data = b"foobar" * 100 + id = repo_objs.id_hash(data) + repository = MockFetchRepo({id: None}) # the object is gone + pipeline = DownloadPipeline(repository, repo_objs) + chunk_list = [ChunkListEntry(id, len(data))] + result = list(pipeline.fetch_many(chunk_list, ro_type=ROBJ_FILE_STREAM, replacement_chunk=replacement_chunk)) + assert result == [zeros[: len(data)] if replacement_chunk else None] + + def test_download_pipeline_zero_chunks_served_locally(): # repeated all-zero chunks (e.g. from the holes of a sparse file) shall be served # directly from the zeros constant, without repository access, see issue #1678. diff --git a/src/borg/testsuite/archiver/webdav_cmd_test.py b/src/borg/testsuite/archiver/webdav_cmd_test.py index 54800398db..c9fab37834 100644 --- a/src/borg/testsuite/archiver/webdav_cmd_test.py +++ b/src/borg/testsuite/archiver/webdav_cmd_test.py @@ -22,6 +22,7 @@ import pytest from ...constants import * # NOQA +from ...archive import Archive from ...manifest import Manifest from ...platform import is_win32 from ...repository import Repository @@ -585,6 +586,41 @@ def test_webdav_file_without_chunks(archivers, request): thread.join(timeout=10) +def test_webdav_damaged_file(archivers, request): + # A file with a chunk missing in the repository must never be served as if it were + # intact: the server aborts the connection, so the client sees a short read. + archiver = request.getfixturevalue(archivers) + _create_archive(archiver) + args = SimpleNamespace( + sort_by="ts", match_archives=None, first=None, last=None, older=None, newer=None, oldest=None, newest=None + ) + repository = Repository(archiver.repository_path, exclusive=True) + with repository: + manifest = Manifest.load(repository, Manifest.NO_OPERATION_CHECK) + archive = Archive(manifest, manifest.archives.get("test").id) + for item in archive.iter_items(): + if item.path.endswith("big"): + repository.delete(item.chunks[-1].id) # get rid of a chunk of "big" + break + else: + assert False # missed the file + server = make_server(manifest, args, port=0) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + conn = http.client.HTTPConnection("127.0.0.1", server.server_address[1]) + conn.request("GET", "/test/input/big") + response = conn.getresponse() + assert response.status == 200 + with pytest.raises(http.client.IncompleteRead): + response.read() # the connection is aborted where the chunk is missing + conn.close() + finally: + server.shutdown() + server.server_close() + thread.join(timeout=10) + + @pytest.mark.skipif(is_win32 or not hasattr(os, "mkfifo"), reason="fifo (a special file) needs POSIX") def test_webdav_special_files(archivers, request): # A named pipe stands in for special files (devices, fifos, sockets): it is shown in