[clang] 13d39cb - [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (#100990)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 29 05:15:06 PDT 2024
Author: Balazs Benics
Date: 2024-07-29T14:15:02+02:00
New Revision: 13d39cb6f7b20e596b66f59ebf4dba766451398b
URL: https://github.com/llvm/llvm-project/commit/13d39cb6f7b20e596b66f59ebf4dba766451398b
DIFF: https://github.com/llvm/llvm-project/commit/13d39cb6f7b20e596b66f59ebf4dba766451398b.diff
LOG: [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (#100990)
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
Added:
Modified:
clang/docs/analyzer/checkers.rst
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream.c
Removed:
################################################################################
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 76a9aae170893..05d3f4d4018f7 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1703,7 +1703,13 @@ 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``.
+Operations performed on ``stdout``, ``stderr``, or ``stdin`` are not checked by
+this checker in contrast to the streams opened 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.
@@ -1769,9 +1775,7 @@ are assumed to succeed.)
**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).
+and ``FILE *`` pointers.
.. _osx-checkers:
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);
+}
More information about the cfe-commits
mailing list