Skip to content

Commit b60fec2

Browse files
authored
[analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (#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
1 parent d5a614d commit b60fec2

File tree

2 files changed

+83
-1
lines changed

2 files changed

+83
-1
lines changed

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,8 @@ inline void assertStreamStateOpened(const StreamState *SS) {
254254
}
255255

256256
class StreamChecker : public Checker<check::PreCall, eval::Call,
257-
check::DeadSymbols, check::PointerEscape> {
257+
check::DeadSymbols, check::PointerEscape,
258+
check::ASTDecl<TranslationUnitDecl>> {
258259
BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
259260
BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
260261
BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -276,11 +277,21 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
276277
const CallEvent *Call,
277278
PointerEscapeKind Kind) const;
278279

280+
/// Finds the declarations of 'FILE *stdin, *stdout, *stderr'.
281+
void checkASTDecl(const TranslationUnitDecl *TU, AnalysisManager &,
282+
BugReporter &) const;
283+
279284
const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
280285
const BugType *getBT_IndeterminatePosition() const {
281286
return &BT_IndeterminatePosition;
282287
}
283288

289+
/// Assumes that the result of 'fopen' can't alias with the pointee of
290+
/// 'stdin', 'stdout' or 'stderr'.
291+
ProgramStateRef assumeNoAliasingWithStdStreams(ProgramStateRef State,
292+
DefinedSVal RetVal,
293+
CheckerContext &C) const;
294+
284295
const NoteTag *constructSetEofNoteTag(CheckerContext &C,
285296
SymbolRef StreamSym) const {
286297
return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
@@ -451,6 +462,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
451462
/// The built-in va_list type is platform-specific
452463
mutable QualType VaListType;
453464

465+
mutable const VarDecl *StdinDecl = nullptr;
466+
mutable const VarDecl *StdoutDecl = nullptr;
467+
mutable const VarDecl *StderrDecl = nullptr;
468+
454469
void evalFopen(const FnDescription *Desc, const CallEvent &Call,
455470
CheckerContext &C) const;
456471

@@ -887,6 +902,30 @@ bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
887902
return C.isDifferent();
888903
}
889904

905+
ProgramStateRef StreamChecker::assumeNoAliasingWithStdStreams(
906+
ProgramStateRef State, DefinedSVal RetVal, CheckerContext &C) const {
907+
auto assumeRetNE = [&C, RetVal](ProgramStateRef State,
908+
const VarDecl *Var) -> ProgramStateRef {
909+
if (!Var)
910+
return State;
911+
const auto *LCtx = C.getLocationContext();
912+
auto &StoreMgr = C.getStoreManager();
913+
auto &SVB = C.getSValBuilder();
914+
SVal VarValue = State->getSVal(StoreMgr.getLValueVar(Var, LCtx));
915+
auto NoAliasState =
916+
SVB.evalBinOp(State, BO_NE, RetVal, VarValue, SVB.getConditionType())
917+
.castAs<DefinedOrUnknownSVal>();
918+
return State->assume(NoAliasState, true);
919+
};
920+
921+
assert(State);
922+
State = assumeRetNE(State, StdinDecl);
923+
State = assumeRetNE(State, StdoutDecl);
924+
State = assumeRetNE(State, StderrDecl);
925+
assert(State);
926+
return State;
927+
}
928+
890929
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
891930
CheckerContext &C) const {
892931
ProgramStateRef State = C.getState();
@@ -899,6 +938,7 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
899938
assert(RetSym && "RetVal must be a symbol here.");
900939

901940
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
941+
State = assumeNoAliasingWithStdStreams(State, RetVal, C);
902942

903943
// Bifurcate the state into two: one with a valid FILE* pointer, the other
904944
// with a NULL.
@@ -2017,6 +2057,36 @@ ProgramStateRef StreamChecker::checkPointerEscape(
20172057
return State;
20182058
}
20192059

2060+
static const VarDecl *
2061+
getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName) {
2062+
ASTContext &Ctx = TU->getASTContext();
2063+
const auto &SM = Ctx.getSourceManager();
2064+
const QualType FileTy = Ctx.getFILEType();
2065+
2066+
if (FileTy.isNull())
2067+
return nullptr;
2068+
2069+
const QualType FilePtrTy = Ctx.getPointerType(FileTy).getCanonicalType();
2070+
2071+
auto LookupRes = TU->lookup(&Ctx.Idents.get(VarName));
2072+
for (const Decl *D : LookupRes) {
2073+
if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
2074+
if (SM.isInSystemHeader(VD->getLocation()) && VD->hasExternalStorage() &&
2075+
VD->getType().getCanonicalType() == FilePtrTy) {
2076+
return VD;
2077+
}
2078+
}
2079+
}
2080+
return nullptr;
2081+
}
2082+
2083+
void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
2084+
AnalysisManager &, BugReporter &) const {
2085+
StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
2086+
StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
2087+
StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
2088+
}
2089+
20202090
//===----------------------------------------------------------------------===//
20212091
// Checker registration.
20222092
//===----------------------------------------------------------------------===//

clang/test/Analysis/stream.c

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,3 +498,15 @@ void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) {
498498
fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash
499499
fclose(f);
500500
}
501+
502+
extern FILE *stdout_like_ptr;
503+
void no_aliasing(void) {
504+
FILE *f = fopen("file", "r");
505+
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}} no-TRUE
506+
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}} no-TRUE
507+
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}} no-TRUE
508+
clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}} expected-warning {{TRUE}}
509+
if (f && f != stdout) {
510+
fclose(f);
511+
}
512+
} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.

0 commit comments

Comments
 (0)