options: accept --max-alloc=0 again, resolved to the parser's own ceiling (#1069)

* options: accept --max-alloc=0 again, resolved to the parser's own ceiling

3.5.0 rejected --max-alloc=0 (CVE-2026-53794).  The stated rationale was that a
zero cap "disabled the per-allocation size cap (the defense behind
CVE-2024-12084)".  That was true up to 3.2.7, where the check short-circuited:

	if (max_alloc && num >= max_alloc/size)

but 2f9b963a ("Make `--max-alloc=0` safer", 3.3.0) removed the short-circuit and
mapped 0 to SIZE_MAX at parse time, leaving the guard unconditional.  Since the
guard admits an allocation only when num < max_alloc/size, num*size stays below
max_alloc at every setting, so the num*size overflow check was armed for 0 just
as for any other value.  From 3.3.0 onward, 0 raised the magnitude ceiling and
nothing else -- and an explicit 8191P raises it exactly as far, is accepted, and
is forwarded to the peer, so rejecting 0 removed no capability.

What it did remove is the only portable spelling.  The parser's ceiling is
SIZE_MAX/2, so it tracks the build's word size: on ILP32 the suffix multiplier
alone exceeds the bound, making every P and T value an error whatever the digits
and capping the option at 2047M.  Because max_alloc_arg goes on the wire
un-normalized, a 0 was re-resolved by each side against its own SIZE_MAX; any
literal is resolved once on the client and shipped verbatim, so nothing above
2047M survives a 64-bit client talking to a 32-bit daemon.  There is no number a
user can compute that does what 0 did, which is what #1056 ran into.

So accept 0 again, but resolve it to SIZE_ARG_MAX (SIZE_MAX/2) rather than
SIZE_MAX, so it lands exactly on the largest value that could also be typed and
is no longer a limit only the 0 spelling can reach.  Keep forwarding it
verbatim: that per-side resolution is the property worth having.  This leaves
the substantive half of the 3.5.0 hardening -- bounding parse_size_arg() against
the unbounded `size *= atof(size_arg)` -- untouched.

The residual concern, a 0 forwarded to a <= 3.2.7 daemon that honours it, is not
something a client-side check can address: the client is the attacker's own
code, as daemon-max-alloc-zero_test.py noted in its own docstring.  Operators
who want peers kept off their cap have `refuse options = max-alloc`.

Also drops the now-unreachable rejection message, which left the min-value error
recommending a value the parser refused ("min: 1.00M or 0 for unlimited").

Tests: max-alloc-zero replaces max-alloc-zero-rejected and
daemon-max-alloc-zero, whose assertions are the behaviour being reverted.  It
checks that 0 is accepted, that it reaches the peer as the literal "0" rather
than a resolved number (verified against a negative control that normalizes it),
and that the parser's upper bound still rejects an out-of-range value.

Fixes #1056

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* testsuite: refresh Cygwin expectations
* docs: note max-alloc safety

options.c

@@ -1157,6 +1157,12 @@ static int count_args(const char **argv)
 	return i;
 }
 
+/* The largest value parse_size_arg() will accept when no explicit max_value is
+ * given.  It is SIZE_MAX/2 rather than SIZE_MAX because the parser computes and
+ * returns the size as a signed ssize_t (with a negative return meaning error),
+ * so this keeps every accepted size representable as a positive ssize_t. */
+#define SIZE_ARG_MAX ((ssize_t)(SIZE_MAX / 2))
+
 /* If the size_arg is an invalid string or the value is < min_value, an error
  * is put into err_buf & the return is -1.  Note that this parser does NOT
  * support negative numbers, so a min_value < 0 doesn't make any sense. */
@@ -1166,7 +1172,7 @@ static ssize_t parse_size_arg(const char *size_arg, char def_suf, const char *op
 	int reps, mult, len;
 	const char *arg, *err = "invalid", *min_max = NULL;
 	ssize_t limit = -1, size = 1;
-	ssize_t size_max = max_value >= 0 ? max_value : (ssize_t)(SIZE_MAX / 2);
+	ssize_t size_max = max_value >= 0 ? max_value : SIZE_ARG_MAX;
 	double dsize;
 
 	for (arg = size_arg; isDigit(arg); arg++) {}
@@ -2067,14 +2073,17 @@ int parse_arguments(int *argc_p, const char ***argv_p)
 		ssize_t size = parse_size_arg(max_alloc_arg, 'B', "max-alloc", 1024*1024, -1, True);
 		if (size < 0)
 			goto cleanup;
-		if (size == 0) {
-			snprintf(err_buf, sizeof err_buf, "max-alloc must be greater than zero\n");
-			goto cleanup;
-		}
 		max_alloc = size;
 	}
+	/* A 0 value means "as large as this build allows".  We resolve it to the
+	 * same ceiling parse_size_arg() enforces, so that --max-alloc=0 is exactly
+	 * the largest value a user could also have typed, and never a limit that
+	 * only the 0 spelling can reach.  Note that max_alloc_arg is forwarded to
+	 * the peer un-normalized (see server_options()), which is what lets each
+	 * side resolve 0 against its own SIZE_MAX -- a 64-bit client and a 32-bit
+	 * daemon each get their own ceiling from the one portable spelling. */
 	if (!max_alloc)
-		max_alloc = SIZE_MAX;
+		max_alloc = SIZE_ARG_MAX;
 
 	if (old_style_args < 0) {
 		if (!am_server && protect_args <= 0 && (arg = getenv("RSYNC_OLD_ARGS")) != NULL && *arg) {

rsync.1.md

@@ -2339,12 +2339,22 @@ sign) if you want the local shell to expand it.
     See the [`--max-size`](#opt) option for a description of how SIZE can be
     specified.  The default suffix if none is given is bytes.
 
-    Beginning in 3.2.7, a value of 0 was an easy way to specify SIZE_MAX (the
-    largest limit possible).  However, beginning with 3.5.0, a value of 0 is
-    rejected as invalid for security reasons (a 0-byte cap could be used to
-    disable the allocation limit, which could lead to a denial-of-service via
-    memory exhaustion).  Use an explicit very large value if you want a very
-    high limit.
+    A value of 0 is an easy way to say "the largest limit this build supports".
+    It resolves to the same ceiling an explicit SIZE is checked against, so it
+    is never a higher limit than one you could have typed out yourself.
+
+    Because the option is passed to the remote rsync as you wrote it, each side
+    resolves a 0 against its own maximum.  That makes 0 the only spelling that
+    is correct for both ends of a transfer between hosts of different word
+    sizes: a literal value large enough to be useful on a 64-bit client is
+    rejected as too large by a 32-bit daemon.
+
+    A value of 0 was accepted beginning in 3.2.3 and rejected in 3.5.0; the
+    release after 3.5.0 accepts it again.
+
+    A daemon administrator who does not want clients to change the configured
+    allocation ceiling can set `refuse options = max-alloc` in the module's
+    `rsyncd.conf`.  This refuses every client-supplied value, including 0.
 
     You can set a default value using the environment variable
     [`RSYNC_MAX_ALLOC`](#) using the same SIZE values as supported by this

testsuite/daemon-max-alloc-zero_test.py

@@ -1,83 +0,0 @@
-#!/usr/bin/env python3
-"""Daemon-mode: the server must reject a wire-supplied --max-alloc=0.
-
-max-alloc-zero-rejected_test.py only proves the *local* client refuses
---max-alloc=0. That alone doesn't protect a daemon: a modified or older client
-still forwards --max-alloc=0 on the wire, and an unpatched daemon honours it and
-disables its my_alloc() allocation cap (the defence behind CVE-2024-12084 and
-friends). This test drives an older rsync client -- which lacks the reject-zero
-check and so forwards the option -- against the current rsync daemon, and
-asserts the *daemon* refuses it.
-
-It uses the in-tree old_versions/rsync_3.2.7 as the client (3.2.7 predates the
-reject-zero fix, so it forwards --max-alloc=0 on the wire). If that binary is
-missing or can't run here (e.g. a non-Linux host that can't run the static
-archive) the test skips.
-"""
-
-import subprocess
-from pathlib import Path
-
-from rsyncfns import (
-    FROMDIR, RSYNC, SCRATCHDIR,
-    makepath, rmtree, start_test_daemon, test_fail, test_skipped,
-    write_daemon_conf,
-)
-
-DAEMON_PORT = 12932
-REJECT_MSG = 'max-alloc must be greater than zero'
-
-OLD_CLIENT = Path(__file__).resolve().parents[1] / 'old_versions' / 'rsync_3.2.7'
-
-if not OLD_CLIENT.exists():
-    test_skipped(f"{OLD_CLIENT} not present")
-
-# Confirm the static binary actually runs as rsync on this OS/arch before we
-# depend on it: exec of a foreign-arch/OS binary raises OSError, while one that
-# loads but can't run won't print the rsync banner. (3.2.7 predates the
-# reject-zero fix, so once it runs it forwards --max-alloc=0 on the wire.)
-try:
-    probe = subprocess.run([str(OLD_CLIENT), '--version'],
-                           stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-                           text=True)
-except OSError as e:
-    test_skipped(f"cannot run {OLD_CLIENT.name} on this OS/arch: {e}")
-if probe.returncode != 0 or 'version 3.2.7' not in probe.stdout:
-    test_skipped(f"{OLD_CLIENT.name} does not run as rsync on this OS/arch")
-
-# Module served by the *current* (patched) daemon.
-src = FROMDIR
-rmtree(src)
-makepath(src)
-(src / 'file.txt').write_text('hello\n')
-
-conf = write_daemon_conf([('mod', {'path': str(src), 'read only': 'yes'})])
-url = start_test_daemon(conf, DAEMON_PORT, rsync_cmd=RSYNC)
-
-dest = SCRATCHDIR / 'out.txt'
-
-
-def run_client(*extra):
-    argv = [str(OLD_CLIENT), *extra, f'{url}mod/file.txt', str(dest)]
-    return subprocess.run(argv, stdout=subprocess.DEVNULL,
-                          stderr=subprocess.PIPE, text=True)
-
-
-# Positive control: the old client and current daemon transfer fine without the
-# option, so the failure below is specifically the daemon refusing the option.
-dest.unlink(missing_ok=True)
-ctrl = run_client()
-if ctrl.returncode != 0:
-    test_fail(f"old client could not talk to the current daemon:\n{ctrl.stderr}")
-
-# The attack: a forwarded --max-alloc=0 must be refused by the daemon.
-dest.unlink(missing_ok=True)
-proc = run_client('--max-alloc=0')
-if proc.returncode == 0:
-    test_fail("daemon accepted a wire-supplied --max-alloc=0")
-if REJECT_MSG not in proc.stderr:
-    test_fail("daemon did not reject --max-alloc=0 with the expected message; "
-              f"stderr:\n{proc.stderr}")
-
-print("daemon-max-alloc-zero: daemon refuses a wire-supplied --max-alloc=0 "
-      f"(client {OLD_CLIENT.name})")

testsuite/max-alloc-zero-rejected_test.py

@@ -1,9 +0,0 @@
-#!/usr/bin/env python3
-from rsyncfns import SCRATCHDIR, rsync_argv
-from rsyncfns import expect_fail
-
-expect_fail(
-    rsync_argv('--max-alloc=0', str(SCRATCHDIR / 'missing-src'), str(SCRATCHDIR / 'missing-dst')),
-    'max-alloc must be greater than zero',
-)
-print("max-alloc-zero-rejected: --max-alloc=0 is rejected")

testsuite/max-alloc-zero_test.py

@@ -0,0 +1,88 @@
+#!/usr/bin/env python3
+"""``--max-alloc=0`` means "the largest limit this build supports".
+
+Three things are asserted, and the second is the reason 0 is worth keeping as a
+spelling at all:
+
+  1. 0 is accepted, and a transfer using it works.
+
+  2. 0 reaches the peer as the literal "0", not as a resolved number.  Each side
+     then resolves it against its own SIZE_MAX.  That is what makes 0 the only
+     value correct for both ends of a mixed-word-size pairing: the ceiling is
+     SIZE_MAX/2, so any number large enough to be worth setting on a 64-bit
+     client (over 2047M) is refused as "too large" by a 32-bit daemon.
+
+  3. The parser's upper bound is still enforced.  Accepting 0 again must not
+     bring back the unbounded ``size *= atof(size_arg)`` that was fixed in
+     3.5.0, so an out-of-range value is still rejected rather than wrapping.
+
+The forwarding check in (2) deliberately inspects the argv the remote shell is
+handed rather than a transfer outcome: a resolved number also copies files
+happily on a same-word-size pair, so an outcome-based assertion would pass on
+exactly the configuration this behaviour does not matter for.
+"""
+
+import os
+import shlex
+
+from rsyncfns import (
+    SCRATCHDIR, SRCDIR, expect_fail, rsh_cmd, rmtree, rsync_argv,
+    rsync_path_arg, run_rsync, test_fail,
+)
+
+base = SCRATCHDIR / 'max-alloc-zero'
+rmtree(base)
+src = base / 'from'
+dst = base / 'to'
+src.mkdir(parents=True)
+dst.mkdir(parents=True)
+(src / 'file.txt').write_text('hello\n')
+
+# --- 1. 0 is accepted -------------------------------------------------------
+
+run_rsync('-r', '--max-alloc=0', f'{src}/', f'{dst}/')
+if (dst / 'file.txt').read_text() != 'hello\n':
+    test_fail('--max-alloc=0 did not copy the file')
+
+# --- 2. 0 goes on the wire un-normalized ------------------------------------
+
+argv_log = base / 'server-argv'
+wrapper = base / 'log-rsh.sh'
+wrapper.write_text(
+    '#!/bin/sh\n'
+    '# Log the command line built for the peer, then behave like lsh.sh.\n'
+    f'printf \'%s\\n\' "$*" >> {shlex.quote(str(argv_log))}\n'
+    f'exec {shlex.quote(str(SRCDIR / "support" / "lsh.sh"))} "$@"\n'
+)
+wrapper.chmod(0o755)
+
+rmtree(dst)
+dst.mkdir()
+os.environ['RSYNC_RSH'] = rsh_cmd(str(wrapper))
+run_rsync('-r', '--max-alloc=0', f'--rsync-path={rsync_path_arg()}',
+          f'localhost:{src}/', f'{dst}/')
+del os.environ['RSYNC_RSH']
+
+if (dst / 'file.txt').read_text() != 'hello\n':
+    test_fail('--max-alloc=0 did not copy the file over the remote shell')
+
+logged = argv_log.read_text() if argv_log.exists() else ''
+if not logged:
+    test_fail('the remote-shell wrapper logged no command line')
+if '--max-alloc=0' not in f' {logged} '.replace('\n', ' '):
+    test_fail('--max-alloc=0 was not forwarded verbatim; the peer was sent:\n'
+              f'{logged}'
+              '\nA resolved number here would be rejected as "too large" by a '
+              'peer with a smaller SIZE_MAX.')
+
+# --- 3. the upper bound still holds -----------------------------------------
+
+# 8192P is one step past SIZE_ARG_MAX (SIZE_MAX/2) on a 64-bit build; on a
+# 32-bit one the P multiplier alone already exceeds it.  Either way: too large.
+expect_fail(rsync_argv('--max-alloc=8192P', f'{src}/', f'{dst}/'), 'is too large')
+
+# And the min-value message must keep advertising a spelling that works.
+expect_fail(rsync_argv('--max-alloc=1', f'{src}/', f'{dst}/'),
+            'or 0 for unlimited')
+
+print('max-alloc-zero: 0 is accepted, forwarded verbatim, and the bound holds')

testsuite/skiplist/cygwin.txt

@@ -25,7 +25,6 @@ copy-xattrs-symlink-race
 daemon-auth-group
 daemon-chroot-munge-default
 daemon-config-symlink
-daemon-max-alloc-zero
 daemon-module-chdir-symlink
 daemon-module-private-parent
 daemon-secrets-file-symlink
@@ -63,7 +62,6 @@ sender-remove-source-root-anchor
 simd-checksum
 source-change-size-continues
 symlink-dest-backupdir
-symlink-exclude-xattr
 symlink-race-dest
 symlink-race-relative-dest
 temp-dir-symlink-injection

testsuite/skiplist/macos.txt

@@ -14,7 +14,6 @@ backup-crossdev-copy
 chmod-temp-dir
 copy-xattrs-symlink-race
 daemon-auth-group
-daemon-max-alloc-zero
 dir-sgid
 fake-super-acl-xattr
 link-dest-symlink-enotsup                # the ENOTSUP hard-link hook is an LD_PRELOAD, Linux-only