[Support] Fix recursive response file expansion guard

[smeenai]

Summary

Response file expansion limits the amount of expansion to prevent
potential infinite recursion. However, the current logic assumes that
any argument beginning with @ is a response file, which is not true for
e.g. -Xlinker -rpath -Xlinker @executable_path/../lib on Darwin.
Having too many of these non-response file arguments beginning with @
prevents actual response files from being expanded. Instead, limit based
on the number of successful response file expansions, which should still
prevent infinite recursion but also avoid false positives.

[hans]

Looks good to me.

llvm/lib/Support/CommandLine.cpp

1041 ↗

(On Diff #194943)

How about getting rid of the negation in the if statement and instead doing:

if (ExpandResponseFile(..) {
  ++ExpandedRspFiles;
} else {
  // We couldn't read this file ...
}

llvm/unittests/Support/CommandLineTest.cpp

773 ↗

(On Diff #194943)

would push_back work instead? seems simpler

[smeenai]

llvm/lib/Support/CommandLine.cpp

1041 ↗

(On Diff #194943)

Sure.

llvm/unittests/Support/CommandLineTest.cpp

773 ↗

(On Diff #194943)

Yep.

[smeenai]

Review comments

llvm/trunk/lib/Support/CommandLine.cpp

static bool ExpandResponseFile(StringRef FName, StringSaver &Saver,
   return true;
 }
 
 /// Expand response files on a command line recursively using the given
 /// StringSaver and tokenization strategy.
 bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer,
                              SmallVectorImpl<const char *> &Argv,
                              bool MarkEOLs, bool RelativeNames) {
-  unsigned RspFiles = 0;
+  unsigned ExpandedRspFiles = 0;
   bool AllExpanded = true;
 
   // Don't cache Argv.size() because it can change.
   for (unsigned I = 0; I != Argv.size();) {
     const char *Arg = Argv[I];
     // Check if it is an EOL marker
     if (Arg == nullptr) {
       ++I;
       continue;
     }
     if (Arg[0] != '@') {
       ++I;
       continue;
     }
 
     // If we have too many response files, leave some unexpanded.  This avoids
     // crashing on self-referential response files.
-    if (RspFiles++ > 20)
+    if (ExpandedRspFiles > 20)
       return false;
 
     // Replace this response file argument with the tokenization of its
     // contents.  Nested response files are expanded in subsequent iterations.
     SmallVector<const char *, 0> ExpandedArgv;
-    if (!ExpandResponseFile(Arg + 1, Saver, Tokenizer, ExpandedArgv,
-                            MarkEOLs, RelativeNames)) {
+    if (ExpandResponseFile(Arg + 1, Saver, Tokenizer, ExpandedArgv, MarkEOLs,
+                           RelativeNames)) {
+      ++ExpandedRspFiles;
+    } else {
       // We couldn't read this file, so we leave it in the argument stream and
       // move on.
       AllExpanded = false;
       ++I;
       continue;
     }
     Argv.erase(Argv.begin() + I);
     Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());

llvm/trunk/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, RecursiveResponseFiles) {
 
   // Ensure some expansion took place.
   EXPECT_GT(Argv.size(), 2U);
   EXPECT_STREQ(Argv[0], "test/test");
   for (size_t i = 1; i < Argv.size(); ++i)
     EXPECT_STREQ(Argv[i], ResponseFileRef.c_str());
 }
 
+TEST(CommandLineTest, ResponseFilesAtArguments) {
+  SmallString<128> TestDir;
+  std::error_code EC = sys::fs::createUniqueDirectory("unittest", TestDir);
+  EXPECT_TRUE(!EC);
+
+  SmallString<128> ResponseFilePath;
+  sys::path::append(ResponseFilePath, TestDir, "test.rsp");
+
+  std::ofstream ResponseFile(ResponseFilePath.c_str());
+  EXPECT_TRUE(ResponseFile.is_open());
+  ResponseFile << "-foo" << "\n";
+  ResponseFile << "-bar" << "\n";
+  ResponseFile.close();
+
+  // Ensure we expand rsp files after lots of non-rsp arguments starting with @.
+  constexpr size_t NON_RSP_AT_ARGS = 64;
+  llvm::SmallVector<const char *, 4> Argv = {"test/test"};
+  Argv.append(NON_RSP_AT_ARGS, "@non_rsp_at_arg");
+  std::string ResponseFileRef = std::string("@") + ResponseFilePath.c_str();
+  Argv.push_back(ResponseFileRef.c_str());
+
+  llvm::BumpPtrAllocator A;
+  llvm::StringSaver Saver(A);
+  bool Res = llvm::cl::ExpandResponseFiles(
+      Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, false);
+  EXPECT_FALSE(Res);
+
+  // ASSERT instead of EXPECT to prevent potential out-of-bounds access.
+  ASSERT_EQ(Argv.size(), 1 + NON_RSP_AT_ARGS + 2);
+  size_t i = 0;
+  EXPECT_STREQ(Argv[i++], "test/test");
+  for (; i < 1 + NON_RSP_AT_ARGS; ++i)
+    EXPECT_STREQ(Argv[i], "@non_rsp_at_arg");
+  EXPECT_STREQ(Argv[i++], "-foo");
+  EXPECT_STREQ(Argv[i++], "-bar");
+}
+
 TEST(CommandLineTest, SetDefautValue) {
   cl::ResetCommandLineParser();
 
   StackOption<std::string> Opt1("opt1", cl::init("true"));
   StackOption<bool> Opt2("opt2", cl::init(true));
   cl::alias Alias("alias", llvm::cl::aliasopt(Opt2));
   StackOption<int> Opt3("opt3", cl::init(3));