[clang] [analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (PR #100085)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 23 02:10:50 PDT 2024
https://github.com/steakhal created https://github.com/llvm/llvm-project/pull/100085
'fopen' should return a new FILE handle, thus we should assume it can't alias with commonly used FILE handles, such as with 'stdin', 'stdout' or 'stderr'.
This problem appears in code that handles either some input/output file with stdin or stdout, as the business logic is basically the same no matter the stream being used.
However, one would should only close the stream if it was opened via 'fopen'. Consequently, such code usually has a condition like `if (f && f != stdout)` to guard the `fclose()` call.
This patch brings this assumption, thus eliminates FPs for not taking the guarded branch.
CPP-5306
>From c8148c88a39434501b9dacb374b3ca9d81ee8fdf Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Tue, 23 Jul 2024 08:55:16 +0200
Subject: [PATCH] [analyzer] Assume the result of 'fopen' can't alias with
'std{in,out,err}'
'fopen' should return a new FILE handle, thus we should assume it can't
alias with commonly used FILE handles, such as with 'stdin', 'stdout' or
'stderr'.
This problem appears in code that handles either some input/output file
with stdin or stdout, as the business logic is basically the same no
matter the stream being used.
However, one would should only close the stream if it was opened via
'fopen'. Consequently, such code usually has a condition like
`if (f && f != stdout)` to guard the `fclose()` call.
This patch brings this assumption, thus eliminates FPs for not taking
the guarded branch.
CPP-5306
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 72 ++++++++++++++++++-
clang/test/Analysis/stream.c | 11 +++
2 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index e8d538388e56c..53770532609d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -254,7 +254,8 @@ inline void assertStreamStateOpened(const StreamState *SS) {
}
class StreamChecker : public Checker<check::PreCall, eval::Call,
- check::DeadSymbols, check::PointerEscape> {
+ check::DeadSymbols, check::PointerEscape,
+ check::ASTDecl<TranslationUnitDecl>> {
BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -276,11 +277,21 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
const CallEvent *Call,
PointerEscapeKind Kind) const;
+ /// Finds the declarations of 'FILE *stdin, *stdout, *stderr'.
+ void checkASTDecl(const TranslationUnitDecl *TU, AnalysisManager &,
+ BugReporter &) const;
+
const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
const BugType *getBT_IndeterminatePosition() const {
return &BT_IndeterminatePosition;
}
+ /// Assumes that the result of 'fopen' can't alias with the pointee of
+ /// 'stdin', 'stdout' or 'stderr'.
+ ProgramStateRef assumeNoAliasingWithStdStreams(ProgramStateRef State,
+ DefinedSVal RetVal,
+ CheckerContext &C) const;
+
const NoteTag *constructSetEofNoteTag(CheckerContext &C,
SymbolRef StreamSym) const {
return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
@@ -451,6 +462,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
/// The built-in va_list type is platform-specific
mutable QualType VaListType;
+ mutable const VarDecl *StdinDecl = nullptr;
+ mutable const VarDecl *StdoutDecl = nullptr;
+ mutable const VarDecl *StderrDecl = nullptr;
+
void evalFopen(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
@@ -887,6 +902,30 @@ bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
return C.isDifferent();
}
+ProgramStateRef StreamChecker::assumeNoAliasingWithStdStreams(
+ ProgramStateRef State, DefinedSVal RetVal, CheckerContext &C) const {
+ auto assumeRetNE = [&C, RetVal](ProgramStateRef State,
+ const VarDecl *Var) -> ProgramStateRef {
+ if (!Var)
+ return State;
+ const auto *LCtx = C.getLocationContext();
+ auto &StoreMgr = C.getStoreManager();
+ auto &SVB = C.getSValBuilder();
+ SVal VarValue = State->getSVal(StoreMgr.getLValueVar(Var, LCtx));
+ auto NoAliasState =
+ SVB.evalBinOp(State, BO_NE, RetVal, VarValue, SVB.getConditionType())
+ .castAs<DefinedOrUnknownSVal>();
+ return State->assume(NoAliasState, true);
+ };
+
+ assert(State);
+ State = assumeRetNE(State, StdinDecl);
+ State = assumeRetNE(State, StdoutDecl);
+ State = assumeRetNE(State, StderrDecl);
+ assert(State);
+ return State;
+}
+
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
@@ -899,6 +938,7 @@ 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.
@@ -2017,6 +2057,36 @@ ProgramStateRef StreamChecker::checkPointerEscape(
return State;
}
+static const VarDecl *
+getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName) {
+ ASTContext &Ctx = TU->getASTContext();
+ const auto &SM = Ctx.getSourceManager();
+ const QualType FileTy = Ctx.getFILEType();
+
+ if (FileTy.isNull())
+ return nullptr;
+
+ const QualType FilePtrTy = Ctx.getPointerType(FileTy).getCanonicalType();
+
+ auto LookupRes = TU->lookup(&Ctx.Idents.get(VarName));
+ for (const Decl *D : LookupRes) {
+ if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
+ if (SM.isInSystemHeader(VD->getLocation()) && VD->hasExternalStorage() &&
+ VD->getType().getCanonicalType() == FilePtrTy) {
+ return VD;
+ }
+ }
+ }
+ return nullptr;
+}
+
+void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
+ AnalysisManager &, BugReporter &) const {
+ StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
+ StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
+ StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+}
+
//===----------------------------------------------------------------------===//
// Checker registration.
//===----------------------------------------------------------------------===//
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index c924cbd36f759..c518820df6ed6 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -498,3 +498,14 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash
fclose(f);
}
+
+extern FILE *stdout_like_ptr;
+void no_aliasing(void) {
+ FILE *f = fopen("file", "r");
+ 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) {
+ fclose(f);
+ }
+} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
More information about the cfe-commits
mailing list