balazske created this revision.
Herald added subscribers: cfe-commits, martong, Charusso, gamesh411, dkrupp,
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware,
xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a project: clang.
The validity checks are performed in precall callback.
Semantic of checking for opened stream has changed:
It is not allowed at all to use the stream after 'fclose'.
This is what cppreference.com says at fclose.
So the double-close error changes to use-after-close.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75163
Files:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -34,7 +34,7 @@
bool isOpened() const { return K == Opened; }
bool isClosed() const { return K == Closed; }
- //bool isOpenFailed() const { return K == OpenFailed; }
+ bool isOpenFailed() const { return K == OpenFailed; }
//bool isEscaped() const { return K == Escaped; }
bool operator==(const StreamState &X) const { return K == X.K; }
@@ -50,66 +50,77 @@
};
class StreamChecker;
-
-using FnCheck = std::function<void(const StreamChecker *, const CallEvent &,
+struct FnDescription;
+using FnCheck = std::function<void(const StreamChecker *, const FnDescription *, const CallEvent &,
CheckerContext &)>;
struct FnDescription {
+ FnCheck PreFn;
FnCheck EvalFn;
+ unsigned int StreamArgI;
};
-class StreamChecker : public Checker<eval::Call,
+class StreamChecker : public Checker<check::PreCall, eval::Call,
check::DeadSymbols > {
mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
- BT_doubleclose, BT_ResourceLeak;
+ BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak;
public:
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
- {{"fopen"}, {&StreamChecker::evalFopen}},
- {{"freopen", 3}, {&StreamChecker::evalFreopen}},
- {{"tmpfile"}, {&StreamChecker::evalFopen}},
- {{"fclose", 1}, {&StreamChecker::evalFclose}},
- {{"fread", 4},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 3)}},
- {{"fwrite", 4},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 3)}},
- {{"fseek", 3}, {&StreamChecker::evalFseek}},
- {{"ftell", 1},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
- {{"rewind", 1},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
- {{"fgetpos", 2},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
- {{"fsetpos", 2},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
- {{"clearerr", 1},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
- {{"feof", 1},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
- {{"ferror", 1},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
- {{"fileno", 1},
- {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
+ {{"fopen"}, {{}, &StreamChecker::evalFopen, -1}},
+ {{"freopen", 3}, {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
+ {{"tmpfile"}, {{}, &StreamChecker::evalFopen, -1}},
+ {{"fclose", 1}, {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
+ {{"fread", 4}, {&StreamChecker::preDefault, {}, 3}},
+ {{"fwrite", 4}, {&StreamChecker::preDefault, {}, 3}},
+ {{"fseek", 3}, {&StreamChecker::preFseek, {}}},
+ {{"ftell", 1}, {&StreamChecker::preDefault, {}, 0}},
+ {{"rewind", 1}, {&StreamChecker::preDefault, {}, 0}},
+ {{"fgetpos", 2}, {&StreamChecker::preDefault, {}, 0}},
+ {{"fsetpos", 2}, {&StreamChecker::preDefault, {}, 0}},
+ {{"clearerr", 1}, {&StreamChecker::preDefault, {}, 0}},
+ {{"feof", 1}, {&StreamChecker::preDefault, {}, 0}},
+ {{"ferror", 1}, {&StreamChecker::preDefault, {}, 0}},
+ {{"fileno", 1}, {&StreamChecker::preDefault, {}, 0}},
};
- void evalFopen(const CallEvent &Call, CheckerContext &C) const;
- void evalFreopen(const CallEvent &Call, CheckerContext &C) const;
- void evalFclose(const CallEvent &Call, CheckerContext &C) const;
- void evalFseek(const CallEvent &Call, CheckerContext &C) const;
- void checkArgNullStream(const CallEvent &Call, CheckerContext &C,
- unsigned ArgI) const;
-
- ProgramStateRef checkNullStream(SVal SV, CheckerContext &C,
- ProgramStateRef State) const;
- ProgramStateRef checkFseekWhence(SVal SV, CheckerContext &C,
- ProgramStateRef State) const;
- ProgramStateRef checkDoubleClose(const CallEvent &Call, CheckerContext &C,
- ProgramStateRef State) const;
+ void preDefault(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const;
+ void preFseek(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const;
+ void preFreopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const;
+
+ void evalFopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const;
+ void evalFreopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const;
+ void evalFclose(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const;
+
+ ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C, ProgramStateRef State) const;
+ ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C, ProgramStateRef State) const;
+ ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C,
+ ProgramStateRef State) const;
+
+ const FnDescription *lookupFn(const CallEvent &Call) const {
+ // Recognize "global C functions" with only integral or pointer arguments
+ // (and matching name) as stream functions.
+ if (!Call.isGlobalCFunction())
+ return nullptr;
+ for (auto P : Call.parameters()) {
+ QualType T = P->getType();
+ if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
+ return nullptr;
+ }
+ // Ensure that not a member function is called.
+ const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+ if (!FD || FD->getKind() != Decl::Function)
+ return nullptr;
+
+ return FnDescriptions.lookup(Call);
+ }
+
};
} // end anonymous namespace
@@ -117,31 +128,64 @@
REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
-bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
- const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
- if (!FD || FD->getKind() != Decl::Function)
- return false;
-
- // Recognize "global C functions" with only integral or pointer arguments
- // (and matching name) as stream functions.
- if (!Call.isGlobalCFunction())
- return false;
- for (auto P : Call.parameters()) {
- QualType T = P->getType();
- if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
- return false;
- }
+void StreamChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+ const FnDescription *Desc = lookupFn(Call);
+ if (!Desc)
+ return;
- const FnDescription *Description = FnDescriptions.lookup(Call);
- if (!Description)
- return false;
+ (Desc->PreFn)(this, Desc, Call, C);
+}
+
+bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
+ const FnDescription *Desc = lookupFn(Call);
+ if (!Desc)
+ return;
- (Description->EvalFn)(this, Call, C);
+ (Desc->EvalFn)(this, Desc, Call, C);
return C.isDifferent();
}
-void StreamChecker::evalFopen(const CallEvent &Call, CheckerContext &C) const {
+
+void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ State = ensureStreamNonNull(Call.getArgSVal(Desc->StreamArgI), C, State);
+ if (!State)
+ return;
+ State = ensureStreamOpened(Call.getArgSVal(Desc->StreamArgI), C, State);
+ if (!State)
+ return;
+
+ C.addTransition(State);
+}
+
+void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ State = ensureStreamNonNull(Call.getArgSVal(Desc->StreamArgI), C, State);
+ if (!State)
+ return;
+ State = ensureStreamOpened(Call.getArgSVal(Desc->StreamArgI), C, State);
+ if (!State)
+ return;
+ State = ensureFseekWhenceCorrect(Call.getArgSVal(2), C, State);
+ if (!State)
+ return;
+
+ C.addTransition(State);
+}
+
+void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ // Do not allow NULL as passed stream pointer.
+ // This is not specified in the man page but may crash on some system.
+ State = ensureStreamNonNull(Call.getArgSVal(Desc->StreamArgI), C, State);
+ if (!State)
+ return;
+
+ C.addTransition(State);
+}
+
+void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef State = C.getState();
SValBuilder &SVB = C.getSValBuilder();
const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
@@ -170,7 +214,7 @@
C.addTransition(StateNull);
}
-void StreamChecker::evalFreopen(const CallEvent &Call,
+void StreamChecker::evalFreopen(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
@@ -181,14 +225,9 @@
Optional<DefinedSVal> StreamVal = Call.getArgSVal(2).getAs<DefinedSVal>();
if (!StreamVal)
return;
- // Do not allow NULL as passed stream pointer.
- // This is not specified in the man page but may crash on some system.
- State = checkNullStream(*StreamVal, C, State);
- if (!State)
- return;
SymbolRef StreamSym = StreamVal->getAsSymbol();
- // Do not care about special values for stream ("(FILE *)0x12345"?).
+ // Do not care about concrete values for stream ("(FILE *)0x12345"?).
if (!StreamSym)
return;
@@ -212,49 +251,34 @@
C.addTransition(StateRetNull);
}
-void StreamChecker::evalFclose(const CallEvent &Call, CheckerContext &C) const {
+void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef State = C.getState();
- State = checkDoubleClose(Call, C, State);
- if (State)
- C.addTransition(State);
-}
-
-void StreamChecker::evalFseek(const CallEvent &Call, CheckerContext &C) const {
- const Expr *AE2 = Call.getArgExpr(2);
- if (!AE2)
+ SymbolRef Sym = Call.getArgSVal(Desc->StreamArgI).getAsSymbol();
+ if (!Sym)
return;
- ProgramStateRef State = C.getState();
-
- State = checkNullStream(Call.getArgSVal(0), C, State);
- if (!State)
+ const StreamState *SS = State->get<StreamMap>(Sym);
+ if (!SS)
return;
- State =
- checkFseekWhence(State->getSVal(AE2, C.getLocationContext()), C, State);
- if (!State)
- return;
+ // Close the File Descriptor.
+ // Regardless if the close fails or not, stream becomes "closed"
+ // and can not be used any more.
+ State = State->set<StreamMap>(Sym, StreamState::getClosed());
C.addTransition(State);
}
-void StreamChecker::checkArgNullStream(const CallEvent &Call, CheckerContext &C,
- unsigned ArgI) const {
- ProgramStateRef State = C.getState();
- State = checkNullStream(Call.getArgSVal(ArgI), C, State);
- if (State)
- C.addTransition(State);
-}
-ProgramStateRef StreamChecker::checkNullStream(SVal SV, CheckerContext &C,
- ProgramStateRef State) const {
- Optional<DefinedSVal> DV = SV.getAs<DefinedSVal>();
- if (!DV)
+ProgramStateRef StreamChecker::ensureStreamNonNull(SVal StreamVal, CheckerContext &C, ProgramStateRef State) const {
+ auto Stream = StreamVal.getAs<DefinedSVal>();
+ if (!Stream)
return State;
ConstraintManager &CM = C.getConstraintManager();
+
ProgramStateRef StateNotNull, StateNull;
- std::tie(StateNotNull, StateNull) = CM.assumeDual(C.getState(), *DV);
+ std::tie(StateNotNull, StateNull) = CM.assumeDual(C.getState(), *Stream);
if (!StateNotNull && StateNull) {
if (ExplodedNode *N = C.generateErrorNode(StateNull)) {
@@ -271,9 +295,8 @@
}
// Check the legality of the 'whence' argument of 'fseek'.
-ProgramStateRef StreamChecker::checkFseekWhence(SVal SV, CheckerContext &C,
- ProgramStateRef State) const {
- Optional<nonloc::ConcreteInt> CI = SV.getAs<nonloc::ConcreteInt>();
+ProgramStateRef StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C, ProgramStateRef State) const {
+ Optional<nonloc::ConcreteInt> CI = WhenceVal.getAs<nonloc::ConcreteInt>();
if (!CI)
return State;
@@ -295,38 +318,52 @@
return State;
}
-ProgramStateRef StreamChecker::checkDoubleClose(const CallEvent &Call,
+// Check:
+// Using a stream pointer after 'fclose' causes undefined behavior
+// according to cppreference.com .
+// Using a stream that has failed to open is likely to cause problems, but not
+// explicitly mentioned in documentation.
+ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
CheckerContext &C,
ProgramStateRef State) const {
- SymbolRef Sym = Call.getArgSVal(0).getAsSymbol();
+ SymbolRef Sym = StreamVal.getAsSymbol();
if (!Sym)
- return State;
+ return nullptr;
const StreamState *SS = State->get<StreamMap>(Sym);
-
- // If the file stream is not tracked, return.
if (!SS)
- return State;
+ return;
- // Check: Double close a File Descriptor could cause undefined behaviour.
- // Conforming to man-pages
if (SS->isClosed()) {
ExplodedNode *N = C.generateErrorNode();
if (N) {
- if (!BT_doubleclose)
- BT_doubleclose.reset(new BuiltinBug(
- this, "Double fclose", "Try to close a file Descriptor already"
- " closed. Cause undefined behaviour."));
+ if (!BT_UseAfterClose)
+ BT_UseAfterClose.reset(new BuiltinBug(
+ this, "Stream used after close", "File descriptor is used after it was closed."
+ "Cause undefined behaviour."));
C.emitReport(std::make_unique<PathSensitiveBugReport>(
- *BT_doubleclose, BT_doubleclose->getDescription(), N));
+ *BT_UseAfterClose, BT_UseAfterClose->getDescription(), N));
return nullptr;
}
-
return State;
}
- // Close the File Descriptor.
- State = State->set<StreamMap>(Sym, StreamState::getClosed());
+ if (SS->isOpenFailed()) {
+ // This should usually not occur because stream pointer is NULL.
+ // But freopen can cause a state when stream pointer remains non-null but failed to open.
+ ExplodedNode *N = C.generateErrorNode();
+ if (N) {
+ if (!BT_UseAfterOpenFailed)
+ BT_UseAfterOpenFailed.reset(new BuiltinBug(
+ this, "Stream used after failed open", "(Re-)Opening the file descriptor has failed."
+ "Using it can cause undefined behaviour."));
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ *BT_UseAfterOpenFailed, BT_UseAfterOpenFailed->getDescription(), N));
+ return nullptr;
+ }
+
+ return State;
+ }
return State;
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
