Skip to content

[clang][analyzer] Fix a nullptr dereference when -ftime-trace is used #139820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 14, 2025

Conversation

fangyi-zhou
Copy link
Contributor

Fixes #139779.

The bug was introduced in #137355 in SymbolConjured::getStmt, when trying to obtain a statement for a CFG initializer without an initializer. This commit adds a null check before access.

Fixes llvm#139779.

The bug was introduced in llvm#137355 in `SymbolConjured::getStmt`, when
trying to obtain a statement for a CFG initializer without an
initializer.  This commit adds a null check before access.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels May 14, 2025
@llvmbot
Copy link
Member

llvmbot commented May 14, 2025

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Fangyi Zhou (fangyi-zhou)

Changes

Fixes #139779.

The bug was introduced in #137355 in SymbolConjured::getStmt, when trying to obtain a statement for a CFG initializer without an initializer. This commit adds a null check before access.


Full diff: https://github.com/llvm/llvm-project/pull/139820.diff

2 Files Affected:

  • (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h (+2)
  • (added) clang/test/Analysis/ftime-trace-no-init.cpp (+5)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
index 9e7c98fdded17..00159971fd7b5 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
@@ -103,6 +103,8 @@ class SymbolConjured : public SymbolData {
   const Stmt *getStmt() const {
     switch (Elem->getKind()) {
     case CFGElement::Initializer:
+      if (Elem->castAs<CFGInitializer>().getInitializer() == nullptr)
+        return nullptr;
       return Elem->castAs<CFGInitializer>().getInitializer()->getInit();
     case CFGElement::ScopeBegin:
       return Elem->castAs<CFGScopeBegin>().getTriggerStmt();
diff --git a/clang/test/Analysis/ftime-trace-no-init.cpp b/clang/test/Analysis/ftime-trace-no-init.cpp
new file mode 100644
index 0000000000000..db62aa8a56ed7
--- /dev/null
+++ b/clang/test/Analysis/ftime-trace-no-init.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang --analyze %s -ftime-trace -Xclang -verify
+// expected-no-diagnostics
+
+// GitHub issue 139779
+struct {} a; // no-crash

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM but a suggestion for a perhaps more clear way to do it

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only agree with Aaron. It looks nice.
Thank you!

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@steakhal steakhal merged commit 440e510 into llvm:main May 14, 2025
7 of 11 checks passed
@fangyi-zhou
Copy link
Contributor Author

This seems to cause asan test failures in buildbot, I'll create a PR to revert.

qinkunbao pushed a commit that referenced this pull request May 14, 2025
…e` is used" (#139936)

Reverts #139820

Reverting due to buildbot failures in asan
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 14, 2025
…-ftime-trace` is used" (#139936)

Reverts llvm/llvm-project#139820

Reverting due to buildbot failures in asan
steakhal pushed a commit that referenced this pull request May 15, 2025
… (Reland) (#139980)

Fixes #139779.

The bug was introduced in #137355 in `SymbolConjured::getStmt`, when
trying to obtain a statement for a CFG initializer without an
initializer.  This commit adds a null check before access.

Previous PR #139820, Revert #139936

Additional notes since previous PR:

When conjuring a symbol, sometimes there is no valid CFG element, e.g.
in the file causing the crash, there is no element at all in the CFG. In
these cases, the CFG element reference in the expression engine will be
invalid. As a consequence, there needs to be extra checks to ensure the
validity of the CFG element reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New --analyze crash with -ftime-trace
4 participants