Recompute invalidated iterator in insertTargetAndModeArgs

[marcan]

Summary

Doing an .insert() can potentially invalidate iterators by reallocating the vector's storage. When all the stars align just right, this causes segfaults or glibc aborts. Fix this by recomputing the position before each insert.

Gentoo Linux bug (crashes while building Chromium): https://bugs.gentoo.org/650082

[lebedev.ri]

Tests?

[marcan]

I'm not sure how to test this. Getting the bug to manifest involves all of having a lot of command line arguments, getting unlucky with the way SmallVectorImpl decides to allocate, and getting unlucky with the glibc allocator deciding to move the data block on realloc (or running under valgrind).

The easiest reproducer I have is valgrind x86_64-pc-linux-gnu-clang++ $(seq 1 512) with a debug build, but I'm not sure that that guarantees an abort in every case. 511 arguments is not sufficient to trigger it on my system, but 512 is.

[sepavloff]

Good catch, thank you.

To avoid code duplication you could change InsertionPoint into an index and use begin() + InsertionPoint in calls to insert.

[sepavloff]

LGTM.

Thank you!

[marcan]

Note that I don't have commit access, so someone else will have to commit it for me :)

[raj.khem]

Can this be backported to release_60 branch too please ?

[sepavloff]

In D44607#1105962, @raj.khem wrote:

Can this be backported to release_60 branch too please ?

The patch is compact, clear and addresses stability issue. I think it is worth of backporting to release_60.

tools/driver/driver.cpp

 extern int cc1_main(ArrayRef<const char *> Argv, const char *Argv0,
                     void *MainAddr);
 extern int cc1as_main(ArrayRef<const char *> Argv, const char *Argv0,
                       void *MainAddr);
 
 static void insertTargetAndModeArgs(const ParsedClangName &NameParts,
                                     SmallVectorImpl<const char *> &ArgVector,
                                     std::set<std::string> &SavedStrings) {
+  if (NameParts.DriverMode) {
     // Put target and mode arguments at the start of argument list so that
     // arguments specified in command line could override them. Avoid putting
     // them at index 0, as an option like '-cc1' must remain the first.
     auto InsertionPoint = ArgVector.begin();
     if (InsertionPoint != ArgVector.end())
         ++InsertionPoint;
 
-  if (NameParts.DriverMode) {
     // Add the mode flag to the arguments.
     ArgVector.insert(InsertionPoint,
                      GetStableCStr(SavedStrings, NameParts.DriverMode));
   }
 
   if (NameParts.TargetIsValid) {
+    auto InsertionPoint = ArgVector.begin();
+    if (InsertionPoint != ArgVector.end())
+        ++InsertionPoint;
+
     const char *arr[] = {"-target", GetStableCStr(SavedStrings,
                                                   NameParts.TargetPrefix)};
     ArgVector.insert(InsertionPoint, std::begin(arr), std::end(arr));
   }
 }
 
 static void getCLEnvVarOptions(std::string &EnvValue, llvm::StringSaver &Saver,
                                SmallVectorImpl<const char *> &Opts) {