fix(cli): reject a flag as another flag's value, and unknown options - #603
Open
singaraiona wants to merge 1 commit into
Open
singaraiona wants to merge 1 commit into
singaraiona wants to merge 1 commit into
Conversation
Every value-taking startup flag was gated on `i + 1 < argc` and then consumed argv[++i] blindly, so one flag silently ate the next. The shape that was reported is the worst one for a service: rayforce -Q -p 5099 svc.rfl `-Q` took `-p` as its value, the port was never bound, and nothing on stdout or stderr said so. Under a supervisor the process starts, the script runs, the unit looks healthy, and every client gets connection refused. `-c` and `-t` had the identical hole. Two neighbouring cases came out of the same code. A trailing flag with no value fell through to the positional-file arm and reported the misleading `cannot open '-Q'`. And that arm took ANY unrecognized token as the script name, which the real positional then overwrote, so a typo'd option was not merely ignored — it was silently swallowed: `rayforce -x svc.rfl` ran the script as if `-x` had never been typed. Every value-taking flag now goes through flag_value(), which refuses a missing value and a value that is EXACTLY one of this program's flag tokens (including the `--` app-args terminator), with a diagnostic and exit 2 before the script is loaded. A value that merely STARTS with '-' stays legal: a password or a negative number is a legitimate value, and refusing those would break working command lines for no gain. An unrecognized `-`-prefixed token is now an error too; a lone `-` is still a positional, and tokens after `--` belong to the app and are never validated. `-Q, --querylog N` was a real flag, referenced twice in the docs, that --help never listed beside its sibling `-t, --timeit N` — added, along with `[-Q 0|1]` in the synopsis. The suggestion to fail when `-p` was requested but nothing bound is already in (#473, listen_fatal.rfl), and cannot catch this: the parser never saw the eaten `-p`, so there was no request to check against. The guard on the value is what closes the class. Also: the two pre-existing `-p` validation bail-outs now release the runtime like every other error path, since these paths are exercised under ASan by the new test. Closes #600
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
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.
What & why
-Qis a real startup flag, referenced twice in the docs, that--helpneverlisted. It takes a mandatory value, and given without one it swallowed the
following argument:
-Qtook-pas its value, so the port was never bound. Under a supervisorthe process starts, the script runs, the unit looks healthy, and every client
gets connection refused. That is the reported shape (#600), but the cause was
systemic: every value-taking flag was gated on
i + 1 < argcand then consumedargv[++i]blindly, so-cand-thad the identical hole.Two neighbouring cases fell out of the same code while reproducing it:
-Q -p 5099 s.rflscript ran, no listener, exit 0error: -Q expects a value, got flag '-p', exit 2-c -p 5097 s.rfl-t -p 5096 s.rfls.rfl -Qerror: cannot open '-Q'error: -Q expects a value-x s.rflscript ran— flag silently ignorederror: unknown option '-x'The last one is the same root cause: the positional-file arm took any
unrecognized token as the script name, which the real positional then
overwrote, so a typo'd option was not merely ignored but silently swallowed.
The change
-Q, --querylog Nadded to--help, beside its sibling-t, --timeit N,and
[-Q 0|1]to the synopsis.flag_value(), which every value-taking flag now goes through. It refusesa missing value, and a value that is exactly one of this program's flag
tokens (including the
--app-args terminator) — diagnostic and exit 2before the script is loaded. A value that merely starts with
-stayslegal: a password or a negative number is a legitimate value, and refusing
those would break working command lines for no gain.
--prefixed options rejected. A lone-is still a positional,and tokens after
--belong to the app and are never validated.-pvalidation bail-outs now release the runtime likeevery other error path, since the new test exercises them under ASan.
The issue also suggested failing when
-pwas requested but nothing bound.That already exists (#473,
listen_fatal.rfl) and cannot catch this — theparser never saw the eaten
-p, so there was no request to check against.Guarding the value is what closes the class.
No docs change:
namespaces/sys.mdalready documents-Qwith its value, andno docs page enumerates the CLI flags.
Tests
test/rfl/system/cli_flag_values.rfl, 11 cases covering each row of the tableabove plus the values that must stay legal (
-u -s3cret,-t -1),--passthrough, and
--helplisting-Q. It fails against the pre-fix binary(exit 99 = "script ran", the reported bug) and passes against the fix.
Closes #600
Checklist
dev(notmaster)feat:/fix:/perf:/docs:/ …)makebuilds cleanly (no new warnings)make testpasses; tests added/updated for behaviour changes