-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[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
Conversation
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.
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Fangyi Zhou (fangyi-zhou) ChangesFixes #139779. The bug was introduced in #137355 in Full diff: https://github.com/llvm/llvm-project/pull/139820.diff 2 Files Affected:
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
|
There was a problem hiding this 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
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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!
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This seems to cause asan test failures in buildbot, I'll create a PR to revert. |
…-ftime-trace` is used" (#139936) Reverts llvm/llvm-project#139820 Reverting due to buildbot failures in asan
… (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.
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.