Page MenuHomePhabricator

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

Authored by manmanren on Oct 24 2016, 11:31 AM.

Details

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.

Diff Detail

Event Timeline

manmanren updated this revision to Diff 75616.Oct 24 2016, 11:31 AM
manmanren retitled this revision from to Modules: emit an error instead of a random crash (or a misleading error) due to use-after-free..
manmanren updated this object.
manmanren added reviewers: benlangmuir, rsmith.
manmanren added a subscriber: cfe-commits.

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.

@ 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 edited edge metadata.Nov 14 2016, 1:26 PM

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?

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

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?