[clang] 2e90fc2 - [AST][PCH][ASTImporter] Fix UB caused by uninited SwitchStmt member

[Gabor Marton via cfe-commits]

Author: Gabor Marton
Date: 2021-03-04T15:10:04+01:00
New Revision: 2e90fc2c407bb9a85e14fa2c75533a984e5a765a

URL: https://github.com/llvm/llvm-project/commit/2e90fc2c407bb9a85e14fa2c75533a984e5a765a
DIFF: https://github.com/llvm/llvm-project/commit/2e90fc2c407bb9a85e14fa2c75533a984e5a765a.diff

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

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)
```

Differential Revision: https://reviews.llvm.org/D97849

Added: 
    

Modified: 
    clang/include/clang/AST/Stmt.h
    clang/test/Analysis/Inputs/ctu-other.c
    clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
    clang/test/Analysis/ctu-main.c

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index c2e69a91e55d..2085904b7f01 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@ class SwitchStmt final : public 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

diff  --git a/clang/test/Analysis/Inputs/ctu-other.c b/clang/test/Analysis/Inputs/ctu-other.c
index 16ebb3506f60..48a3f322cbd5 100644
--- a/clang/test/Analysis/Inputs/ctu-other.c
+++ b/clang/test/Analysis/Inputs/ctu-other.c
@@ -49,3 +49,9 @@ int identImplicit(int in) {
 int structInProto(struct DataType {int a;int b; } * d) {
   return 0;
 }
+
+int switchWithoutCases(int x) {
+  switch (x) {
+  };
+  return 0;
+}

diff  --git a/clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt b/clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
index 9abaa501a4cb..a6de9304551b 100644
--- a/clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
+++ b/clang/test/Analysis/Inputs/ctu-other.c.externalDefMap.ast-dump.txt
@@ -4,3 +4,4 @@ c:@F at f ctu-other.c.ast
 c:@F at enumCheck ctu-other.c.ast
 c:@F at identImplicit ctu-other.c.ast
 c:@F at structInProto ctu-other.c.ast
+c:@F at switchWithoutCases ctu-other.c.ast

diff  --git a/clang/test/Analysis/ctu-main.c b/clang/test/Analysis/ctu-main.c
index d991eb73a95c..1415490668ba 100644
--- a/clang/test/Analysis/ctu-main.c
+++ b/clang/test/Analysis/ctu-main.c
@@ -69,3 +69,8 @@ void testStructDefInArgument() {
   d.b = 0;
   clang_analyzer_eval(structInProto(&d) == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
 }
+
+int switchWithoutCases(int);
+void testSwitchStmtCrash(int x) {
+  switchWithoutCases(x);
+}