ArgParser: parser state has external linkage, leaking symbols and blocking tests
- Dominant language
- C++
- Stars
- 2k
- Forks
- 874
- Avg merge
- 6d 15h
- Merged PRs (30d)
- 46
Description
### Impact
`ArgParser` keeps its parser-wide state in five file-scope variables that have
**external linkage** — they are not `static` and not in an anonymous namespace — so
they are exported as plain, unmangled symbols from `libtscore`:
```
$ nm -g build/src/tscore/libtscore.a | grep -E ' [SDB] _(description|default_command|global_usage|parser_program_name|usage_return_code)$'
000000000010e880 S _default_command
000000000010e850 S _description
000000000010e838 S _global_usage
000000000010e868 S _parser_program_name
000000000001bcbc D _usage_return_code
```
Two consequences:
1. **Symbol leak.** `libtscore` exports `_description` and `_global_usage`, names
generic enough to collide with any other translation unit or plugin that defines a
global with the same name. Nothing in-tree collides today, so there is no known
production impact — but the surface is real and it is trivially avoidable.
2. **`set_default()` leaks state into the rest of the process**, because
`default_command` is never cleared. This is what makes the `ret = Arguments{}` guard
added in #13570 untestable: a unit test that calls `set_default()` changes
`default_command` for every later test in the same binary, so the default-command
retry path cannot be exercised in isolation.
No operator-visible misbehaviour, and no workaround needed — this is a latent
maintainability and testability defect, filed as follow-up to review feedback on
#13570.
```
Version: master @ d0fb2834069, present since e677cebdf6 (2018-10-09, shipped in 10.0.0)
Platform: any
Config: n/a
```
### Proof
All five declarations are at file scope with no `static` and no enclosing anonymous
namespace —
[`src/tscore/ArgParser.cc#L34-L41`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/tscore/ArgParser.cc#L34-L41):
```cpp
std::string global_usage;
std::string description;
std::string parser_program_name;
std::string default_command;
// by default return EX_USAGE(64) when usage is called.
// if -h or --help is called specifically, return 0
int usage_return_code = EX_USAGE;
```
`default_command` is written in two places and cleared in none —
[`ArgParser.cc#L163`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/tscore/ArgParser.cc#L163)
(`set_default_command`) and
[`ArgParser.cc#L825`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/tscore/ArgParser.cc#L825)
(`Command::set_default`):
```cpp
ArgParser::Command &
ArgParser::Command::set_default()
{
default_command = _name;
return *this;
}
```
The retry path it gates, at
[`ArgParser.cc#L195-L202`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/tscore/ArgParser.cc#L195-L202):
```cpp
if (!_top_level_command.parse(ret, args)) {
// deal with default command
if (!default_command.empty()) {
args = _argv;
args.insert(args.begin() + 1, default_command);
// The pass that failed may have collected options before it gave up. Those values would
// now accumulate on top of the ones the retry collects rather than be replaced.
ret = Arguments{};
_top_level_command.parse(ret, args);
}
};
```
Every use of the five variables is inside `ArgParser.cc`; the only other mention in the
tree is a comment in the header, so nothing depends on the external linkage:
```bash
$ grep -rn '\bdefault_command\b\|\bparser_program_name\b\|\busage_return_code\b\|\bglobal_usage\b' \
src/ include/ plugins/ --include='*.cc' --include='*.h' | grep -v src/tscore/ArgParser.cc
include/tscore/ArgParser.h:312: // Add the usage to global_usage for help_message(). Something like: traffic_blabla [--SWITCH [ARG]]
```
Found by inspection plus `nm`; not reproduced as a runtime failure, because the
symbol collision needs a second definition that does not currently exist in-tree.
### Proposed change
Give the variables internal linkage and add a test-only reset so the retry path
becomes testable.
```diff
--- a/src/tscore/ArgParser.cc
+++ b/src/tscore/ArgParser.cc
@@ -31,6 +31,10 @@
#include
#include
+// Internal linkage: these are parser-wide state for this translation unit only. At file
+// scope without static they are exported from libtscore under names generic enough
+// ("description", "global_usage") to collide with any other global of the same name.
+namespace
+{
std::string global_usage;
std::string description;
std::string parser_program_name;
@@ -39,6 +43,7 @@
// by default return EX_USAGE(64) when usage is called.
// if -h or --help is called specifically, return 0
int usage_return_code = EX_USAGE;
+} // namespace
namespace ts
{
@@ -45,6 +50,17 @@ namespace ts
bool ArgParser::_test_mode = false;
+void
+ArgParser::reset_global_state()
+{
+ global_usage.clear();
+ description.clear();
+ parser_program_name.clear();
+ default_command.clear();
+ usage_return_code = EX_USAGE;
+}
+
```
```diff
--- a/include/tscore/ArgParser.h
+++ b/include/tscore/ArgParser.h
@@
+ /// Clear the parser-wide state held at file scope in ArgParser.cc.
+ ///
+ /// Test-only. Production code builds one ArgParser per process and never reuses the
+ /// state, but a unit test binary runs many parsers in sequence, and set_default()
+ /// would otherwise leak default_command into every later test.
+ static void reset_global_state();
```
Making them members of `ArgParser` would be the tidier fix, but they are read from
`ArgParser::Command` methods as well, so that turns into a wider refactor with no extra
benefit for the symbol problem. Internal linkage plus a reset hook fixes both the export
and the testability in a change that is easy to review.
No API, config-key or metric-name change, so no release note or backport is implied.
Still to do: a unit test for the default-command retry, using `reset_global_state()`,
asserting that options collected by the failed first pass do not accumulate on top of
the retry's values.
### Remaining review notes from #13570
Recorded here so they are not lost. All were raised as explicitly non-blocking.
- **Empty-value policy is split across two layers.** The parser rejects `-c ""` for
at-most-one arity, while `-D ""` and `-d ""` parse fine and are caught later by
`has_empty_value()` at
[`CtrlCommands.cc#L60`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/traffic_ctl/CtrlCommands.cc#L60).
Having `handle_args` reject an empty value token uniformly would centralise it, but it
would affect other consumers, so the split may be deliberate.
- **Dead branch.** `has_empty_value(dir_args)` returns early if *any* directive value is
empty
([`CtrlCommands.cc#L621-L625`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/traffic_ctl/CtrlCommands.cc#L621-L625)),
so the `if (dir.empty()) { continue; }` in the loop below at
[`#L628-L630`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/traffic_ctl/CtrlCommands.cc#L628-L630)
is unreachable and can go.
- **Document the `--` rule rather than change it.** It is one rule: option recognition
goes off for the remainder of *that option's* value collection. It only reads as three
behaviours because the arities differ — `-D` collects to the end of the line, `-c`
escapes exactly one token, a fixed arity lasts until its values are filled. Stating the
general rule in `doc/developer-guide/internal-libraries/ArgParser.en.rst` would stop the
three arities from looking like three separate features.
- **Comment worth adding to `is_registered_option()`.** It consults only the current
command's options
([`ArgParser.cc#L527`](https://github.com/apache/trafficserver/blob/d0fb283406931e502ec0737e5af8ddf0dcc71c57/src/tscore/ArgParser.cc#L527)),
which is correct because `append_option_data()` sweeps the remaining vector for the
parent's options before recursing into subcommands. It does mean a variable-arity option
declared on a command that *has* subcommands would still swallow the subcommand name.
Every variable-arity option in `traffic_ctl` today is on a leaf command (`reload`,
`invoke`), so nothing reaches it — but the next person to add one will want to know.
Contributor guide
Assessment
This issue has not been assessed yet.