[analyzer] Fix crash of StreamChecker when eval calling 'fopen' #100990
Conversation
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
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balazs Benics (steakhal) ChangesActually, on the failure branch of To fix this, let's just only add this no-alias property for the success Fixes #100901 Full diff: https://github.com/llvm/llvm-project/pull/100990.diff 3 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 76a9aae170893..452948aa2cb00 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1703,7 +1703,11 @@ are detected:
* Invalid 3rd ("``whence``") argument to ``fseek``.
The stream operations are by this checker usually split into two cases, a success
-and a failure case. However, in the case of write operations (like ``fwrite``,
+and a failure case.
+On the success case it also assumes that the current value of ``stdout``,
+``stderr``, or ``stdin`` can't be equal to the file pointer returned by ``fopen``.
+
+In the case of write operations (like ``fwrite``,
``fprintf`` and even ``fsetpos``) this behavior could produce a large amount of
unwanted reports on projects that don't have error checks around the write
operations, so by default the checker assumes that write operations always succeed.
@@ -1766,13 +1770,6 @@ are assumed to succeed.)
fclose(p);
}
-**Limitations**
-
-The checker does not track the correspondence between integer file descriptors
-and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not
-treated specially and are therefore often not recognized (because these streams
-are usually not opened explicitly by the program, and are global variables).
-
.. _osx-checkers:
osx
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 53770532609d5..4454f30630e27 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -615,30 +615,22 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
});
}
- void initMacroValues(CheckerContext &C) const {
+ void initMacroValues(const Preprocessor &PP) const {
if (EofVal)
return;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("EOF", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("EOF", PP))
EofVal = *OptInt;
else
EofVal = -1;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("SEEK_SET", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_SET", PP))
SeekSetVal = *OptInt;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("SEEK_END", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_END", PP))
SeekEndVal = *OptInt;
- if (const std::optional<int> OptInt =
- tryExpandAsInteger("SEEK_CUR", C.getPreprocessor()))
+ if (const std::optional<int> OptInt = tryExpandAsInteger("SEEK_CUR", PP))
SeekCurVal = *OptInt;
}
- void initVaListType(CheckerContext &C) const {
- VaListType = C.getASTContext().getBuiltinVaListType().getCanonicalType();
- }
-
/// Searches for the ExplodedNode where the file descriptor was acquired for
/// StreamSym.
static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
@@ -880,9 +872,6 @@ static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
void StreamChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
- initMacroValues(C);
- initVaListType(C);
-
const FnDescription *Desc = lookupFn(Call);
if (!Desc || !Desc->PreFn)
return;
@@ -938,7 +927,6 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
assert(RetSym && "RetVal must be a symbol here.");
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
- State = assumeNoAliasingWithStdStreams(State, RetVal, C);
// Bifurcate the state into two: one with a valid FILE* pointer, the other
// with a NULL.
@@ -951,6 +939,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
StateNull =
StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
+ StateNotNull = assumeNoAliasingWithStdStreams(StateNotNull, RetVal, C);
+
C.addTransition(StateNotNull,
constructLeakNoteTag(C, RetSym, "Stream opened here"));
C.addTransition(StateNull);
@@ -2081,10 +2071,12 @@ getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName) {
}
void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
- AnalysisManager &, BugReporter &) const {
+ AnalysisManager &Mgr, BugReporter &) const {
StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+ VaListType = TU->getASTContext().getBuiltinVaListType().getCanonicalType();
+ initMacroValues(Mgr.getPreprocessor());
}
//===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index b3a47ce4153d3..b9a5b1ba8cd49 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -1,11 +1,11 @@
// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
// RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
// RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
// RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,unix.Stream,debug.ExprInspection \
-// RUN: -analyzer-config unix.Stream:Pedantic=true -verify %s
+// RUN: -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
#include "Inputs/system-header-simulator.h"
#include "Inputs/system-header-simulator-for-malloc.h"
@@ -499,14 +499,34 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
fclose(f);
}
-extern FILE *stdout_like_ptr;
-void no_aliasing(void) {
+extern FILE *non_standard_stream_ptr;
+void test_fopen_does_not_alias_with_standard_streams(void) {
FILE *f = fopen("file", "r");
- clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE
- clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
- clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
- clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}}
- if (f && f != stdout) {
+ if (!f) return;
+ clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE
+ clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
+ clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
+ clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning {{UNKNOWN}}
+ if (f != stdout) {
fclose(f);
}
} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
+
+void reopen_std_stream(void) {
+ FILE *oldStdout = stdout;
+ fclose(stdout);
+ FILE *fp = fopen("blah", "w");
+ if (!fp) return;
+
+ stdout = fp; // Let's make them alias.
+ clang_analyzer_eval(fp == oldStdout); // expected-warning {{UNKNOWN}}
+ clang_analyzer_eval(fp == stdout); // expected-warning {{TRUE}} no-FALSE
+ clang_analyzer_eval(oldStdout == stdout); // expected-warning {{UNKNOWN}}
+}
+
+void only_success_path_does_not_alias_with_stdout(void) {
+ if (stdout) return;
+ FILE *f = fopen("/tmp/foof", "r"); // no-crash
+ if (!f) return;
+ fclose(f);
+}
|
clang/docs/analyzer/checkers.rst
Outdated
| The checker does not track the correspondence between integer file descriptors | ||
| and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not | ||
| treated specially and are therefore often not recognized (because these streams | ||
| are usually not opened explicitly by the program, and are global variables). |
There was a problem hiding this comment.
I wasn't sure of this section, so I just removed it. Let me know if it's still relevant or not.
There was a problem hiding this comment.
Hmm, perhaps keep a note saying "This checker does not track operations that use integer file descriptors instead of FILE * pointers". (If I recall correctly this is true and might be relevant for someone who's used to work with low-level stuff.) However it's also completely fine (from my POV) if you delete the whole Limitations section.
There was a problem hiding this comment.
Yea, but I didn't know exactly what it refers to. We could add an example for such a case there. That would help.
There was a problem hiding this comment.
It refers to functions like write (man page) where the file is specified by an int argument.
There was a problem hiding this comment.
Ah indeed. So basically opening a file with file descriptor "ints" and then using fdopen() to get the corresponding FILE * - and back with fileno(). Yea, that seems weird to do :D Probably has its use somewhere though.
Anyways, makes sense now.
|
I decided to put the fixup NFC changes along with this PR (the ones were submitted after I merged the original commit), but on hindsight probably it would be better to merge those NFC changes separately. |
|
LGTM, someone else must approve. Thanks for fixing this! |
clang/docs/analyzer/checkers.rst
Outdated
| The checker does not track the correspondence between integer file descriptors | ||
| and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not | ||
| treated specially and are therefore often not recognized (because these streams | ||
| are usually not opened explicitly by the program, and are global variables). |
There was a problem hiding this comment.
Hmm, perhaps keep a note saying "This checker does not track operations that use integer file descriptors instead of FILE * pointers". (If I recall correctly this is true and might be relevant for someone who's used to work with low-level stuff.) However it's also completely fine (from my POV) if you delete the whole Limitations section.
Feel free to keep the NFC changes in this commit, just briefly mention them in the description ("This commit also contains some minor NFC code quality improvements." or something similar.) |
|
@balazske Could you please have a look? I wanna make sure everyone is aligned. |
Elaborated the docs section. |
Actually, on the failure branch of
fopen, the resulting pointer couldalias with
stdoutiffstdoutis 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