From cc61aed10cd7b1dbba50d9e5b911b6c10371b0b8 Mon Sep 17 00:00:00 2001 From: Maksim Shulga Date: Mon, 3 Aug 2026 23:08:24 +0000 Subject: [PATCH] Preserve symlinks and old-snapshot immutability (#9) * Preserve symlinks and old-snapshot immutability Two real bugs found while implementing this item, both in fs.py: - copy_direntry() checked entry.is_dir() (default follow_symlinks=True) before entry.is_symlink(). A symlink pointing at a directory was misclassified as a directory and an empty real directory was created in its place, silently destroying the symlink and losing its target entirely. Fixed by checking is_symlink() first, and is_dir() with follow_symlinks=False. - rsync()'s permission/ownership-sync branch called os.chmod/os.chown on dst_entry.path unconditionally. Two problems: (1) for a symlink whose target already matched, this had no follow_symlinks=False guard and could rewrite an external symlink target's permissions; (2) for a regular file, that path may still be hardlinked to the same file in the previous, completed snapshot (hardlink_dir() links unchanged files in at the start of every backup) - mutating it in place silently corrupted every older snapshot sharing the inode. Fixed by skipping symlinks entirely (nothing to sync once their target matches) and adding _break_shared_hardlink(), a copy-on-write step that gives a file its own inode before any in-place metadata change. Also fixed the dir-mtime-restore loop's is_dir() call (same default-follow_symlinks issue, lower severity since its own operations were already lstat-based/no-follow). Co-Authored-By: Claude Sonnet 5 * Address Copilot review: preserve ownership/timestamps when breaking hardlinks, fix test fd leak - _break_shared_hardlink() now explicitly restores the original file's uid/gid and atime/mtime on the new private copy before it replaces the shared one. copy_file() only carries over mode at create time, so without this a caller that only meant to update permissions would silently also reset ownership/mtime as a side effect of the copy-on-write break itself. Also cleans up the temp file on failure and uses a less collision-prone temp name. - Fixed a file descriptor leak in the new hardlink-immutability test (bare open(...).read() without a context manager) - Strengthened the same test to assert mtime survives the break Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Maxim Shulga Co-authored-by: Claude Sonnet 5 --- IMPROVEMENTS.md | 8 ++--- curateipsum/fs.py | 74 ++++++++++++++++++++++++++++++++++++++++++--- tests/test_fs.py | 26 ++++++++++++++++ tests/test_rsync.py | 74 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 9 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index de80766..f22970c 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -43,10 +43,10 @@ Acceptance criteria: ### Preserve symlinks and old-snapshot immutability -- [ ] Classify entries with `lstat()` and process symlinks before directories. -- [ ] Never follow symlinks while applying symlink metadata. -- [ ] Break shared hardlinks before changing mode, ownership, flags, or times. -- [ ] Define copy-on-write invariants for every mutation. +- [x] Classify entries with `lstat()` and process symlinks before directories. +- [x] Never follow symlinks while applying symlink metadata. +- [x] Break shared hardlinks before changing mode, ownership, flags, or times. +- [x] Define copy-on-write invariants for every mutation. Acceptance criteria: diff --git a/curateipsum/fs.py b/curateipsum/fs.py index 52146d3..e5c00fe 100644 --- a/curateipsum/fs.py +++ b/curateipsum/fs.py @@ -8,6 +8,7 @@ import logging import os import subprocess import sys +import uuid from typing import Iterable, Tuple, Union _lg = logging.getLogger(__name__) @@ -243,16 +244,59 @@ def copy_file(src, dst): pass +def _break_shared_hardlink(path: str): + """ + Give `path` a private inode if it currently shares one with another + file (nlink > 1), so a subsequent in-place metadata change (chmod, + chown, utime, flags) can never mutate that other file. Files copied + into a staging directory via hardlink_dir() start out hardlinked to + the previous snapshot; this is the copy-on-write step that keeps + that previous, completed snapshot immutable. + + The break itself must be metadata-neutral: copy_file() only carries + over the mode at file-creation time, not ownership or timestamps, so + the new private copy is explicitly restored to the original's full + stat before it replaces `path` - otherwise a caller that only meant + to update permissions would silently also reset ownership/mtime as a + side effect of this implementation detail. + """ + st = os.lstat(path) + if st.st_nlink <= 1: + return + tmp_path = "%s.cow-%d-%s" % (path, os.getpid(), uuid.uuid4().hex[:8]) + try: + copy_file(path, tmp_path) + try: + os.chown(tmp_path, st.st_uid, st.st_gid) + except PermissionError: + _lg.debug("Cannot preserve ownership while breaking hardlink " + "(not root): %s", path) + os.chmod(tmp_path, st.st_mode) + os.utime(tmp_path, (st.st_atime, st.st_mtime)) + os.replace(tmp_path, path) + except OSError: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + + def copy_direntry(entry: Union[os.DirEntry, PseudoDirEntry], dst_path): """ Non-recursive DirEntry (file/dir/symlink) copy. """ src_stat = entry.stat(follow_symlinks=False) - if entry.is_dir(): - os.mkdir(dst_path) - - elif entry.is_symlink(): + # Classify with lstat semantics and check symlink-ness first: is_dir() + # (and is_file()) default to follow_symlinks=True, so a symlink + # pointing at a directory would otherwise be misclassified as a + # directory here and get an empty directory created in its place, + # silently losing the symlink and its target. + if entry.is_symlink(): link_target = os.readlink(entry.path) os.symlink(link_target, dst_path) + elif entry.is_dir(follow_symlinks=False): + os.mkdir(dst_path) + else: copy_file(entry.path, dst_path) @@ -416,6 +460,26 @@ def rsync(src_dir, yield rel_path, Actions.ERROR, str(exc) continue + # Symlinks reaching here already have a matching target (handled + # above), and applying chmod/chown to a symlink path without + # follow_symlinks=False support would silently modify whatever + # the symlink points to - including a target outside the backup + # entirely. There's nothing meaningful left to sync for them. + if src_entry.is_symlink(): + continue + + # A regular file reaching here is unchanged in content, but may + # still be hardlinked to the same file in the previous, supposedly + # immutable snapshot (hardlink_dir() links unchanged files in at + # the start of every backup to save space). Break that link + # before mutating permissions/ownership in place, or the change + # would silently apply to every older snapshot sharing the inode. + if src_entry.is_file(follow_symlinks=False): + if src_stat.st_mode != dst_stat.st_mode or ( + src_stat.st_uid != dst_stat.st_uid + or src_stat.st_gid != dst_stat.st_gid): + _break_shared_hardlink(dst_entry.path) + # update permissions and ownership if src_stat.st_mode != dst_stat.st_mode: _lg.debug("Rsync, updating permissions: %s", rel_path) @@ -443,7 +507,7 @@ def rsync(src_dir, # restore dir mtimes in dst, updated by updating files for src_entry in scantree(src_root_abs, dir_first=True): - if not src_entry.is_dir(): + if not src_entry.is_dir(follow_symlinks=False): continue rel_path = src_entry.path[len(src_root_abs) + 1:] dst_path = os.path.join(dst_root_abs, rel_path) diff --git a/tests/test_fs.py b/tests/test_fs.py index 09347e6..a66d280 100644 --- a/tests/test_fs.py +++ b/tests/test_fs.py @@ -285,6 +285,32 @@ class TestCopyDirEntry: dst_stat = os.lstat(dst_path) assert dst_stat.st_mode == src_stat.st_mode + def test_copy_symlink_to_directory_stays_a_symlink(self, tmp_path): + """ + Regression: is_dir()/is_file() default to follow_symlinks=True, + so a symlink pointing at a directory must be classified via + is_symlink() first, or it gets misclassified as a directory and + an empty real directory is created in its place - silently + destroying the symlink and losing its target entirely. + """ + target_dir = os.path.join(str(tmp_path), "target_dir") + src_link = os.path.join(str(tmp_path), "source_link") + dst_link = os.path.join(str(tmp_path), "dest_link") + + os.mkdir(target_dir) + with open(os.path.join(target_dir, "canary"), "w") as f: + f.write("content") + os.symlink(target_dir, src_link) + + with os.scandir(str(tmp_path)) as it: + for entry in it: + if entry.name == "source_link": + fs.copy_direntry(entry, dst_link) + break + + assert os.path.islink(dst_link) + assert os.readlink(dst_link) == target_dir + def test_copy_symlink_preserves_times_if_supported(self, tmp_path): """Test that symlink timestamps are preserved (OS-dependent)""" target_path = os.path.join(str(tmp_path), "target.txt") diff --git a/tests/test_rsync.py b/tests/test_rsync.py index a65bf7f..536f268 100644 --- a/tests/test_rsync.py +++ b/tests/test_rsync.py @@ -635,3 +635,77 @@ class TestRsyncExt: assert results[2][0] == "config.json" assert results[3][0] == "existing.txt" assert results[4][0] == "old/" + + +class TestRsyncHardlinkImmutability: + """ + Regression tests: rsync() is run against a staging directory that + was populated by hardlinking in the previous, completed snapshot + (curateipsum.backup.hardlink_dir). Any in-place metadata change made + while updating staging must never mutate that previous snapshot's + files via the shared inode. + """ + + def test_permission_change_does_not_mutate_shared_hardlink( + self, common_fs_dirs, tmp_path + ): + src_dir, dst_dir = common_fs_dirs + # Independent source file with new content/mtime but the SAME + # eventual permissions the old snapshot file already has - only + # the fallback "update permissions and ownership" branch should + # fire, not a full rewrite (which would already be safe via + # rm+recreate). + src_fpath = create_file(str(src_dir)) + os.chmod(src_fpath, 0o600) + + # Simulate staging (dst_dir) having been hardlinked in from the + # previous, completed snapshot: a separate directory tree outside + # dst_dir, sharing an inode with the staging file, with content + # and mtime matching src (so only permissions differ) but the old + # mode. + dst_fpath = os.path.join(str(dst_dir), + relpath(src_fpath, str(src_dir), str(dst_dir))) + prior_snapshot_fpath = str(tmp_path / "prior_snapshot") + with open(src_fpath) as src_f: + src_content = src_f.read() + with open(prior_snapshot_fpath, "w") as f: + f.write(src_content) + os.chmod(prior_snapshot_fpath, 0o644) + src_stat = os.lstat(src_fpath) + os.utime(prior_snapshot_fpath, (src_stat.st_atime, src_stat.st_mtime)) + os.link(prior_snapshot_fpath, dst_fpath) + prior_mode_before = os.lstat(prior_snapshot_fpath).st_mode + dst_mtime_before = os.lstat(dst_fpath).st_mtime + assert os.lstat(dst_fpath).st_ino == os.lstat( + prior_snapshot_fpath).st_ino # sanity: genuinely shared + + all(fs.rsync(str(src_dir), str(dst_dir))) + + assert os.lstat(dst_fpath).st_mode == os.lstat(src_fpath).st_mode + assert os.lstat(prior_snapshot_fpath).st_mode == prior_mode_before + # dst must now have its own inode, no longer shared + assert os.lstat(dst_fpath).st_ino != os.lstat( + prior_snapshot_fpath).st_ino + # breaking the hardlink must not, as a side effect, reset mtime: + # rsync only intended to change permissions here (content/mtime + # already matched), and uid/gid are identical for this test user + # so the chown branch never fires to "accidentally" fix ownership + assert os.lstat(dst_fpath).st_mtime == dst_mtime_before + + def test_symlink_metadata_never_touches_external_target( + self, common_fs_dirs, tmp_path + ): + src_dir, dst_dir = common_fs_dirs + external_target = tmp_path / "external_target" + external_target.mkdir() + (external_target / "canary").write_text("do-not-touch") + external_mode_before = os.lstat(str(external_target)).st_mode + + link_name = "a_symlink" + os.symlink(str(external_target), os.path.join(str(src_dir), link_name)) + os.symlink(str(external_target), os.path.join(str(dst_dir), link_name)) + + all(fs.rsync(str(src_dir), str(dst_dir))) + + assert os.lstat(str(external_target)).st_mode == external_mode_before + assert (external_target / "canary").read_text() == "do-not-touch"