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


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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to