Modules: emit an error instead of a random crash (or a misleading error) due to use-after-free.

[manmanren]

Summary

With implicit modules, it is hard to debug issues that depend on state of the module cache before the clang invocation. The state of the module cache can be changed by other parallel clang invocations that share the module cache.

We build a module by spawning a child thread. Even when the parent thread already validated some modules and stored the FileEntries, the child thread can invalidate those modules, causing use-after-free for the parent thread.

Since this issue is time-sensitive, it is hard to reproduce with a release compiler. With an assert or an ASAN'ed compiler the chance of reproducing is even smaller. This patch tries to diagnose the use-after-free in the compiler, by passing up the list of invalidated modules from the child thread and emitting a hard error in the parent thread.

Another option is to pass down the list of validated modules from the parent thread, the child thread can emit a warning and not invalidate those modules.
I am open to other suggestions as well.

[manmanren]

In this testing case, the first clang invocation builds a system module X and a non-system module Y (X imports Y). At the second clang invocation, the parent thread validates the existing module X and module Y. Because X is a system module, we don't diagnose the differences in diagnostic options. When building module Z in the child thread, since Z is not a system module, we will spot the diagnostic differences and invalidate module Y. But the parent thread will continue accessing the deleted FileEntry for module Y.

[manmanren]

@ Ben,

We are hitting this issue when building large projects, but the reproducibility is quite low.

This proposed patch is currently a little too complicated. I am thinking about just fixing the testing case for now, and adding the check later when we start to share some data structures between threads (the idea of keeping MemoryBuffer consistent for threads within a single clang invocation).

For this testing case, we ignore the diagnostic options when a module is imported by a system module (see the code snippet below):

ModuleFile *TopImport = *ModuleMgr.rbegin();
while (!TopImport->ImportedBy.empty())
  TopImport = TopImport->ImportedBy[0];
if (TopImport->Kind != MK_ImplicitModule)
  return false;

StringRef ModuleName = TopImport->ModuleName;
assert(!ModuleName.empty() && "diagnostic options read before module name");

Module *M = PP.getHeaderSearchInfo().lookupModule(ModuleName);
assert(M && "missing module");

// FIXME: if the diagnostics are incompatible, save a DiagnosticOptions that
// contains the union of their flags.
return checkDiagnosticMappings(*Diags, ExistingDiags, M->IsSystem, Complain);

And here

// If we're reading the first module for this group, check its options
// are compatible with ours. For modules it imports, no further checking
// is required, because we checked them when we built it.
if (Listener && !ImportedBy) {

Does it mean that a system module should only import system modules? If a system module is allowed to import non-system modules, for a non-system module, we will validate diagnostic options differently depending on whether a system module or a non-system module imports it. This will cause a non-system module that was validated earlier to be invalidated by a child thread.

Thanks,
Manman

[benlangmuir]

Does it mean that a system module should only import system modules? If a system module is allowed to import non-system modules, for a non-system module, we will validate diagnostic options differently depending on whether a system module or a non-system module imports it. This will cause a non-system module that was validated earlier to be invalidated by a child thread.

It seems like we should validate the options the same way regardless of what the importer is, but I'm guessing this was done for a reason... What's the behaviour of a user-header imported by a system header (without modules)? If the user header warnings show up even without -Wsystem-headers, then we should be okay validating, right?

[manmanren]

In D25916#594844, @benlangmuir wrote:

Does it mean that a system module should only import system modules? If a system module is allowed to import non-system modules, for a non-system module, we will validate diagnostic options differently depending on whether a system module or a non-system module imports it. This will cause a non-system module that was validated earlier to be invalidated by a child thread.

It seems like we should validate the options the same way regardless of what the importer is, but I'm guessing this was done for a reason... What's the behaviour of a user-header imported by a system header (without modules)? If the user header warnings show up even without -Wsystem-headers, then we should be okay validating, right?

I tried a simple example:
cat test.mm
#include "a.h"
cat Inputs/System/a.h
#include "b.h"
cat Inputs/b.h
void double_declarator1(int *_Nonnull *); // expected-warning{{pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)}}

clang -cc1 -fsyntax-only -fblocks -I Inputs/ -isystem Inputs/System/ test.mm
--> has no warning
~/llvm_gmail/debug/bin/clang -cc1 -fsyntax-only -fblocks -I Inputs/ -isystem Inputs/System/ test.mm -Wsystem-headers
--> has one warning

Without modules, the user header warnings do not show up if it is included by a system header (without -Wsystem-headers). To exactly match this behavior, user modules need to be validated considering the importing context. We will need to change the code snippets I mentioned earlier to re-validate the options when the importing context changes.

Thanks,
Manman

[benlangmuir]

I'm inclined to suggest we break that behaviour with modules and treat a user module the same no matter where it was imported. @rsmith, what do you think?

include/clang/Basic/DiagnosticCommonKinds.td

 def err_module_lock_failure : Error<
   "could not acquire lock file for module '%0': %1">, DefaultFatal;
 def err_module_lock_timeout : Error<
   "timed out waiting to acquire lock file for module '%0'">, DefaultFatal;
 def err_module_cycle : Error<"cyclic dependency in module '%0': %1">, 
   DefaultFatal;
 def err_module_prebuilt : Error<
   "error in loading module '%0' from prebuilt module path">, DefaultFatal;
+def err_module_invalidated_by_child : Error<
+  "module '%0' is invalidated by a child thread">, DefaultFatal;
 def note_pragma_entered_here : Note<"#pragma entered here">;  
 def note_decl_hiding_tag_type : Note<
   "%1 %0 is hidden by a non-type declaration of %0 here">;
 def err_attribute_not_type_attr : Error<
   "%0 attribute cannot be applied to types">;
 def err_enum_template : Error<"enumeration cannot be a template">;
 
 }

include/clang/Serialization/ModuleManager.h

class ModuleManager {
 
   // \brief The roots of the dependency DAG of AST files. This is used
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector<ModuleFile *, 2> Roots;
 
   /// \brief All loaded modules, indexed by name.
   llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
 
+  /// \brief Modules that are invalidated by the child threads.
+  llvm::DenseMap<const FileEntry *, std::string> InvalidatedModules;
+
   /// \brief FileManager that handles translating between filenames and
   /// FileEntry *.
   FileManager &FileMgr;
 
   /// \brief Knows how to unwrap module containers.
   const PCHContainerReader &PCHContainerRdr;
 
   /// \brief A lookup of in-memory (virtual file) buffers
public:
   /// \returns True if a file exists but does not meet the size/
   /// modification time criteria, false if the file is either available and
   /// suitable, or is missing.
   bool lookupModuleFile(StringRef FileName,
                         off_t ExpectedSize,
                         time_t ExpectedModTime,
                         const FileEntry *&File);
 
+  /// In the context of a parent thread spawn a child thread to build a module,
+  /// the child thread's module manager checks to see if any successfully loaded
+  /// module in the parent thread is invalidated here. It returns the name of
+  /// the module if such module exists.
+  std::string checkForInvalidateEntries(const ModuleMap *modMapInParent,
+                                        ModuleManager *parentManager);
+
+  /// Pull invalidated entries from the child manager.
+  void mergeInvalidatedEntries(ModuleManager *childManager);
+
   /// \brief View the graphviz representation of the module graph.
   void viewGraph();
 };
 
 } } // end namespace clang::serialization
 
 #endif

lib/Frontend/CompilerInstance.cpp

static bool compileModuleImpl(CompilerInstance &ImportingInstance,
   llvm::CrashRecoveryContext CRC;
   CRC.RunSafelyOnThread([&]() { Instance.ExecuteAction(CreateModuleAction); },
                         ThreadStackSize);
 
   ImportingInstance.getDiagnostics().Report(ImportLoc,
                                             diag::remark_module_build_done)
     << Module->Name;
 
+  if (ImportingInstance.getModuleManager() && Instance.getModuleManager()) {
+    serialization::ModuleManager &ParentManager =
+        ImportingInstance.getModuleManager()->getModuleManager();
+    serialization::ModuleManager &ChildManager =
+        Instance.getModuleManager()->getModuleManager();
+
+    // Copy the invalidated modules to the parent thread.
+    ParentManager.mergeInvalidatedEntries(&ChildManager);
+    std::string Invalid = ChildManager.checkForInvalidateEntries(
+        &ModMap, &ParentManager);
+    if (!Invalid.empty())
+      Instance.getDiagnostics().Report(ImportLoc,
+          diag::err_module_invalidated_by_child) << Invalid;
+  }
+
   // Delete the temporary module map file.
   // FIXME: Even though we're executing under crash protection, it would still
   // be nice to do this with RemoveFileOnSignal when we can. However, that
   // doesn't make sense for all clients, so clean this up manually.
   Instance.clearOutputFiles(/*EraseFiles=*/true);
 
   // We've rebuilt a module. If we're allowed to generate or update the global
   // module index, record that fact in the importing compiler instance.

lib/Serialization/ModuleManager.cpp

ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   if (!ModuleEntry->isModule())
     PCHChain.push_back(ModuleEntry);
   if (!ImportedBy)
     Roots.push_back(ModuleEntry);
 
   return NewlyLoaded;
 }
 
+// Check to see if the parent thread is holding on to an entry that the child
+// thread invalidated.
+std::string ModuleManager::checkForInvalidateEntries(
+    const ModuleMap *modMapInParent, ModuleManager *parentManager) {
+  for (auto E : InvalidatedModules) {
+    if (parentManager->lookup(E.first))
+      return E.second;
+
+    if (modMapInParent) {
+      if (Module *mod = modMapInParent->findModule(E.second)) {
+        if (mod->getASTFile() == E.first)
+          return E.second;
+      }
+    }
+  }
+  return "";
+}
+
+void ModuleManager::mergeInvalidatedEntries(ModuleManager *child) {
+  for (auto E : child->InvalidatedModules)
+    InvalidatedModules[E.first] = E.second;
+}
+
 void ModuleManager::removeModules(
     ModuleIterator first, ModuleIterator last,
     llvm::SmallPtrSetImpl<ModuleFile *> &LoadedSuccessfully,
     ModuleMap *modMap) {
   if (first == last)
     return;
 
   // Explicitly clear VisitOrder since we might not notice it is stale.
if (modMap) {
       if (Module *mod = modMap->findModule(ModuleName)) {
         mod->setASTFile(nullptr);
       }
     }
 
     // Files that didn't make it through ReadASTCore successfully will be
     // rebuilt (or there was an error). Invalidate them so that we can load the
     // new files that will be renamed over the old ones.
-    if (LoadedSuccessfully.count(*victim) == 0)
+    if (LoadedSuccessfully.count(*victim) == 0) {
+      InvalidatedModules[(*victim)->File] = (*victim)->ModuleName;
       FileMgr.invalidateCache((*victim)->File);
+    }
 
     delete *victim;
   }
 
   // Remove the modules from the chain.
   Chain.erase(first, last);
 }
 

test/Modules/Inputs/system-out-of-date/X.h

+#import <Y.h>

test/Modules/Inputs/system-out-of-date/Y.h

+//empty

test/Modules/Inputs/system-out-of-date/Z.h

+#import <Y.h>

test/Modules/Inputs/system-out-of-date/module.map

+module X [system] {
+  header "X.h" // imports Y
+  export *
+}
+module Y {
+  header "Y.h"
+  export *
+}
+module Z {
+  header "Z.h" // imports Y
+  export *
+}

test/Modules/system-out-of-date-test.m

+// RUN: rm -rf %t
+// RUN: echo '@import X;' | \
+// RUN:   %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:     -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date \
+// RUN:     -fsyntax-only -x objective-c -
+
+// We have an version built with different diagnostic options.
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date -Wnon-modular-include-in-framework-module -Werror=non-modular-include-in-framework-module %s -fsyntax-only 2>&1 | FileCheck %s
+
+@import X;
+
+#import <Z.h>
+// CHECK: fatal error: module 'Y' is invalidated by its child
+// CHECK: fatal error: could not build module 'Z'