[PATCH] D60631: [Support] Fix recursive response file expansion guard

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai created this revision.
smeenai added reviewers: rnk, hans.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60631

Files:
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp


Index: llvm/unittests/Support/CommandLineTest.cpp
===================================================================
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -751,6 +751,43 @@
     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.emplace_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();
 
Index: llvm/lib/Support/CommandLine.cpp
===================================================================
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1007,7 +1007,7 @@
 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.
@@ -1025,7 +1025,7 @@
 
     // 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
@@ -1038,6 +1038,8 @@
       AllExpanded = false;
       ++I;
       continue;
+    } else {
+      ++ExpandedRspFiles;
     }
     Argv.erase(Argv.begin() + I);
     Argv.insert(Argv.begin() + I, ExpandedArgv.begin(), ExpandedArgv.end());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60631.194943.patch
Type: text/x-patch
Size: 2862 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190412/f7c94f41/attachment.bin>