From a70997e6c9e1444a898866dc2a8935c4239fa3ee Mon Sep 17 00:00:00 2001 From: Stuart Inglis Date: Mon, 20 Jul 2026 09:42:47 +1200 Subject: [PATCH 1/2] util: restore ".." collapsing in clean_fname() Commit 9e089846 ("util: fixed issue in clean_fname()") fixed a genuine read-before-buffer in the ".." back-up loop, but the replacement loop leaves 's' in a different place than the one it replaced. The old loop while (s > limit && *--s != '/') {} pre-decremented, so it stopped with 's' pointing *at* the preceding '/'. The new loop while (s > limit && s[-1] != '/') s--; stops with 's' pointing at the *start* of the preceding component, one byte past that '/'. The surrounding test and assignment were carried over unchanged, so they are now off by one: if (s != t - 1 && (s <= name || *s == '/')) { t = (s == name) ? name : s + 1; '*s' is a component character and is essentially never '/', so the collapse only still happens via the "s == name" case -- a single leading component with no slash before it. Every other form silently stops collapsing: a/../b -> b (still worked) /a/../b -> /a/../b (should be /b) a/b/../c -> a/b/../c (should be a/c) /usr/local/../bin -> /usr/local/../bin (should be /usr/bin) x/y/../../z -> x/y/../../z (should be z) Adjust the two spots for where the new loop actually stops: check the byte before 's' for the '/' (or that we reached the start of the name), and set 't = s'. This keeps the underflow fix -- 's[-1]' is only read when 's' is past 'name', and the "s == name" test short-circuits -- while restoring the long-standing collapsing behaviour. clean_fname(..., CFN_COLLAPSE_DOT_DOT_DIRS) is rsync's canonical path normalizer, used for filter and merge-file names and for the --backup-dir, --partial-dir and --temp-dir options, so paths containing an interior ".." were left un-normalized. The CFN_REFUSE_DOT_DOT_DIRS path returns before this code and is unaffected, as is sanitize_path(). testsuite/clean-fname-collapse_test.py exercises the collapse through a merge-filter filename: the filter file is at "a/c" but is named as "a/none/../c" with no "a/none" directory, so it can only be opened if ".." is collapsed. It fails before this change and passes after. --- testsuite/clean-fname-collapse_test.py | 68 ++++++++++++++++++++++++++ util1.c | 11 +++-- 2 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 testsuite/clean-fname-collapse_test.py diff --git a/testsuite/clean-fname-collapse_test.py b/testsuite/clean-fname-collapse_test.py new file mode 100644 index 000000000..0f48211e6 --- /dev/null +++ b/testsuite/clean-fname-collapse_test.py @@ -0,0 +1,68 @@ +#!/usr/bin/env python3 +"""Regression test for clean_fname()'s ".." collapsing. + +clean_fname(name, CFN_COLLAPSE_DOT_DOT_DIRS) is rsync's canonical path +normalizer. Commit 9e089846 ("util: fixed issue in clean_fname()") fixed a +read-before-buffer in the ".." back-up loop, but the new loop leaves 's' +pointing at the *start* of the preceding component (just past its '/'), +whereas the old loop left it *at* the '/'. The surrounding test and +assignment (`*s == '/'` / `t = s + 1`) were not adjusted for that one-byte +shift, so the collapse silently stopped working for every path except a +single leading no-slash component: + + a/../b -> b (still worked) + /a/../b -> /a/../b (BROKEN: should be /b) + a/b/../c -> a/b/../c (BROKEN: should be a/c) + x/y/../../z -> x/y/../../z(BROKEN: should be z) + +We exercise it through a merge-filter filename, one of the callers that pass +CFN_COLLAPSE_DOT_DOT_DIRS. The filter file lives at "a/c"; we reference it as +"a/none/../c" where the "none" directory does NOT exist. A correct collapse +turns that into "a/c" (which opens), so the filter is read and applied; the +buggy code leaves the literal "a/none/../c", which fails to open (ENOENT) and +aborts the whole run. This is also a faithful reproduction of the user-facing +regression: merge/exclude files referenced via a path containing "..". +""" + +import os + +from rsyncfns import ( + FROMDIR, TODIR, rmtree, run_rsync, test_fail, +) + +rmtree(FROMDIR) +rmtree(TODIR) +FROMDIR.mkdir(parents=True, exist_ok=True) +TODIR.mkdir(parents=True, exist_ok=True) + +# Source: one file to keep, one the filter should drop. +(FROMDIR / 'keep.txt').write_text('keep\n') +(FROMDIR / 'drop.skip').write_text('drop\n') + +# Merge-filter file at a/c; note that a/none does NOT exist, so the path +# "a/none/../c" only resolves if rsync collapses ".." to a plain "a/c". +adir = TODIR.parent / 'a' +rmtree(adir) +adir.mkdir(parents=True, exist_ok=True) +(adir / 'c').write_text('- *.skip\n') + +saved = os.getcwd() +os.chdir(TODIR.parent) +try: + proc = run_rsync('-a', '--filter=merge a/none/../c', + f'{FROMDIR}/', f'{TODIR}/', check=False) +finally: + os.chdir(saved) + +if proc.returncode != 0: + test_fail( + "rsync failed to open the merge filter via 'a/none/../c'; " + "clean_fname() did not collapse '..' (regression of 9e089846)") + +if not (TODIR / 'keep.txt').is_file(): + test_fail("keep.txt was not transferred") +if (TODIR / 'drop.skip').exists(): + test_fail("drop.skip was transferred -- the merge filter was not applied, " + "so clean_fname() resolved the filter path to the wrong file") + +print("OK: clean_fname() collapsed 'a/none/../c' -> 'a/c' and the filter applied") diff --git a/util1.c b/util1.c index d9d9f4bcf..061f41baa 100644 --- a/util1.c +++ b/util1.c @@ -1004,13 +1004,16 @@ int clean_fname(char *name, int flags) f += 2; continue; } - /* backing up for ".." — avoid reading before 'name' */ + /* backing up for ".." — avoid reading before 'name'. + * The loop stops with 's' pointing at the start of the + * preceding component (just past its '/'), or at 'limit'. */ while (s > limit && s[-1] != '/') s--; - /* If found prior '/', or we reached the start, adjust t. */ - if (s != t - 1 && (s <= name || *s == '/')) { - t = (s == name) ? name : s + 1; + /* If we backed up over a real component (found a prior + * '/' or reached the start of the name), drop it. */ + if (s != t - 1 && (s == name || s[-1] == '/')) { + t = s; f += 2; continue; } From e42f5917703b1318c9df1d6df10e18e7b7d2cf9f Mon Sep 17 00:00:00 2001 From: Stuart Inglis Date: Mon, 20 Jul 2026 13:34:51 +1200 Subject: [PATCH 2/2] testsuite: cover the other two ".." shapes in clean-fname-collapse Mutation testing on clean_fname() showed the test only pinned one of the three shapes the ".." logic has to get right. Fifteen small edits to the function were applied and the suite re-run; ten survived, and classifying them against a reference model of the intended semantics showed six of those changed behaviour and were simply not being detected. Notably, restoring the pre-fix "limit = name - 1" initialiser -- i.e. a partial revert of this very fix -- still passed, as did turning the "s == name" test into "s != name", which both stops the collapse and reintroduces the out-of-bounds read that 9e089846 set out to remove. The gap was that "a/none/../c" only exercises a ".." that eats a component with a '/' in front of it. Two more merge-filter names cover the rest: "zzz/../f1" eats the leading component, where the back-up reaches the start of the name and there is no earlier '/' to find, and "../../updir" (run from two directories down) has nothing to collapse into and must be preserved. Each name resolves only if clean_fname() handles that shape exactly right; otherwise the merge file cannot be opened, or a different file is opened and *.skip is not filtered out. That takes the kill rate on behaviour-changing mutants from 5/11 to 10/11. Of the five that still survive, four are equivalent mutants -- the reference model confirms they alter no output -- so they cannot be killed by any test. The remaining one (advancing the scan by one byte instead of two) only shows up on a name ending in "..", which cannot be expressed as a merge-filter file, so it is left uncovered rather than contorting the test to reach it. --- testsuite/clean-fname-collapse_test.py | 73 +++++++++++++++++++------- 1 file changed, 53 insertions(+), 20 deletions(-) diff --git a/testsuite/clean-fname-collapse_test.py b/testsuite/clean-fname-collapse_test.py index 0f48211e6..0d05b9272 100644 --- a/testsuite/clean-fname-collapse_test.py +++ b/testsuite/clean-fname-collapse_test.py @@ -46,23 +46,56 @@ adir.mkdir(parents=True, exist_ok=True) (adir / 'c').write_text('- *.skip\n') -saved = os.getcwd() -os.chdir(TODIR.parent) -try: - proc = run_rsync('-a', '--filter=merge a/none/../c', - f'{FROMDIR}/', f'{TODIR}/', check=False) -finally: - os.chdir(saved) - -if proc.returncode != 0: - test_fail( - "rsync failed to open the merge filter via 'a/none/../c'; " - "clean_fname() did not collapse '..' (regression of 9e089846)") - -if not (TODIR / 'keep.txt').is_file(): - test_fail("keep.txt was not transferred") -if (TODIR / 'drop.skip').exists(): - test_fail("drop.skip was transferred -- the merge filter was not applied, " - "so clean_fname() resolved the filter path to the wrong file") - -print("OK: clean_fname() collapsed 'a/none/../c' -> 'a/c' and the filter applied") +# Interior collapse only exercises the case where the ".." eats a component +# that has a '/' before it. Two more filter files pin the other two branches: +# 'f1' for a leading component (the s == name case, where there is no earlier +# '/' to find) and 'updir' for a leading ".." that must NOT be collapsed away. +(TODIR.parent / 'f1').write_text('- *.skip\n') +(TODIR.parent / 'updir').write_text('- *.skip\n') +deepdir = TODIR.parent / 'sub1' / 'sub2' +rmtree(deepdir) +deepdir.mkdir(parents=True, exist_ok=True) + + +def check(rundir, filter_path, what): + """Run with the merge filter named via `filter_path` and require that the + file was found *and* applied. Each name only resolves when clean_fname() + collapses (or preserves) the ".." exactly right; get it wrong and the open + fails or lands on a different file, so nothing filters *.skip out.""" + rmtree(TODIR) + TODIR.mkdir(parents=True, exist_ok=True) + saved = os.getcwd() + os.chdir(rundir) + try: + proc = run_rsync('-a', f'--filter=merge {filter_path}', + f'{FROMDIR}/', f'{TODIR}/', check=False) + finally: + os.chdir(saved) + + if proc.returncode != 0: + test_fail(f"rsync could not open the merge filter via '{filter_path}' " + f"({what}); clean_fname() mishandled the '..'") + if not (TODIR / 'keep.txt').is_file(): + test_fail(f"keep.txt was not transferred ({what})") + if (TODIR / 'drop.skip').exists(): + test_fail(f"drop.skip was transferred ({what}): the merge filter was " + "not applied, so clean_fname() resolved the filter path to " + "the wrong file") + + +# Interior component: "a/none/../c" -> "a/c" (a/none does not exist). +check(TODIR.parent, 'a/none/../c', 'interior component') + +# Leading component: "zzz/../f1" -> "f1". Here the back-up reaches the start +# of the name with no '/' before it, so a bounds slip or an off-by-one in that +# test stops the collapse (this is the shape the 9e089846 regression left +# working, and the shape a partial revert breaks). +check(TODIR.parent, 'zzz/../f1', 'leading component') + +# A leading ".." has nothing to collapse into and must be kept: run two levels +# down and reach back up. If it were collapsed away the name would resolve +# inside sub1/sub2, where no such file exists. +check(deepdir, '../../updir', 'leading ".." preserved') + +print("OK: clean_fname() collapsed interior and leading components and kept a " + "leading '..'; the merge filter applied in every case")