[AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[martong]

Summary

The SwitchStmt::FirstCase member is not initialized when the AST is
built by the ASTStmtReader. See the below code of
ASTStmtReader::VisitSwitchStmt in the case where the for loop does not
have any iterations:

  // ... more code ...
  SwitchCase *PrevSC = nullptr;
  for (auto E = Record.size(); Record.getIdx() != E; ) {
    SwitchCase *SC = Record.getSwitchCaseWithID(Record.readInt());
    if (PrevSC)
      PrevSC->setNextSwitchCase(SC);
    else
      S->setSwitchCaseList(SC); // Sets FirstCase !!!

    PrevSC = SC;
  }
} // return

Later, in ASTNodeImporter::VisitSwitchStmt,
we have a condition that depends on this uninited value:

for (SwitchCase *SC = S->getSwitchCaseList(); SC != nullptr;
     SC = SC->getNextSwitchCase()) {
     // ... more code ...
}

This is clearly an UB. This causes non-deterministic crashes when
ClangSA analyzes some code with CTU. See the below report by valgrind
(the whole valgrind output is attached):

==31019== Conditional jump or move depends on uninitialised value(s)
==31019==    at 0x12ED1983: clang::ASTNodeImporter::VisitSwitchStmt(clang::SwitchStmt*) (ASTImporter.cpp:6195)
==31019==    by 0x12F1D509: clang::StmtVisitorBase<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Stmt*>>::Visit(clang::Stmt*) (StmtNodes.inc:591)
==31019==    by 0x12EE4FDF: clang::ASTImporter::Import(clang::Stmt*) (ASTImporter.cpp:8484)
==31019==    by 0x12F09498: llvm::Expected<clang::Stmt*> clang::ASTNodeImporter::import<clang::Stmt>(clang::Stmt*) (ASTImporter.cpp:164)
==31019==    by 0x12F3A1F5: llvm::Error clang::ASTNodeImporter::ImportArrayChecked<clang::Stmt**, clang::Stmt**>(clang::Stmt**, clang::Stmt**, clang::Stmt**) (ASTImporter.cpp:653)
==31019==    by 0x12F13152: llvm::Error clang::ASTNodeImporter::ImportContainerChecked<llvm::iterator_range<clang::Stmt**>, llvm::SmallVector<clang::Stmt*, 8u> >(llvm::iterator_range<clang::Stmt**> const&, llvm::SmallVector<clang::Stmt*, 8u>&) (ASTImporter.cpp:669)
==31019==    by 0x12ED099F: clang::ASTNodeImporter::VisitCompoundStmt(clang::CompoundStmt*) (ASTImporter.cpp:6077)
==31019==    by 0x12F1CC2D: clang::StmtVisitorBase<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Stmt*>>::Visit(clang::Stmt*) (StmtNodes.inc:73)
==31019==    by 0x12EE4FDF: clang::ASTImporter::Import(clang::Stmt*) (ASTImporter.cpp:8484)
==31019==    by 0x12F09498: llvm::Expected<clang::Stmt*> clang::ASTNodeImporter::import<clang::Stmt>(clang::Stmt*) (ASTImporter.cpp:164)
==31019==    by 0x12F13275: clang::Stmt* clang::ASTNodeImporter::importChecked<clang::Stmt*>(llvm::Error&, clang::Stmt* const&) (ASTImporter.cpp:197)
==31019==    by 0x12ED0CE6: clang::ASTNodeImporter::VisitCaseStmt(clang::CaseStmt*) (ASTImporter.cpp:6098)

[martong]

valgrind-output.txt59 KBDownload

[aaron.ballman]

This looks reasonable to me (good catch!), but is there a way for us to add a regression test for it?

[steakhal]

In D97849#2600201, @aaron.ballman wrote:

This looks reasonable to me (good catch!), but is there a way for us to add a regression test for it?

I'm not sure if it's possible to write a test for deterministically demonstrating the bug - which is a non-deterministic crash.
So, even if we would have a test case, that would not catch the regression deterministically.
We could include the minimal reproducer for CTU analysis - the way we observed and tracked down this crash.

AFAIK, it did not reproduce with memory sanitizers.

[teemperor]

We can (and do) run LIT tests under valgrind, so if you have a test that triggers this valgrind error then that seems like the way to go. I don't think the test has to deterministically fail outside valgrind to be a valid test.

[martong]

In D97849#2600220, @steakhal wrote:

In D97849#2600201, @aaron.ballman wrote:

This looks reasonable to me (good catch!), but is there a way for us to add a regression test for it?

I'm not sure if it's possible to write a test for deterministically demonstrating the bug - which is a non-deterministic crash.
So, even if we would have a test case, that would not catch the regression deterministically.
We could include the minimal reproducer for CTU analysis - the way we observed and tracked down this crash.

AFAIK, it did not reproduce with memory sanitizers.

Address and ubsan could not catch it (LLVM_USE_SANITIZER: Address;Undefined). And I had trouble with the Memory and MemoryWithOrigins setup because ninja could not generate the .inc files since clang-tblgen failed always with uninited errors. Fortunately, vlagrind could catch the error consistently all the time.

[martong]

In D97849#2600300, @teemperor wrote:

We can (and do) run LIT tests under valgrind, so if you have a test that triggers this valgrind error then that seems like the way to go. I don't think the test has to deterministically fail outside valgrind to be a valid test.

Do you mean to use lit with --vg or should we explicitly call valgrind in a lit test? If the latter, could you please show an example how to do that properly (I could not find any explicit valgrind call, not even in lldb) ?

[aaron.ballman]

I don't think we have any tests that explicitly use valgrind and testing with valgrind enabled is an optional thing. I would be fine landing this as-is without a test case if it turns out that testing this is too involved, so I'm accepting the review. However, if it turns out there is a way to test it that others can think of, I would appreciate adding the test coverage (even if it's post-commit).

[martong]

Add a test case which fails if lit --vg is used.

[martong]

Thanks for the review!

[teemperor]

Sorry for the delay. Yes, I meant the --vg flag which is I think the only valid way to test this from your description. That it can't be deterministically tested without this (or in configurations that don't run valgrind) is fine IMHO.

clang/include/clang/AST/Stmt.h

 };
 
 /// SwitchStmt - This represents a 'switch' stmt.
 class SwitchStmt final : public Stmt,
                          private llvm::TrailingObjects<SwitchStmt, Stmt *> {
   friend TrailingObjects;
 
   /// Points to a linked list of case and default statements.
-  SwitchCase *FirstCase;
+  SwitchCase *FirstCase = nullptr;
 
   // SwitchStmt is followed by several trailing objects,
   // some of which optional. Note that it would be more convenient to
   // put the optional trailing objects at the end but this would change
   // the order in children().
   // The trailing objects are in order:
   //
   // * A "Stmt *" for the init statement.

clang/test/Analysis/Inputs/ctu-other.c

 int identImplicit(int in) {
   return in;
 }
 
 // ASTImporter doesn't support this construct.
 int structInProto(struct DataType {int a;int b; } * d) {
   return 0;
 }
+
+int switchWithoutCases(int x) {
+  switch (x) {
+  };
+  return 0;
+}

clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt

 c:@F@inlineAsm ctu-other.c.ast
 c:@F@g ctu-other.c.ast
 c:@F@f ctu-other.c.ast
 c:@F@enumCheck ctu-other.c.ast
 c:@F@identImplicit ctu-other.c.ast
 c:@F@structInProto ctu-other.c.ast
+c:@F@switchWithoutCases ctu-other.c.ast

clang/test/Analysis/ctu-main.c

 };
 int structInProto(struct DataType *d);
 void testStructDefInArgument() {
   struct DataType d;
   d.a = 1;
   d.b = 0;
   clang_analyzer_eval(structInProto(&d) == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
 }
+
+int switchWithoutCases(int);
+void testSwitchStmtCrash(int x) {
+  switchWithoutCases(x);
+}