Page MenuHomePhabricator

[Support] Fix recursive response file expansion guard
ClosedPublic

Authored by smeenai on Apr 12 2019, 12:54 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai created this revision.Apr 12 2019, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 12:54 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
hans accepted this revision.Apr 15 2019, 1:45 AM

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

This revision is now accepted and ready to land.Apr 15 2019, 1:45 AM
smeenai marked 2 inline comments as done.Apr 15 2019, 2:23 PM
smeenai added inline comments.
llvm/lib/Support/CommandLine.cpp
1041 ↗(On Diff #194943)

Sure.

llvm/unittests/Support/CommandLineTest.cpp
773 ↗(On Diff #194943)

Yep.

smeenai updated this revision to Diff 195246.Apr 15 2019, 2:24 PM

Review comments

This revision was automatically updated to reflect the committed changes.