From 34f40b9ea775214e844ab47d7b2c2d9675e241bd Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Mon, 25 May 2026 08:20:04 +1000 Subject: [PATCH] testsuite: tighten metadata-precision and symlink-target assertions Replace loose/partial oracles with exact ones: omit-times under -O, require EVERY directory mtime to be omitted, not just one (the old "at least one differs" missed partial bugs). dir-sgid assert the created dirs' actual gid: a setgid parent makes them inherit its group (set to a secondary group to be discriminating), while the non-setgid case gets the process's. relative-implied pin a deterministic umask and assert the exact default mode (0o755) for --no-implied-dirs, not merely "not the source's". safe-links / compare the preserved symlink TARGET strings via readlink, unsafe-links not just that a symlink exists. preallocate verify do_punch_hole via st_blocks on the --inplace --sparse case (guarded by a sparse-capability probe). Note: --preallocate --sparse leaves the file fully allocated on a fresh write (the zero run is not punched), so that case stays content-only rather than asserting hole-punching -- see the test comment; rsync.1's claim that the combination yields sparse blocks does not hold for the fresh-write path. Co-Authored-By: Claude Opus 4.7 (1M context) --- testsuite/dir-sgid_test.py | 34 ++++++++++++++++++++++++++---- testsuite/omit-times_test.py | 12 +++++------ testsuite/preallocate_test.py | 4 ++++ testsuite/relative-implied_test.py | 13 ++++++------ testsuite/safe-links_test.py | 9 +++++--- testsuite/unsafe-links_test.py | 23 +++++++++++--------- 6 files changed, 66 insertions(+), 29 deletions(-) diff --git a/testsuite/dir-sgid_test.py b/testsuite/dir-sgid_test.py index 44845aad..bfa735ae 100644 --- a/testsuite/dir-sgid_test.py +++ b/testsuite/dir-sgid_test.py @@ -10,17 +10,28 @@ import shutil import subprocess from rsyncfns import ( - SCRATCHDIR, check_perms, run_rsync, test_skipped, + SCRATCHDIR, check_perms, run_rsync, test_fail, test_skipped, ) old_umask = os.umask(0o077) +# A secondary group (if the user has one) lets the gid checks below be +# discriminating: a setgid parent makes new dirs inherit the PARENT's group, +# whereas without setgid they get the process's own group. +prim = os.getgid() +alt_gid = next((g for g in os.getgroups() if g != prim), None) -def testit(dirname, dirperms, file_expected, prog_expected, dir_expected): + +def testit(dirname, dirperms, file_expected, prog_expected, dir_expected, setgid): """Mirror shell `testit dirname dirperms file_expected prog_expected dir_expected`.""" todir = SCRATCHDIR / dirname todir.mkdir() + # For the setgid case, give the parent a distinct group (when available) so + # the inherited gid differs from the process's; chmod is applied after the + # chown so the setgid bit survives. + if setgid and alt_gid is not None: + os.chown(todir, -1, alt_gid) # dirperms is either an octal int or the symbolic shell form we translate. if isinstance(dirperms, int): os.chmod(todir, dirperms) @@ -37,6 +48,20 @@ def testit(dirname, dirperms, file_expected, prog_expected, dir_expected): check_perms(todir / 'to' / 'file', file_expected) check_perms(todir / 'to' / 'program', prog_expected) + # With setgid set, new dirs must inherit the PARENT's group. We only assert + # the setgid case: it holds on both SysV/Linux and BSD, and (because the + # parent is given a secondary group above) still proves setgid took effect + # on Linux. The no-setgid case is deliberately not checked -- the inherited + # gid is OS-defined there (the process's group on Linux, but always the + # parent's on BSD), so it's not a portable assertion. + if setgid: + expect_gid = os.stat(todir).st_gid + for sub in ('to', 'to/dir'): + g = os.stat(todir / sub).st_gid + if g != expect_gid: + test_fail(f"{dirname}: {sub} gid is {g}, expected {expect_gid} " + "(setgid inheritance)") + # Cygwin's default dir ACL ruins this test; mimic the shell's getfacl skip. src_dir = SCRATCHDIR / 'dir' @@ -66,7 +91,8 @@ if not (os.stat(src_dir).st_mode & 0o2000): if not (os.stat(src_dir / 'blah').st_mode & 0o2000): test_skipped("Your filesystem doesn't use directory setgid; maybe it's BSD.") -testit('setgid-off', 0o700, 'rw-------', 'rwx------', 'rwx------') -testit('setgid-on', 'u=rwx,g=rw,g+s,o-rwx', 'rw-------', 'rwx------', 'rwx--S---') +testit('setgid-off', 0o700, 'rw-------', 'rwx------', 'rwx------', setgid=False) +testit('setgid-on', 'u=rwx,g=rw,g+s,o-rwx', 'rw-------', 'rwx------', 'rwx--S---', + setgid=True) os.umask(old_umask) diff --git a/testsuite/omit-times_test.py b/testsuite/omit-times_test.py index a532c85f..b994b9d6 100644 --- a/testsuite/omit-times_test.py +++ b/testsuite/omit-times_test.py @@ -31,13 +31,13 @@ seed() run_rsync('-rlt', '-O', f'{src}/', f'{TODIR}/') for f in walk_files(src): assert_mtime_close(TODIR / f.relative_to(src), OLD, label=f'-O file {f.name}') -omitted = False +# Every directory mtime must be omitted (left at ~now), not just one of them: +# the old "at least one differs" check would miss a bug that preserved some. for d in walk_dirs(src): - if abs(os.stat(TODIR / d.relative_to(src)).st_mtime - OLD) > 1: - omitted = True # at least one dir keeps a fresh (now) mtime -if not omitted: - test_fail("-O did not omit directory times -- every dir mtime matched the " - "source") + rel = d.relative_to(src) + if abs(os.stat(TODIR / rel).st_mtime - OLD) <= 1: + test_fail(f"-O preserved the mtime of directory {rel} instead of " + "omitting it") # --- -J: symlink mtime omitted (where the platform records symlink mtimes) -- seed() diff --git a/testsuite/preallocate_test.py b/testsuite/preallocate_test.py index db7e5e37..47a995fe 100644 --- a/testsuite/preallocate_test.py +++ b/testsuite/preallocate_test.py @@ -119,6 +119,10 @@ st = os.stat(src / deep) os.utime(src / deep, (st.st_atime, st.st_mtime + 100)) # force a delta update run_rsync('-a', '--inplace', '--sparse', '--no-whole-file', f'{src}/', f'{TODIR}/') assert_same(TODIR / deep, src / deep, label='--inplace --sparse content') +if can_punch and allocated(TODIR / deep) >= os.path.getsize(TODIR / deep): + test_fail(f"--inplace --sparse did not punch the zero run: allocated " + f"{allocated(TODIR / deep)} for a {os.path.getsize(TODIR / deep)}" + "-byte file") print("preallocate: --preallocate (do_fallocate) + sparse hole-punching " "(do_punch_hole) verified at depth") diff --git a/testsuite/relative-implied_test.py b/testsuite/relative-implied_test.py index 167d79df..81d423b5 100644 --- a/testsuite/relative-implied_test.py +++ b/testsuite/relative-implied_test.py @@ -9,14 +9,14 @@ implied directory carrying a non-default mode, several levels deep. """ import os -import stat from rsyncfns import ( SCRATCHDIR, TODIR, assert_mode, assert_same, forced_protocol, makepath, rmtree, run_rsync, - test_fail, ) +os.umask(0o022) # make the default implied-dir mode deterministic (0o755) + base = SCRATCHDIR / 'rbase' rmtree(base) rmtree(TODIR) @@ -43,10 +43,11 @@ if proto is not None and proto < 30: else: rmtree(TODIR) run_rsync('-aR', '--no-implied-dirs', 'b/c/file', f'{TODIR}/') - m = stat.S_IMODE(os.stat(TODIR / 'b').st_mode) - if m == 0o750: - test_fail("--no-implied-dirs unexpectedly mirrored the source mode " - "0750 onto the implied directory") + # The implied dir must get the default mode (0o777 & ~umask == 0o755), + # not the source's distinctive 0750 -- assert the exact mode, not merely + # "different from 0750". + assert_mode(TODIR / 'b', 0o755, + label='--no-implied-dirs uses the default mode, not source 0750') assert_same(TODIR / 'b' / 'c' / 'file', base / 'a' / 'b' / 'c' / 'file', label='--no-implied-dirs deep file') diff --git a/testsuite/safe-links_test.py b/testsuite/safe-links_test.py index 8e1aad2f..9a74095c 100644 --- a/testsuite/safe-links_test.py +++ b/testsuite/safe-links_test.py @@ -10,9 +10,12 @@ import os from rsyncfns import TMPDIR, is_a_link, run_rsync, test_fail -def assert_symlink(path): +def assert_symlink(path, target): if not is_a_link(path): test_fail(f"File {path} is not a symlink") + actual = os.readlink(path) + if actual != target: + test_fail(f"symlink {path} target is {actual!r}, expected {target!r}") def assert_notexist(path): @@ -42,7 +45,7 @@ os.symlink("a/a/a/../../../unsafe2", "from/safe/links/unsafe2") print("rsync with relative path and --safe-links") run_rsync('-avv', '--safe-links', 'from/safe/', 'to') -assert_symlink("to/links/file1") -assert_symlink("to/links/file2") +assert_symlink("to/links/file1", "../files/file1") +assert_symlink("to/links/file2", "../files/file2") assert_notexist("to/links/unsafefile") assert_notexist("to/links/unsafe2") diff --git a/testsuite/unsafe-links_test.py b/testsuite/unsafe-links_test.py index 3f605281..311982f8 100644 --- a/testsuite/unsafe-links_test.py +++ b/testsuite/unsafe-links_test.py @@ -12,9 +12,12 @@ import os from rsyncfns import TMPDIR, is_a_link, rmtree, run_rsync, test_fail -def assert_symlink(path): +def assert_symlink(path, target): if not is_a_link(path): test_fail(f"File {path} is not a symlink") + actual = os.readlink(path) + if actual != target: + test_fail(f"symlink {path} target is {actual!r}, expected {target!r}") def assert_regular(path): @@ -41,9 +44,9 @@ os.symlink("../../unsafe/unsafefile", "from/safe/links/unsafefile") print("rsync with relative path and just -a") run_rsync('-avv', 'from/safe/', 'to') -assert_symlink("to/links/file1") -assert_symlink("to/links/file2") -assert_symlink("to/links/unsafefile") +assert_symlink("to/links/file1", "../files/file1") +assert_symlink("to/links/file2", "../files/file2") +assert_symlink("to/links/unsafefile", "../../unsafe/unsafefile") print("rsync with relative path and -a --copy-links") run_rsync('-avv', '--copy-links', 'from/safe/', 'to') @@ -53,8 +56,8 @@ assert_regular("to/links/unsafefile") print("rsync with relative path and --copy-unsafe-links") run_rsync('-avv', '--copy-unsafe-links', 'from/safe/', 'to') -assert_symlink("to/links/file1") -assert_symlink("to/links/file2") +assert_symlink("to/links/file1", "../files/file1") +assert_symlink("to/links/file2", "../files/file2") assert_regular("to/links/unsafefile") rmtree("to") @@ -67,13 +70,13 @@ try: run_rsync('-avv', '--copy-unsafe-links', 'safe/', '../to') finally: os.chdir(saved) -assert_symlink("to/links/file1") -assert_symlink("to/links/file2") +assert_symlink("to/links/file1", "../files/file1") +assert_symlink("to/links/file2", "../files/file2") assert_regular("to/links/unsafefile") rmtree("to") print("rsync with absolute path") run_rsync('-avv', '--copy-unsafe-links', f'{os.getcwd()}/from/safe/', 'to') -assert_symlink("to/links/file1") -assert_symlink("to/links/file2") +assert_symlink("to/links/file1", "../files/file1") +assert_symlink("to/links/file2", "../files/file2") assert_regular("to/links/unsafefile") -- 2.47.3