more test coverage improvements#917
Open
tridge wants to merge 8 commits into
Open
Conversation
Both fuzzy tests asserted only that the final file content matched, which a
full transfer that ignored --fuzzy would also satisfy -- so a broken fuzzy
basis selection would pass undetected. Drive rsync directly with --debug=FUZZY
and assert the generator reports the expected basis ("fuzzy basis selected
for <f>: <basis>", generator.c find_fuzzy): rsync2.c for fuzzy, and the
closest-named candidate archive-v1.tar for fuzzy-basis. fuzzy switches from
checkit() to a manual run plus verify_dirs() so the output can be captured.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
compress-options only checked that each requested algorithm yielded byte-identical output, which proves parsing/non-corruption but not that the advertised algorithm was actually used -- the test would pass if the choice were silently ignored. Capture --debug=NSTR (compat.c / checksum.c) and assert the selected compressor, compress level, and checksum match the request (anchored so zlib != zlibx). --skip-compress / --checksum-seed stay content checks: they have no comparable negotiation-string signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Several subcases ran rsync without checking the exit status, so a silent failure could pass as the expected (often empty) output -- most notably -q, which only asserted empty stdout. Route every expected-success run through a helper that asserts the exit status, and verify -q actually transferred the tree. Replace the "-h/-8 didn't break the transfer" check with positive format assertions: -h must render byte counts with a K/M/G suffix (and the default must not), and -8 must leave a high-bit filename byte unescaped (\RsyncProject#371 absent) where the default escapes it -- best-effort, self-skipping where the platform can't store the raw byte. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The symlink-race tests only asserted that an outside sentinel was unchanged or unlisted while ignoring rsync's exit status, so an attack transfer/listing that failed before reaching the vulnerable receiver/sender path would pass without the security property ever being exercised. Add a positive control to each -- an ordinary in-module write (bare-do-open, chdir) or an in-module listing (sender-flist-leak) that must succeed -- so a globally broken/refusing daemon can no longer make the sentinel checks vacuous, and assert the attack run did not die from a signal. clean-fname-underflow now also enforces a non-zero exit: clean_fname() collapses "a/../test" to "test", whose merge file is absent, so rsync must reject it; accepting it (rc 0) would mean the crafted name was mis-collapsed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These daemon tests confirmed refusals/exclusions but accepted the allowed
transfers on exit status alone, so a transfer that exited cleanly while moving
nothing would pass:
daemon-refuse allowed() imported verify_dirs but never called it; now it
confirms the allowed push/pull actually populated the dest.
daemon-filter pull()/the incoming push ignored their exit status, and the
outgoing-chmod loop iterated only files that exist -- a
zero-file pull passed vacuously. Check the codes and require
at least one file to have been mode-checked.
daemon run_and_check's unused `expected` param is dropped; the
hidden-module and glob listings now compare the exact set of
listed paths (catching a leaked extra path), replacing the
per-path containment check and the dead normalise() helper
whose regex never matched the -r listing format anyway.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Several tests proved only that rsync exited cleanly (or that a file merely
exists), so a no-op/short transfer would pass:
protected-regular compare the dst bytes to the source after --inplace.
00-hello re-assert one/two were copied on the RSYNC_OLD_ARGS=1
env-var path (the explicit --old-args case already did).
missing check the dry-run's exit status in test 1.
mkpath compare transferred bytes (not just existence) and add a
negative control: a transfer WITHOUT --mkpath must fail
and create no intermediate path.
size-filter compare each kept file's content to its source.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
symlink-dirlink-basis assert the --backup file holds the pre-update content,
not merely that the backup file exists.
acls-default check that clearing the inherited default ACL actually
succeeded, so the no-default-ACL cases can't silently
test against the scratch dir's seeded default ACL.
alt-dest assert --copy-dest produces a distinct inode from the
alt-dir candidate (a copy, not a hard link) -- the
property that distinguishes it from --link-dest, which
checkit's tree comparison alone doesn't capture.
(crtimes' "independently pin the historical create time" gap is left as-is: the
touch-trick pinning is APFS-specific and not locally verifiable, and a mistuned
probe would make the test skip on macOS and break its expected-skip set.)
Co-Authored-By: Claude Opus 4.7 (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.
thanks to codex for analysing corner cases