diff --git a/strix/interface/cloud/source_upload.py b/strix/interface/cloud/source_upload.py index 780a845695..4fc3babc11 100644 --- a/strix/interface/cloud/source_upload.py +++ b/strix/interface/cloud/source_upload.py @@ -127,6 +127,21 @@ def close(self) -> None: ... ) +def _stat_identity(info: os.stat_result) -> tuple[int, ...]: + """Fields that must not change between selecting a file and archiving it. + + Both sides of the comparison must come from `fstat` on an open handle. + What `st_ctime_ns` means depends on how a file is stat'ed: on POSIX a path + stat and `fstat` both report the inode-change time, but on Windows a path + stat reports the creation time while `fstat` reports the last metadata + change. Pinning files with a path stat therefore rejected every file + modified after it was created (#1258). With handle stats on both sides the + two agree, and `st_ctime_ns` stays a tamper signal on every platform: a + chmod or a rename that leaves size and mtime alone still moves it. + """ + return (info.st_dev, info.st_ino, info.st_size, info.st_mtime_ns, info.st_ctime_ns) + + @dataclass(frozen=True) class SelectedFile: path: Path @@ -137,6 +152,10 @@ class SelectedFile: mtime_ns: int ctime_ns: int + @property + def identity(self) -> tuple[int, ...]: + return (self.device, self.inode, self.size, self.mtime_ns, self.ctime_ns) + @dataclass(frozen=True) class SourceManifest: @@ -302,10 +321,21 @@ def select_source( if not stat.S_ISREG(info.st_mode): excluded["symlink_or_non_file"] += 1 continue - if not include_archives and _has_archive_magic(path): + # Pin the file with the same kind of stat _write_archive checks it with. + handle_info, header = _inspect_regular_file( + path, archive_name, read_header=not include_archives + ) + if not stat.S_ISREG(handle_info.st_mode) or ( + handle_info.st_dev, + handle_info.st_ino, + ) != (info.st_dev, info.st_ino): + raise http.CloudError( + f"{archive_name} changed while the source was being selected; retry." + ) + if not include_archives and _is_archive_header(header): excluded["nested_archive"] += 1 continue - if info.st_size > MAX_FILE_BYTES: + if handle_info.st_size > MAX_FILE_BYTES: raise http.CloudError( f"{archive_name} is larger than the 25 MB per-file limit; exclude it explicitly." ) @@ -313,14 +343,14 @@ def select_source( SelectedFile( path=path, archive_name=archive_name, - size=info.st_size, - device=info.st_dev, - inode=info.st_ino, - mtime_ns=info.st_mtime_ns, - ctime_ns=info.st_ctime_ns, + size=handle_info.st_size, + device=handle_info.st_dev, + inode=handle_info.st_ino, + mtime_ns=handle_info.st_mtime_ns, + ctime_ns=handle_info.st_ctime_ns, ) ) - total_bytes += info.st_size + total_bytes += handle_info.st_size if len(selected) > MAX_FILES: raise http.CloudError( f"source contains more than {MAX_FILES:,} files; narrow --source or add exclusions." @@ -597,14 +627,7 @@ def _write_archive(destination: Path, files: tuple[SelectedFile, ...]) -> None: raise http.CloudError(f"could not safely read {item.archive_name}: {exc}") from exc with os.fdopen(descriptor, "rb") as source_file: current = os.fstat(source_file.fileno()) - if ( - not stat.S_ISREG(current.st_mode) - or current.st_size != item.size - or current.st_dev != item.device - or current.st_ino != item.inode - or current.st_mtime_ns != item.mtime_ns - or current.st_ctime_ns != item.ctime_ns - ): + if not stat.S_ISREG(current.st_mode) or _stat_identity(current) != item.identity: raise http.CloudError( f"{item.archive_name} changed while the source archive was being built; " "retry." @@ -627,11 +650,7 @@ def _write_archive(destination: Path, files: tuple[SelectedFile, ...]) -> None: if ( source_file.read(1) or not stat.S_ISREG(final.st_mode) - or final.st_size != item.size - or final.st_dev != item.device - or final.st_ino != item.inode - or final.st_mtime_ns != item.mtime_ns - or final.st_ctime_ns != item.ctime_ns + or _stat_identity(final) != item.identity ): raise http.CloudError( f"{item.archive_name} changed while the source archive was being " @@ -653,12 +672,34 @@ def _has_archive_magic(path: Path) -> bool: try: descriptor = os.open(path, flags) with os.fdopen(descriptor, "rb") as stream: - header = stream.read(512) + header = stream.read(_ARCHIVE_HEADER_BYTES) except OSError: return False + return _is_archive_header(header) + + +_ARCHIVE_HEADER_BYTES = 512 + + +def _is_archive_header(header: bytes) -> bool: return header.startswith(_ARCHIVE_MAGIC_PREFIXES) or header[257:262] == b"ustar" +def _inspect_regular_file( + path: Path, archive_name: str, *, read_header: bool +) -> tuple[os.stat_result, bytes]: + """Stat a file through its own handle, reading its header too if asked.""" + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) + try: + descriptor = os.open(path, flags) + with os.fdopen(descriptor, "rb") as stream: + info = os.fstat(stream.fileno()) + header = stream.read(_ARCHIVE_HEADER_BYTES) if read_header else b"" + except OSError as exc: + raise http.CloudError(f"could not safely read {archive_name}: {exc}") from exc + return info, header + + def _load_ignore_patterns(source: Path) -> list[str]: path = source / ".strixignore" raw_text = _read_ignore_file(path) diff --git a/tests/test_cloud_source_upload.py b/tests/test_cloud_source_upload.py index dcd40baa8f..7352972b32 100644 --- a/tests/test_cloud_source_upload.py +++ b/tests/test_cloud_source_upload.py @@ -6,6 +6,7 @@ import os import shutil import subprocess +import time import zipfile from typing import TYPE_CHECKING, Any @@ -696,3 +697,95 @@ def test_oversize_archive_names_largest_files_and_exclude_guidance( assert "--exclude" in raised.value.next_step assert "--dry-run --show-files" in raised.value.next_step assert not list(tmp_path.glob("strix-source-*.zip")) + + +class _StatWith: + """A stat result with some of its fields replaced.""" + + def __init__(self, real: Any, **fields: int) -> None: + self._real = real + self.__dict__.update(fields) + + def __getattr__(self, name: str) -> Any: + return getattr(self._real, name) + + +def test_archive_accepts_a_path_stat_with_a_different_ctime( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Selection must not pin files with what a path stat calls `st_ctime_ns`. + + On Windows a path stat reports the creation time there and `fstat` the last + metadata change, so for a file modified after it was created the two + differ for good (#1258). This reproduces that on any platform. + """ + (tmp_path / "app.py").write_bytes(b"safe") + real_lstat = source_upload.Path.lstat + + def lstat_reporting_creation_time(self: Path) -> Any: + info = real_lstat(self) + return _StatWith(info, st_ctime_ns=info.st_ctime_ns - 7 * 60 * 1_000_000_000) + + monkeypatch.setattr(source_upload.Path, "lstat", lstat_reporting_creation_time) + manifest = source_upload.select_source(tmp_path) + + source_upload._write_archive(tmp_path / "source.zip", manifest.files) + + with zipfile.ZipFile(tmp_path / "source.zip") as archive: + assert archive.read("app.py") == b"safe" + + +@pytest.mark.skipif(os.name != "nt", reason="only Windows reports different ctimes") +def test_archive_accepts_a_windows_file_modified_after_creation(tmp_path: Path) -> None: + source_path = tmp_path / "app.py" + source_path.write_bytes(b"safe") + time.sleep(0.1) + with source_path.open("ab") as stream: + stream.write(b" and sound") + with source_path.open("rb") as stream: + # the situation from #1258: the path and the handle disagree on ctime + assert os.fstat(stream.fileno()).st_ctime_ns != source_path.lstat().st_ctime_ns + manifest = source_upload.select_source(tmp_path) + + source_upload._write_archive(tmp_path / "source.zip", manifest.files) + + with zipfile.ZipFile(tmp_path / "source.zip") as archive: + assert archive.read("app.py") == b"safe and sound" + + +def test_archive_rejects_a_metadata_only_change_after_selection(tmp_path: Path) -> None: + """A chmod leaves size and mtime alone but moves the handle ctime, on Windows too.""" + source_path = tmp_path / "app.py" + source_path.write_bytes(b"safe") + manifest = source_upload.select_source(tmp_path) + time.sleep(0.1) # step past the filesystem's timestamp resolution + source_path.chmod(0o444) + try: + with pytest.raises( + http.CloudError, match="changed while the source archive was being built" + ): + source_upload._write_archive(tmp_path / "source.zip", manifest.files) + finally: + source_path.chmod(0o644) + + +def test_archive_rejects_a_metadata_change_during_the_read( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + (tmp_path / "app.py").write_bytes(b"safe") + manifest = source_upload.select_source(tmp_path) + real_fstat = os.fstat + calls = 0 + + def fstat_changing_after_the_first_call(fileno: int) -> Any: + nonlocal calls + calls += 1 + info = real_fstat(fileno) + if calls == 1: + return info + return _StatWith(info, st_ctime_ns=info.st_ctime_ns + 1) + + monkeypatch.setattr(source_upload.os, "fstat", fstat_changing_after_the_first_call) + + with pytest.raises(http.CloudError, match="changed while the source archive was being built"): + source_upload._write_archive(tmp_path / "source.zip", manifest.files)