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