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

[brbzull0]

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 Argparser variable arg option parsing fix/improvement. #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:

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
(set_default_command) and
ArgParser.cc#L825
(Command::set_default):

ArgParser::Command &
ArgParser::Command::set_default()
{
  default_command = _name;
  return *this;
}

The retry path it gates, at
ArgParser.cc#L195-L202:

  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:

$ 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.

--- 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;
+}
+

--- 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.
  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),
  so the if (dir.empty()) { continue; } in the loop below at
  #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),
  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.