[analyzer] Avoid crashes in the stream checker #100901
Conversation
This change avoids crashes in the stream checker when bifurcating the analysis state produces a non-null return value for opening a stream and does not produces a null return value because of constraints found during an analysis path. A snippet of the crash stack seen is shown here. ... llvm#6 __restore_rt sigaction.c:0:0 llvm#7 clang::ento::ProgramState::getStateManager() const clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:148:13 llvm#8 llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const> clang::ento::ProgramState::set<(anonymous namespace)::StreamMap>(clang::ento::ProgramStateTrait<( anonymous namespace)::StreamMap>::key_type,clang::ento::ProgramStateTrait<(anonymous namespace)::StreamMap>::value_type) const clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:864:10 llvm#9 (anonymous namespace)::StreamChecker::evalFopen((anonymous namespace)::FnDescription const*, clang::ento::CallEvent const&, clang::ento::CheckerContext&) const clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:951:13 ...
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: None (vabridgers) ChangesThis change avoids crashes in the stream checker when bifurcating the analysis state produces a non-null return value for opening a stream and does not produces a null return value because of constraints found during an analysis path. A snippet of the crash stack seen is shown here. ... Full diff: https://github.com/llvm/llvm-project/pull/100901.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 53770532609d5..c34c6b9fc6c0d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -946,14 +946,18 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
std::tie(StateNotNull, StateNull) =
C.getConstraintManager().assumeDual(State, RetVal);
- StateNotNull =
- StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
- StateNull =
- StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
-
- C.addTransition(StateNotNull,
- constructLeakNoteTag(C, RetSym, "Stream opened here"));
- C.addTransition(StateNull);
+ if (StateNotNull)
+ StateNotNull =
+ StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
+ if (StateNull)
+ StateNull =
+ StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
+
+ if (StateNotNull)
+ C.addTransition(StateNotNull,
+ constructLeakNoteTag(C, RetSym, "Stream opened here"));
+ if (StateNull)
+ C.addTransition(StateNull);
}
void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
diff --git a/clang/test/Analysis/stream-no-crash.c b/clang/test/Analysis/stream-no-crash.c
new file mode 100644
index 0000000000000..7a4922c8a35c2
--- /dev/null
+++ b/clang/test/Analysis/stream-no-crash.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Stream -verify %s
+
+// This test is isolate since it uses line markers to repro the problem.
+// The test is expected to find the issues noted below without crashing.
+
+# 1 "" 1
+# 1 "" 1
+# 1 "" 1
+# 1 "" 1 3
+typedef FILE;
+extern *stdout;
+char a;
+*fopen();
+# 0 "" 2
+# 7 "" 2
+# 7 "" 2
+# 7 "" 2
+void b() {
+ fopen(&a, "");
+ int c = stdout && c;
+ b();
+}
+// expected-warning@-3{{Assigned value is garbage or undefined}}
+// expected-warning@-4{{Opened stream never closed. Potential resource leak}}
+
|
|
@llvm/pr-subscribers-clang Author: None (vabridgers) ChangesThis change avoids crashes in the stream checker when bifurcating the analysis state produces a non-null return value for opening a stream and does not produces a null return value because of constraints found during an analysis path. A snippet of the crash stack seen is shown here. ... Full diff: https://github.com/llvm/llvm-project/pull/100901.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 53770532609d5..c34c6b9fc6c0d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -946,14 +946,18 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
std::tie(StateNotNull, StateNull) =
C.getConstraintManager().assumeDual(State, RetVal);
- StateNotNull =
- StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
- StateNull =
- StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
-
- C.addTransition(StateNotNull,
- constructLeakNoteTag(C, RetSym, "Stream opened here"));
- C.addTransition(StateNull);
+ if (StateNotNull)
+ StateNotNull =
+ StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
+ if (StateNull)
+ StateNull =
+ StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
+
+ if (StateNotNull)
+ C.addTransition(StateNotNull,
+ constructLeakNoteTag(C, RetSym, "Stream opened here"));
+ if (StateNull)
+ C.addTransition(StateNull);
}
void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
diff --git a/clang/test/Analysis/stream-no-crash.c b/clang/test/Analysis/stream-no-crash.c
new file mode 100644
index 0000000000000..7a4922c8a35c2
--- /dev/null
+++ b/clang/test/Analysis/stream-no-crash.c
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Stream -verify %s
+
+// This test is isolate since it uses line markers to repro the problem.
+// The test is expected to find the issues noted below without crashing.
+
+# 1 "" 1
+# 1 "" 1
+# 1 "" 1
+# 1 "" 1 3
+typedef FILE;
+extern *stdout;
+char a;
+*fopen();
+# 0 "" 2
+# 7 "" 2
+# 7 "" 2
+# 7 "" 2
+void b() {
+ fopen(&a, "");
+ int c = stdout && c;
+ b();
+}
+// expected-warning@-3{{Assigned value is garbage or undefined}}
+// expected-warning@-4{{Opened stream never closed. Potential resource leak}}
+
|
|
A little background on how this was found, and a few debugging notes. This was found in a daily static analysis systems level test that we drive internally on our daily integrations, on the lz4 project, found here https://github.com/lz4/lz4.git. There are a number of open source projects that we drive these daily code analysis tests on, and in this case we seemed to have caught a regression because previous tests had been passing. When I looked at state after a crash through gdb, the crash occurred because StateNull in method evalFopen was NULL, and StateNotNull was not NULL - so that seemed to indicate the assumeDual(State, RetVal) operation in method evalFopen had not returned expected non null states. That led me to look at RetSym and it's constraints in Program State. See below. I suspect the change to add assumeNoAliasingWithStdStreams somehow is causing this since when I revert b60fec2 ( [analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (#100085) ) I do not see this problem. (gdb) p StateNull (gdb) p RetSym->dump() (gdb) p State->dump() |
|
In the state dump I see that |
Exactly. I didn't want to rush too much, but I can share that my current idea is to call |
This can be a better (and more simple) solution. |
|
Thanks for the comments. I do not mind abandoning this change in favor of a better solution. Please let me know when you have posted an improved PR and I'll abandon this one. Best |
|
Thanks for the really valuable reduced case @vabridgers. I'll close this in favor of #100990 if you don't mind. |
|
Thanks @steakhal ! |
) Actually, on the failure branch of `fopen`, the resulting pointer could alias with `stdout` iff `stdout` is already known to be null. We crashed in this case as the implementation assumed that the state-split for creating the success and failure branches both should be viable; thus dereferenced both of those states - leading to the crash. To fix this, let's just only add this no-alias property for the success path, and that's it :) Fixes #100901
This change avoids crashes in the stream checker when bifurcating the analysis state produces a non-null return value for opening a stream and does not produces a null return value because of constraints found during an analysis path.
A snippet of the crash stack seen is shown here.
...
#6 __restore_rt sigaction.c:0:0
#7 clang::ento::ProgramState::getStateManager() const
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:148:13
#8 llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>
clang::ento::ProgramState::set<(anonymous namespace)::StreamMap>(clang::ento::ProgramStateTrait<(
anonymous namespace)::StreamMap>::key_type,clang::ento::ProgramStateTrait<(anonymous namespace)::StreamMap>::value_type) const
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:864:10
#9 (anonymous namespace)::StreamChecker::evalFopen((anonymous namespace)::FnDescription const*,
clang::ento::CallEvent const&, clang::ento::CheckerContext&) const
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:951:13
...