apache / apache/trafficserver

ArgParser: parser state has external linkage, leaking symbols and blocking tests

Open
#13,648 0 comments 0 reactions 1 assignee Claimed by @brbzull0 View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.