[sanitizer] Don't treat colon before slash as a flag separator

[rnk]

Summary

Users often have path escaping bugs in their ASan options. They do
things like this
ASAN_OPTIONS=foo=1:bar=0:symbolizer_path=%s
... where %s is an absolute path. They may have project rules that
require paths to not contain spaces, so they get away with this on
Linux. Then they come to Windows, and the substituted options look like:
symbolizer_path=C:/path/to/llvm-symbolizer.exe
This doesn't work because we have chosen colon to be an option
separator.

Many users have hit this, it affects the Go race detector, and it
affects Chromium Windows ASan usage. Try to make their life a little
easier by not treating colon before slash as a flag separator.

Fixes https://github.com/google/sanitizers/issues/884

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cc

77

All these variables are mapped to the same C++ members. So not sure if it's worth it but what if we generalize to:
":" is separateor IFF it's followed by "[a-zA_Z0-9_]+="

[rnk]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cc

77

I think that'll be too magic. Consider this case where the user just forgets to say "flag2" before "=":

flag1=val1:=val2:flag3=val3

We'll parse that as flag1="val1:=val2" and not give any errors. The same could be said with this if they add a slash after a colon that's supposed to be a separator, but hopefully it's not an issue in practice.

[vitalybuka]

works for me any way

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cc

77

Both approaches too magic but one can avoid OS specifics.
To get error there we can just use "[a-zA_Z0-9_]*=" and it will hit same error :-)

BTW. The patch does not cover "c:win\path" which is impractical but valid path.

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

public:
   void ParseString(const char *s);
   bool ParseFile(const char *path, bool ignore_missing);
   void PrintFlagDescriptions();
 
   static LowLevelAllocator Alloc;
 
  private:
   void fatal_error(const char *err);
-  bool is_space(char c);
+  bool is_space();
   void skip_whitespace();
   void parse_flags();
   void parse_flag();
   bool run_handler(const char *name, const char *value);
   char *ll_strndup(const char *s, uptr n);
 };
 
 template <typename T>

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cc

for (int i = 0; i < n_flags_; ++i)
     Printf("\t%s\n\t\t- %s\n", flags_[i].name, flags_[i].desc);
 }
 
 void FlagParser::fatal_error(const char *err) {
   Printf("ERROR: %s\n", err);
   Die();
 }
 
-bool FlagParser::is_space(char c) {
-  return c == ' ' || c == ',' || c == ':' || c == '\n' || c == '\t' ||
-         c == '\r';
+bool FlagParser::is_space() {
+  char c = buf_[pos_];
+  if (c == ' ' || c == ',' || c == '\n' || c == '\t' || c == '\r')
+    return true;
+
+  // Treat ':' as an option separator unless it is immediately followed by a
+  // path separator. This avoids common portability issues when an unquoted
+  // Windows path is used, as in 'symbolizer_path=C:/foo/llvm-symbolizer.exe'.
+  // Since no option names should start with / or \, it should be unambiguous.
+  if (c == ':')
+    return buf_[pos_ + 1] != '\\' && buf_[pos_ + 1] != '/';
+  return false;
 }
 
 void FlagParser::skip_whitespace() {
-  while (is_space(buf_[pos_])) ++pos_;
+  while (is_space()) ++pos_;
 }
 
 void FlagParser::parse_flag() {
   uptr name_start = pos_;
-  while (buf_[pos_] != 0 && buf_[pos_] != '=' && !is_space(buf_[pos_])) ++pos_;
+  while (buf_[pos_] != 0 && buf_[pos_] != '=' && !is_space()) ++pos_;
   if (buf_[pos_] != '=') fatal_error("expected '='");
   char *name = ll_strndup(buf_ + name_start, pos_ - name_start);
 
   uptr value_start = ++pos_;
   char *value;
   if (buf_[pos_] == '\'' || buf_[pos_] == '"') {
     char quote = buf_[pos_++];
     while (buf_[pos_] != 0 && buf_[pos_] != quote) ++pos_;
     if (buf_[pos_] == 0) fatal_error("unterminated string");
     value = ll_strndup(buf_ + value_start + 1, pos_ - value_start - 1);
     ++pos_; // consume the closing quote
   } else {
-    while (buf_[pos_] != 0 && !is_space(buf_[pos_])) ++pos_;
-    if (buf_[pos_] != 0 && !is_space(buf_[pos_]))
+    while (buf_[pos_] != 0 && !is_space()) ++pos_;
+    if (buf_[pos_] != 0 && !is_space())
       fatal_error("expected separator or eol");
     value = ll_strndup(buf_ + value_start, pos_ - value_start);
   }
 
   bool res = run_handler(name, value);
   if (!res) fatal_error("Flag parsing failed.");
 }
 

compiler-rt/lib/sanitizer_common/tests/sanitizer_flags_test.cc

TEST(SanitizerCommon, CommonFlags) {
 
   parser.ParseString("symbolize=1:heap_profile=false log_path='path/two'");
   EXPECT_TRUE(cf.symbolize);
   EXPECT_TRUE(cf.coverage);
   EXPECT_FALSE(cf.heap_profile);
   EXPECT_STREQ("path/two", cf.log_path);
 }
 
+TEST(SanitizerCommon, WindowsPath) {
+  FlagParser parser;
+  const char *p1 = "";
+  const char *p2 = "";
+  RegisterFlag(&parser, "path1", kFlagDesc, &p1);
+  RegisterFlag(&parser, "path2", kFlagDesc, &p2);
+  parser.ParseString("path1=C:/win/path.exe:path2=C:\\win\\path.exe");
+  ReportUnrecognizedFlags();
+
+  EXPECT_STREQ(p1, "C:/win/path.exe");
+  EXPECT_STREQ(p2, "C:\\win\\path.exe");
+}
+
 }  // namespace __sanitizer