Skip to content

fix(cli): reject an unknown --info/--debug item the way upstream does - #7637

Merged
oferchen merged 1 commit into
masterfrom
fix/cli-unknown-output-word
Sep 3, 2026
Merged

fix(cli): reject an unknown --info/--debug item the way upstream does#7637
oferchen merged 1 commit into
masterfrom
fix/cli-unknown-output-word

Conversation

@oferchen

@oferchen oferchen commented Sep 2, 2026

Copy link
Copy Markdown
Owner

--debug=" flist" exited 0 silently where upstream exits 1, and every rejection oc did emit used invented wording.

Upstream

One tokenizer serves both options: parse_output_words(words, levels, str, priority) at options.c:443-490, called from OPT_INFO (options.c:1877-1880) and OPT_DEBUG (options.c:1882-1885).

lines shape decision
448-454 comma split, if (!len) continue; zero-length segments are accepted no-ops — --debug=, ,,,, name,. No whitespace trim anywhere.
455-458 if (!isDigit(str)) while (len && isDigit(str+len-1)) len--; trailing digits stripped only when the token does not start with a digit
459-464 lev = …; if (lev > MAX_OUT_LEVEL || lev < 0) lev = MAX_OUT_LEVEL; level clamped to 4 (options.c:261), never rejected
465-468 len==4 && strncasecmp(str,"help",4)==0 output_item_help(); exit_cleanup(0);exits 0 from inside the loop
469-472 none / all zero or set every word
473-483 strncasecmp(str, words[j].name, len) with len == namelen exact-length, case-insensitive
484-488 if (len && !words[j].name && !am_server) rprintf(FERROR, "Unknown %s item: \"%.*s\"\n", words[j].help, len, str); exit_cleanup(RERR_SYNTAX);

At :484 words[j] is the NULL sentinel whose help is the literal "--info" (options.c:301) / "--debug" (options.c:333); len is the digit-stripped length; RERR_SYNTAX is 1 (errcode.h:25). The !am_server half means the server side stays silent on an unknown word.

Measured against the real 3.5.0 binary

Every probed shape that already matched is omitted — flist, flist2, all0, ALL2, none2, HELP, DeLtAsUm, ,,,, ,bogus, 2flist, 99999999999999999999, nobogus, -bogus, stats3, and the --info= twins all agreed on rc and message before this change.

argv upstream before after
--debug= flist rc=1 Unknown --debug item: " flist" rc=0, silent matches
--debug=flist rc=1 Unknown --debug item: "flist " rc=0, silent matches
--info="name2, del1" rc=1 Unknown --info item: " del" rc=0, silent matches
--debug=bogus2 rc=1 Unknown --debug item: "bogus" rc=1, invented wording matches
--debug=help,bogus rc=0, prints the table rc=1 matches
--debug=bogus rc=1 Unknown --debug item: "bogus" rc=1, invented wording matches

Four distinct defects: the trim, the un-stripped token in the message, help not short-circuiting, and the invented text.

oc's line keeps the repo-wide branding envelope (oc-rsync error: … (code 1) at debug.rs(179)), which is the established convention for every option error — --chmod=zzz renders identically. The diagnostic text, stream and exit code are ported; restructuring Message repo-wide is out of scope.

Word-split trap avoided: each cell passed as one argv word via an array, and the non-empty upstream rc=1 rows prove the flags reached the binary.

Seven parser sites enumerated, three changed

site verdict
flags/output_words.rs — shared tokenizer changed — trim dropped, TokenFlow short-circuit, digit-stripped case-preserving base
flags/debug.rs changed
flags/info.rs changed (spec_for also made case-insensitive per options.c:475)
server/run.rs:198-215 unchanged — correctly permissive, mirrors the !am_server half
server/flags.rs:1160-1165 unchanged — server --debug= captured, never parsed; matches upstream silence
logging/src/config.rs:201-303 unchanged — downstream applier, every caller does let _ =; only sees already-accepted tokens
frontend/info_output.rs:250-300 unchanged, flagged: a second parallel parse_info_flags with its own .trim() and its own name table. Publicly re-exported, but its only consumer is tests/output_parity.rs — off the binary's path. Latent recurrence of this exact defect class if ever wired up.

Mutation proof

Filter test(/flags::tests|info_debug/), test files untouched, one predicate reverted per arm:

arm result
baseline 155 run: 155 passed
M1 restore the trim 155 run: 153 passed, 2 failed
M2 report the raw token 155 run: 154 passed, 1 failed
M3 Help returns Continue 155 run: 154 passed, 1 failed
M4 restore the invented wording 155 run: 146 passed, 9 failed

passed + failed == run in every arm. Companions stayed green under M1-M3 (an_unpadded_word_is_still_accepted, a_known_word_with_a_level_suffix_is_still_accepted, an_unknown_item_before_help_still_errors, the_server_side_decoder_still_ignores_an_unknown_item), so "reject everything" cannot satisfy the pins. The CLI companion deliberately avoids --version as terminator — --version short-circuits before --debug is parsed and would make the control vacuous.

Gates

Workspace 32627 run: 32622 passed, 5 failed. All 5 reproduce at pristine HEAD with the changed files reverted (5 run: 0 passed, 5 failed) — APFS sparse-block accounting plus one xtask fixture, unrelated. check_rustfmt_all.py clean (3389 files); clippy -p cli --all-targets silent on touched files; no --help snapshot moved and --debug=help output is byte-identical.

Reported, not fixed

--version / --help precedence is a separate divergence: upstream is strictly argv-ordered (popt handles each option where it sits), oc's maybe_print_help_or_version runs before option validation and wins regardless of position — so --debug=bogus --version exits 0 on oc, 1 upstream. Fixing it means reordering global preflight against every validated option, with broad snapshot blast radius.

Task 775's premise (an oc-invented no…/-… negation prefix) was measured not to reproduce: --debug=nobogus, -bogus, noflist, --info=nodel, -skip all exit 1 matching upstream, already pinned by pre-existing tests, and no prefix stripping exists at any of the 7 sites.

@github-actions github-actions Bot added the bug Something isn't working label Sep 2, 2026
oferchen added a commit that referenced this pull request Sep 3, 2026
`interop (Windows, best-effort)` hung for 25 minutes on PR #7637 and was
killed by the workflow, not by any assertion. The job log ends at

  PASS: baseline upstream local copy

and then emits nothing for 25 minutes until the cancel. Every setup step
had succeeded, including the MSYS2 install.

`wait_for_port` is where it sat, by elimination: the only thing between
that PASS and the next output is `start_daemon`, whose sole unbounded
operation is this readiness loop. The `deadline` is checked once per
iteration, so a probe that never returns defeats it entirely - the loop
never gets back to the test. Neither probe was individually bounded.

On Windows that is not a corner case, it is the normal path. The function's
own comment says `/dev/tcp` is not exposed by Cygwin's bash, so every
iteration falls through to the rsync probe; and a SYN to a port nothing is
listening on is DROPPED rather than refused when the runner's firewall is
in the way, so `connect()` blocks instead of returning ECONNREFUSED.

Measured against a real rsync 3.5.0, running the harness's exact invocation
against a blackhole address (127.0.0.1 always refuses, so the loopback probe
cannot reproduce the case that matters):

  WITH --contimeout=2:  rc=35, 2s
  WITHOUT:              still running at 41s, killed by `timeout`

`--contimeout` bounds the connect (options.c:831 declares it; socket.c:422
arms a SIGALRM around it) and `--timeout` bounds everything after, so each
iteration now costs at most a couple of seconds and the 15-second deadline
means what it says. When the daemon genuinely cannot bind - the documented
Windows condition this function exists to detect - the loop returns 1 in
~15s and the caller takes its existing SKIP path instead of hanging.

Behavioural contract re-measured on the patched function: dead port
rc=1 in 15s, live port rc=0 in 0s. `bash -n` clean; shellcheck finding
count unchanged at 1 (a pre-existing SC2086 on an untouched line).

⚠ This is NOT what reddened #7637. That job's 10 real failures are the
inherited master red; the Windows cell was CANCELLED, and the cancel came
from this hang. The hang is in code that runs the UPSTREAM rsync binary
before oc-rsync is invoked at all, so no oc-side diff can cause it.
upstream: options.c:443-490 `parse_output_words()`. Three parser decisions
diverged from rsync 3.5.0, each measured against the pinned oracle binary at
target/interop/upstream-src/rsync-3.5.0/rsync.

- oc trimmed ASCII whitespace off every comma-separated item, so
  `--debug= flist` was accepted silently. Upstream splits on ',' and skips
  only zero-length segments (options.c:448-454), then compares the raw
  segment bytes with strncasecmp (options.c:475), so a padded word is the
  unknown item `" flist"` and exits RERR_SYNTAX.
- oc echoed the raw token in the diagnostic. Upstream strips the trailing
  level digits before the table lookup (options.c:455-458) and prints
  `"%.*s"` with that shortened length (options.c:485-486), so
  `--debug=BOGUS2` is reported as `BOGUS`.
- oc validated the whole list before honouring `help`. Upstream calls
  exit_cleanup(0) from inside the token loop (options.c:465-468), so
  `--debug=help,bogus` prints the word table and exits 0.

The diagnostic text now matches upstream verbatim - `Unknown --debug item:
"<tok>"` on stderr with RERR_SYNTAX = 1 (errcode.h:25) - instead of the
invented `invalid --debug flag '<tok>': use --debug=help for supported
flags`. The server-side `--info` decoder stays permissive, mirroring the
`!am_server` half of the guard at options.c:484.

Closes audit gaps I14 and G1.
@oferchen
oferchen force-pushed the fix/cli-unknown-output-word branch from 9b68278 to c6b2aff Compare September 3, 2026 05:47
@oferchen
oferchen merged commit f8f435b into master Sep 3, 2026
71 checks passed
@oferchen
oferchen deleted the fix/cli-unknown-output-word branch September 3, 2026 07:53
oferchen added a commit that referenced this pull request Sep 3, 2026
…dline is real (#7640)

* fix(ci): bound the interop smoke harness's readiness probe

`interop (Windows, best-effort)` hung for 25 minutes on PR #7637 and was
killed by the workflow, not by any assertion. The job log ends at

  PASS: baseline upstream local copy

and then emits nothing for 25 minutes until the cancel. Every setup step
had succeeded, including the MSYS2 install.

`wait_for_port` is where it sat, by elimination: the only thing between
that PASS and the next output is `start_daemon`, whose sole unbounded
operation is this readiness loop. The `deadline` is checked once per
iteration, so a probe that never returns defeats it entirely - the loop
never gets back to the test. Neither probe was individually bounded.

On Windows that is not a corner case, it is the normal path. The function's
own comment says `/dev/tcp` is not exposed by Cygwin's bash, so every
iteration falls through to the rsync probe; and a SYN to a port nothing is
listening on is DROPPED rather than refused when the runner's firewall is
in the way, so `connect()` blocks instead of returning ECONNREFUSED.

Measured against a real rsync 3.5.0, running the harness's exact invocation
against a blackhole address (127.0.0.1 always refuses, so the loopback probe
cannot reproduce the case that matters):

  WITH --contimeout=2:  rc=35, 2s
  WITHOUT:              still running at 41s, killed by `timeout`

`--contimeout` bounds the connect (options.c:831 declares it; socket.c:422
arms a SIGALRM around it) and `--timeout` bounds everything after, so each
iteration now costs at most a couple of seconds and the 15-second deadline
means what it says. When the daemon genuinely cannot bind - the documented
Windows condition this function exists to detect - the loop returns 1 in
~15s and the caller takes its existing SKIP path instead of hanging.

Behavioural contract re-measured on the patched function: dead port
rc=1 in 15s, live port rc=0 in 0s. `bash -n` clean; shellcheck finding
count unchanged at 1 (a pre-existing SC2086 on an untouched line).

⚠ This is NOT what reddened #7637. That job's 10 real failures are the
inherited master red; the Windows cell was CANCELLED, and the cancel came
from this hang. The hang is in code that runs the UPSTREAM rsync binary
before oc-rsync is invoked at all, so no oc-side diff can cause it.

* test(ci): backdate the 1 MiB delta seed so the mutation is always visible

D10/F10 seed a 1 MiB file, pull it, mutate it MID-FILE with `dd ...
conv=notrunc`, and pull again expecting a delta. The mutation changes
neither the file's size nor - at proto 29's whole-second mtime
granularity - necessarily its timestamp. Upstream's quick check reads
size and mtime and nothing else (generator.c quick_check_ok), so when
the seed write and the `dd` land in the same whole second the second
pull transfers nothing, the destination keeps the pre-mutation bytes,
and the cell's verify_diff fails on a file rsync was correct to skip.

That is a wall-clock coin flip, not a behaviour. MEASURED on two CI runs
of the same harness on 2026-09-03:

  master   setup finished 06:14:05.309, dd at ~06:14:06.3
           -> different seconds -> transfer -> F10.pull PASS (18/18)
  #7640    setup finished 06:04:18.005, dd at ~06:04:18.96
           -> SAME second -> `sent 107 / received 119` -> FAIL (17/18)

The two runs differ only in when they were scheduled; the failing job's
diff cannot reach this script.

Reproduced standalone against real rsync 3.5.0 with `--protocol=29`,
same fixture shape, arms differing ONLY by the backdate, 6 runs each:

  seed stamped now       pass=0 fail=6
  seed backdated         pass=6 fail=0

A fixed past timestamp makes the mutation unambiguously newer however
fast the harness runs. Nothing asserts on either seed's mtime - the
inode assertions are on f8/d8 - and `-a` carries the backdated stamp to
the destination on the first pull, so the quick check still has a
matched pair to compare.

Verified by running the committed setup scripts themselves, not a hand
copy: both payloads come out 1048576 bytes at 2001-09-09T01:46:40.
`bash -n` clean; shellcheck reports zero findings on both files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant