This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by martong on Mar 3 2021, 4:50 AM.

Details

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)

Diff Detail

Event Timeline

martong created this revision.Mar 3 2021, 4:50 AM
martong requested review of this revision.Mar 3 2021, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 4:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

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.

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.

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.

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 accepted this revision.Mar 3 2021, 10:28 AM

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

This revision is now accepted and ready to land.Mar 3 2021, 10:28 AM
martong updated this revision to Diff 328149.Mar 4 2021, 6:07 AM

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

This revision was landed with ongoing or failed builds.Mar 4 2021, 6:10 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!

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.