diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -34,8 +34,8 @@ bool isOpened() const { return K == Opened; } bool isClosed() const { return K == Closed; } - //bool isOpenFailed() const { return K == OpenFailed; } - //bool isEscaped() const { return K == Escaped; } + bool isOpenFailed() const { return K == OpenFailed; } + // bool isEscaped() const { return K == Escaped; } bool operator==(const StreamState &X) const { return K == X.K; } @@ -44,104 +44,183 @@ static StreamState getOpenFailed() { return StreamState(OpenFailed); } static StreamState getEscaped() { return StreamState(Escaped); } - void Profile(llvm::FoldingSetNodeID &ID) const { - ID.AddInteger(K); - } + void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); } }; class StreamChecker; +struct FnDescription; +using FnCheck = std::function; -using FnCheck = std::function; +using ArgNoTy = unsigned int; +static const ArgNoTy ArgNone = std::numeric_limits::max(); struct FnDescription { + FnCheck PreFn; FnCheck EvalFn; + ArgNoTy StreamArgNo; }; -class StreamChecker : public Checker { +/// Get the value of the stream argument out of the passed call event. +/// The call should contain a function that is described by Desc. +SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) { + assert(Desc && Desc->StreamArgNo != ArgNone && + "Try to get a non-existing stream argument."); + return Call.getArgSVal(Desc->StreamArgNo); +} + +class StreamChecker + : public Checker { mutable std::unique_ptr 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 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"}, {nullptr, &StreamChecker::evalFopen, ArgNone}}, + {{"freopen", 3}, + {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}}, + {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone}}, + {{"fclose", 1}, + {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}}, + {{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3}}, + {{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3}}, + {{"fseek", 3}, {&StreamChecker::preFseek, nullptr}}, + {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"rewind", 1}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"fgetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"fsetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"clearerr", 1}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"feof", 1}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"ferror", 1}, {&StreamChecker::preDefault, nullptr, 0}}, + {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 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; + + /// Check that the stream (in StreamVal) is not NULL. + /// If it can only be NULL a fatal error is emitted and nullptr returned. + /// Otherwise a new state where the stream is constrained to be non-null. + ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C, + ProgramStateRef State) const; + + /// Check the legality of the 'whence' argument of 'fseek'. + /// Generate error and return nullptr if it is found to be illegal. + /// Otherwise returns the state. + /// (State is not changed here because the "whence" value is already known.) + ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C, + ProgramStateRef State) const; + + /// Check that the stream is in opened state. + /// If the stream is known to be not opened an error is generated + /// and nullptr returned, otherwise the original state is returned. + ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C, + ProgramStateRef State) const; + + /// Find the description data of the function called by a call event. + /// Returns nullptr if no function is recognized. + 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(Call.getDecl()); + if (!FD || FD->getKind() != Decl::Function) + return nullptr; + + return FnDescriptions.lookup(Call); + } }; } // end anonymous namespace REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState) +void StreamChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + const FnDescription *Desc = lookupFn(Call); + if (!Desc || !Desc->PreFn) + return; + + Desc->PreFn(this, Desc, Call, C); +} bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const { - const auto *FD = dyn_cast_or_null(Call.getDecl()); - if (!FD || FD->getKind() != Decl::Function) + const FnDescription *Desc = lookupFn(Call); + if (!Desc || !Desc->EvalFn) 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; - } + Desc->EvalFn(this, Desc, Call, C); - const FnDescription *Description = FnDescriptions.lookup(Call); - if (!Description) - return false; + return C.isDifferent(); +} - (Description->EvalFn)(this, Call, C); +void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + SVal StreamVal = getStreamArg(Desc, Call); + State = ensureStreamNonNull(StreamVal, C, State); + if (!State) + return; + State = ensureStreamOpened(StreamVal, C, State); + if (!State) + return; - return C.isDifferent(); + C.addTransition(State); +} + +void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + SVal StreamVal = getStreamArg(Desc, Call); + State = ensureStreamNonNull(StreamVal, C, State); + if (!State) + return; + State = ensureStreamOpened(StreamVal, C, State); + if (!State) + return; + State = ensureFseekWhenceCorrect(Call.getArgSVal(2), C, State); + if (!State) + return; + + C.addTransition(State); } -void StreamChecker::evalFopen(const CallEvent &Call, CheckerContext &C) const { +void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call, + CheckerContext &C) const { + // Do not allow NULL as passed stream pointer but allow a closed stream. + ProgramStateRef State = C.getState(); + State = ensureStreamNonNull(getStreamArg(Desc, Call), 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 +249,8 @@ 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(); @@ -178,17 +258,14 @@ if (!CE) return; - Optional StreamVal = Call.getArgSVal(2).getAs(); + Optional StreamVal = + getStreamArg(Desc, Call).getAs(); 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"?). + // FIXME: Are stdin, stdout, stderr such values? if (!StreamSym) return; @@ -212,49 +289,36 @@ 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->StreamArgNo).getAsSymbol(); + if (!Sym) return; - ProgramStateRef State = C.getState(); - - State = checkNullStream(Call.getArgSVal(0), C, State); - if (!State) + const StreamState *SS = State->get(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(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 DV = SV.getAs(); - if (!DV) +ProgramStateRef +StreamChecker::ensureStreamNonNull(SVal StreamVal, CheckerContext &C, + ProgramStateRef State) const { + auto Stream = StreamVal.getAs(); + 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)) { @@ -270,10 +334,10 @@ return StateNotNull; } -// Check the legality of the 'whence' argument of 'fseek'. -ProgramStateRef StreamChecker::checkFseekWhence(SVal SV, CheckerContext &C, - ProgramStateRef State) const { - Optional CI = SV.getAs(); +ProgramStateRef +StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C, + ProgramStateRef State) const { + Optional CI = WhenceVal.getAs(); if (!CI) return State; @@ -295,38 +359,54 @@ return State; } -ProgramStateRef StreamChecker::checkDoubleClose(const CallEvent &Call, - CheckerContext &C, - ProgramStateRef State) const { - SymbolRef Sym = Call.getArgSVal(0).getAsSymbol(); +ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal, + CheckerContext &C, + ProgramStateRef State) const { + SymbolRef Sym = StreamVal.getAsSymbol(); if (!Sym) return State; const StreamState *SS = State->get(Sym); - - // If the file stream is not tracked, return. if (!SS) return State; - // Check: Double close a File Descriptor could cause undefined behaviour. - // Conforming to man-pages if (SS->isClosed()) { + // Using a stream pointer after 'fclose' causes undefined behavior + // according to cppreference.com . 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, "Closed stream", + "Stream might be already closed. " + "Causes undefined behaviour.")); C.emitReport(std::make_unique( - *BT_doubleclose, BT_doubleclose->getDescription(), N)); + *BT_UseAfterClose, BT_UseAfterClose->getDescription(), N)); return nullptr; } - return State; } - // Close the File Descriptor. - State = State->set(Sym, StreamState::getClosed()); + if (SS->isOpenFailed()) { + // Using a stream that has failed to open is likely to cause problems. + // 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, "Invalid stream", + "Stream might be invalid after " + "(re-)opening it has failed. " + "Can cause undefined behaviour.")); + C.emitReport(std::make_unique( + *BT_UseAfterOpenFailed, BT_UseAfterOpenFailed->getDescription(), N)); + return nullptr; + } + + return State; + } return State; } @@ -337,7 +417,7 @@ // TODO: Clean up the state. const StreamMapTy &Map = State->get(); - for (const auto &I: Map) { + for (const auto &I : Map) { SymbolRef Sym = I.first; const StreamState &SS = I.second; if (!SymReaper.isDead(Sym) || !SS.isOpened()) @@ -360,6 +440,4 @@ mgr.registerChecker(); } -bool ento::shouldRegisterStreamChecker(const LangOptions &LO) { - return true; -} +bool ento::shouldRegisterStreamChecker(const LangOptions &LO) { return true; } diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -108,19 +108,56 @@ void f_double_close(void) { FILE *p = fopen("foo", "r"); - fclose(p); - fclose(p); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}} + if (!p) + return; + fclose(p); + fclose(p); // expected-warning {{Stream might be already closed}} } void f_double_close_alias(void) { FILE *p1 = fopen("foo", "r"); + if (!p1) + return; FILE *p2 = p1; fclose(p1); - fclose(p2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}} + fclose(p2); // expected-warning {{Stream might be already closed}} +} + +void f_use_after_close(void) { + FILE *p = fopen("foo", "r"); + if (!p) + return; + fclose(p); + clearerr(p); // expected-warning {{Stream might be already closed}} +} + +void f_open_after_close(void) { + FILE *p = fopen("foo", "r"); + if (!p) + return; + fclose(p); + p = fopen("foo", "r"); + if (!p) + return; + fclose(p); +} + +void f_reopen_after_close(void) { + FILE *p = fopen("foo", "r"); + if (!p) + return; + fclose(p); + // Allow reopen after close. + p = freopen("foo", "w", p); + if (!p) + return; + fclose(p); } void f_leak(int c) { FILE *p = fopen("foo.c", "r"); + if (!p) + return; if(c) return; // expected-warning {{Opened File never closed. Potential Resource leak}} fclose(p); @@ -155,13 +192,13 @@ if (f2) { // Check if f1 and f2 point to the same stream. fclose(f1); - fclose(f2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}} + fclose(f2); // expected-warning {{Stream might be already closed.}} } else { // Reopen failed. - // f1 points now to a possibly invalid stream but this condition is currently not checked. - // f2 is NULL. - rewind(f1); - rewind(f2); // expected-warning {{Stream pointer might be NULL}} + // f1 is non-NULL but points to a possibly invalid stream. + rewind(f1); // expected-warning {{Stream might be invalid}} + // f2 is NULL but the previous error stops the checker. + rewind(f2); } } } @@ -170,9 +207,9 @@ FILE *f1 = fopen("foo.c", "r"); if (f1) { // Unchecked result of freopen. - // The f1 may be invalid after this call (not checked by the checker). + // The f1 may be invalid after this call. freopen(0, "w", f1); - rewind(f1); + rewind(f1); // expected-warning {{Stream might be invalid}} fclose(f1); } }