brbzull0 opened a new issue, #13648:
URL: https://github.com/apache/trafficserver/issues/13648
### 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 <utility>
#include <sysexits.h>
+// 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.
--
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]
