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

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: riccibruno, aaron.ballman, shafik, teemperor, rsmith.
Herald added subscribers: gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
martong requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97849

Files:
  clang/include/clang/AST/Stmt.h


Index: clang/include/clang/AST/Stmt.h
===================================================================
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -2119,7 +2119,7 @@
   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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D97849.327745.patch
Type: text/x-patch
Size: 483 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210303/35cb0dcb/attachment.bin>