brbzull0 opened a new issue, #13746:
URL: https://github.com/apache/trafficserver/issues/13746
### Impact
`ArgParser` matches a command's options anywhere in the rest of the command
line, and the `--` escape added in #13570 does not reach that scan. For the
top-level command that is the whole argv, so a global option is recognised even
when it appears as a subcommand's value, and even after `--`, which #13570 made
the way to pass a value that starts with `-`. The same scan lets a global
option silently shadow a subcommand option that has the same short name.
What an operator sees, with `traffic_ctl` (it applies equally to
`traffic_layout` and `traffic_cache_tool`):
- A value spelled like a global option (`-f`, `-w`, `--run-root`,
`--read-timeout-ms`, ...) is taken as that option: usage error, exit 64, and
`--` does not help.
- A value of `-h` or `-V` prints help or the version and exits **0**. The
command never runs, but a script sees success.
- `traffic_ctl config reload -w <seconds>` sets the global `--watch`, which
nothing in the tree reads, instead of the documented `config reload
--initial-wait, -w`.
It takes a value that happens to match a global option name, so real-world
impact is low. There is no workaround for such values. For the `-w` clash,
`--initial-wait` works.
```
Version: master @ 738cd651b3
top-level scan unchanged since d9b06a12acf (2018); the "--" escape
added in d0fb283406 (#13570)
only covers collecting an option's own values. The -w clash dates
from 5bab268cb4 (#12892).
Platform: Darwin 25.6.0, Apple clang 21.0.0
Config: n/a
```
### Proof
`Command::parse()` runs `append_option_data()` on the command's own argv
before recursing into subcommands, and `append_option_data()` walks every
remaining token with no check for `--`:
[`src/tscore/ArgParser.cc#L737-L740`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/tscore/ArgParser.cc#L737-L740),
called at
[`#L861`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/tscore/ArgParser.cc#L861):
```cpp
ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int
index)
{
std::map<std::string, unsigned> check_map;
for (unsigned i = index; i < args.size(); i++) {
```
`--` is only honoured inside `handle_args()`, once an option has been
matched and is collecting its values
([`#L564`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/tscore/ArgParser.cc#L564),
[`#L591`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/tscore/ArgParser.cc#L591),
[`#L620`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/tscore/ArgParser.cc#L620)).
A `-h` anywhere prints help with exit code 0
([`#L768-L778`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/tscore/ArgParser.cc#L768-L778)).
`traffic_ctl` registers `-w` twice, globally for `--watch`
([`src/traffic_ctl/traffic_ctl.cc#L95`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/traffic_ctl/traffic_ctl.cc#L95))
and on `config reload` for `--initial-wait`
([`#L172`](https://github.com/apache/trafficserver/blob/738cd651b389bd32b4d0887f99d0a1960abc12b7/src/traffic_ctl/traffic_ctl.cc#L172)).
The `watch` key is never read.
Reproduction, with no traffic_server running; the RPC connection error shows
parsing got through:
```
$ traffic_ctl config set proxy.config.diags.debug.tags -- -m # -m is
not a global option: passed through
Error found:
RPC Node Error: connect(attempts=1/5): Couldn't open connection with
.../jsonrpc20.sock. Last error: No such file or directory(2)
[exit 2]
$ traffic_ctl config set proxy.config.diags.debug.tags -- -f # -f is:
taken as --format
Error: 1 argument(s) expected by format
[exit 64]
$ traffic_ctl config set proxy.config.diags.debug.tags -- -f json
Error: 2 argument(s) expected by set
[exit 64]
$ traffic_ctl config set proxy.config.diags.debug.tags -h # help,
exit 0, nothing set
Usage: traffic_ctl [OPTIONS] CMD [ARGS ...]
[exit 0]
$ traffic_ctl config reload -w #
documented as --initial-wait
Error: 1 argument(s) expected by watch
[exit 64]
```
JSON output form (the parse fails before any output is formatted, so it
matches):
```
$ traffic_ctl --format json config set proxy.config.diags.debug.tags -- -f
json
Error: 2 argument(s) expected by set
[exit 64]
```
Expected: after `--`, `-f` and `-h` are the value of
`proxy.config.diags.debug.tags`, as `-m` already is.
`--` itself works. It is the global scan that ignores it: `-m` is not a
global option and passes, `-f` and `-h` are and do not.
### Proposed change
`--` is scoped to one option today (#13570):
- For a fixed or at-most-one option it escapes the next value, and options
written after that value still parse: `server debug enable --tags -- -a`,
`config get -c -- -weird.yaml proxy.config.x`.
- For a variable-arity option it ends that option's own recognition: `config
reload -D -- -m`.
The parent levels just need to leave alone the token that `--` escapes. Let
`append_option_data()` skip `--` and the token after it, and leave both in
place for the option or command further down that owns them:
```diff
--- a/src/tscore/ArgParser.cc
+++ b/src/tscore/ArgParser.cc
@@ -738,6 +738,11 @@ ArgParser::Command::append_option_data(Arguments &ret,
AP_StrVec &args, int inde
{
std::map<std::string, unsigned> check_map;
for (unsigned i = index; i < args.size(); i++) {
+ // "--" escapes the token after it, which belongs to an option or
command further down: leave both for it.
+ if (args[i] == "--") {
+ ++i;
+ continue;
+ }
// find matches of the arg
if (args[i][0] == '-' && args[i][1] == '-' && args[i].find('=') !=
std::string::npos) {
// deal with --args=
```
I tried it on the tree above. The escaped tokens are handed on:
- `config set <rec> -- -f` and `config set <rec> -- -h` now reach the RPC.
Every existing pattern keeps its behaviour, with exit codes and results
identical to master:
- `server debug enable --tags -- -a -f json`
- `config get --cold -- -weird.yaml <rec> -f json`
- `config reload -D -- -m -f json`
- `config set <rec> -f json -- -x`
The tscore unit tests pass (115/115). So do the AuTests that use `--`:
`traffic_ctl_server_debug`, `traffic_ctl_cold_config`,
`config_reload_directive_cli`, `traffic_ctl_config_output`,
`traffic_ctl_config_reload`.
I rejected the obvious alternative, stopping option recognition at `--`
altogether (`break`). It turns `--` into an end-of-line marker at every level
and breaks a global option written after an escaped value: `server debug enable
--tags -- -a -f json` fails with `Unknown command, option or args: '-f' 'json'`.
What skip-one does not cover:
- After a variable-arity option's `--`, only the first value is protected
from the global scan. In `config reload -D -- -a -f json`, `-f json` is still
taken as the global `--format`, the same as today. That contradicts the
`traffic_ctl.en.rst` note that options written after `--` are swallowed by
`-D`. `--directive=-value` stays the way to pass several dash values.
- The `-w` clash. The global `--watch` is undocumented and unused, so
dropping it (or its short form) gives `config reload -w` back to
`--initial-wait`.
Still to do:
- A unit test in `src/tscore/unit_tests/test_ArgParser.cc`: a subcommand
value after `--` that spells a global option is left to the subcommand, and a
global option after the escaped value is still recognised.
- Optionally, reject at registration a subcommand option whose long or short
name is already taken by an ancestor, so the next clash fails at startup
instead of being silently shadowed.
Related:
- #13597: the other direction, where a parent option takes a subcommand's
option as its value. Same root cause: every level scans the rest of argv.
- #13570: added the `--` escape for option values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
