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.
A warning is produced if the `getc` call fails (returns EOF) and
the next called stream function is not `feof` and `ferror`
(both are needed).
This check is inspired by CERT rule FIO34-C "Distinguish between characters
read from a file and EOF or WEOF".
https://wiki.sei.cmu.edu/confluence/display/c/FIO34-C.+Distinguish+between+characters+read+from+a+file+and+EOF+or+WEOF
Other similar functions (and wide versions) are to be added.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75045
Files:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream.c
Index: clang/test/Analysis/stream.c
===================================================================
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -6,11 +6,13 @@
#define SEEK_SET 0 /* Seek from beginning of file. */
#define SEEK_CUR 1 /* Seek from current position. */
#define SEEK_END 2 /* Seek from end of file. */
+#define EOF (-1)
extern FILE *fopen(const char *path, const char *mode);
extern FILE *tmpfile(void);
extern int fclose(FILE *fp);
extern size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
extern size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
+extern int fgetc(FILE *stream);
extern int fseek (FILE *__stream, long int __off, int __whence);
extern long int ftell (FILE *__stream);
extern void rewind (FILE *__stream);
@@ -176,3 +178,74 @@
fclose(f1);
}
}
+
+void check_fgetc_error() {
+ FILE *fp = fopen("foo.c", "r");
+ if (fp) {
+ int C;
+ fpos_t pos;
+ C = fgetc(fp);
+ fread(0, 0, 0, fp); // expected-warning {{Should call}}
+ fread(0, 0, 0, fp);
+ C = fgetc(fp);
+ fwrite(0, 0, 0, fp); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ fseek(fp, 1, SEEK_SET); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ ftell(fp); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ rewind(fp); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ fgetpos(fp, &pos); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ fsetpos(fp, 0); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ clearerr(fp); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ C = fgetc(fp); // expected-warning {{Should call}}
+ fp = freopen(0, "w", fp); // expected-warning {{Should call}}
+ C = fgetc(fp);
+ fclose(fp); // expected-warning {{Should call}}
+ }
+}
+
+void check_fgetc_error1() {
+ FILE *fp = fopen("foo.c", "r");
+ if (fp) {
+ int C;
+ C = fgetc(fp);
+ feof(fp);
+ fclose(fp); // expected-warning {{Should call}}
+ }
+ fp = fopen("foo.c", "r");
+ if (fp) {
+ int C;
+ C = fgetc(fp);
+ ferror(fp);
+ fclose(fp); // expected-warning {{Should call}}
+ }
+}
+
+void check_fgetc_noerror() {
+ FILE *fp = fopen("foo.c", "r");
+ if (fp) {
+ int C;
+ C = fgetc(fp);
+ feof(fp);
+ ferror(fp);
+ fclose(fp);
+ }
+}
+
+void check_fgetc_checked() {
+ FILE *fp = fopen("foo.c", "r");
+ if (fp) {
+ int C;
+ C = fgetc(fp);
+ if (C == EOF) {
+ feof(fp);
+ ferror(fp);
+ }
+ fclose(fp);
+ }
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -16,6 +16,7 @@
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
@@ -29,15 +30,23 @@
struct StreamState {
enum Kind { Opened, Closed, OpenFailed, Escaped } K;
+ bool ShouldCallFeof{false};
+ bool ShouldCallFerror{false};
- StreamState(Kind k) : K(k) {}
+ StreamState(Kind k, bool Feof, bool Ferror)
+ : K(k), ShouldCallFeof{Feof}, ShouldCallFerror{Ferror} {}
+ StreamState(Kind k, bool MakeError = false)
+ : K(k), ShouldCallFeof{MakeError}, ShouldCallFerror{MakeError} {}
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 operator==(const StreamState &X) const { return K == X.K; }
+ bool operator==(const StreamState &X) const {
+ return K == X.K && ShouldCallFeof == X.ShouldCallFeof &&
+ ShouldCallFerror == X.ShouldCallFerror;
+ }
static StreamState getOpened() { return StreamState(Opened); }
static StreamState getClosed() { return StreamState(Closed); }
@@ -46,13 +55,17 @@
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
+ ID.AddBoolean(ShouldCallFeof);
+ ID.AddBoolean(ShouldCallFerror);
}
};
class StreamChecker : public Checker<eval::Call,
check::DeadSymbols > {
mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
- BT_doubleclose, BT_ResourceLeak;
+ BT_doubleclose, BT_ResourceLeak, BT_missingerrorcheck;
+
+ mutable int EOFv{0};
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
@@ -67,36 +80,35 @@
{{"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)},
+ {{"fread", 4}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 3)},
+ {{"fwrite", 4}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 3)},
+ {{"fgetc", 1}, &StreamChecker::evalGetc},
{{"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)},
+ {{"ftell", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+ {{"rewind", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+ {{"fgetpos", 2}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+ {{"fsetpos", 2}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+ {{"clearerr", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+ {{"feof", 1}, &StreamChecker::evalFeof},
+ {{"ferror", 1}, &StreamChecker::evalFerror},
+ {{"fileno", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 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;
-
+ void evalGetc(const CallEvent &Call, CheckerContext &C) const;
+ void evalFeof(const CallEvent &Call, CheckerContext &C) const;
+ void evalFerror(const CallEvent &Call, CheckerContext &C) const;
+ void evalDefault(const CallEvent &Call, CheckerContext &C,
+ unsigned StreamArgI) const;
+
+ ProgramStateRef resetErrorState(SVal SV, CheckerContext &C,
+ ProgramStateRef State, bool FeofCalled,
+ bool FerrorCalled) const;
+ ExplodedNode *checkErrorState(SVal SV, CheckerContext &C,
+ ExplodedNode *N) const;
ProgramStateRef checkNullStream(SVal SV, CheckerContext &C,
ProgramStateRef State) const;
ProgramStateRef checkFseekWhence(SVal SV, CheckerContext &C,
@@ -185,6 +197,10 @@
if (!StreamSym)
return;
+ ExplodedNode *N = C.addTransition(State);
+ N = checkErrorState(*StreamVal, C, N);
+ State = N->getState();
+
// Generate state for non-failed case.
// Return value is the passed stream pointer.
// According to the documentations, the stream is closed first
@@ -201,23 +217,27 @@
StateRetNull =
StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed());
- C.addTransition(StateRetNotNull);
- C.addTransition(StateRetNull);
+ C.addTransition(StateRetNotNull, N);
+ C.addTransition(StateRetNull, N);
}
void StreamChecker::evalFclose(const CallEvent &Call, CheckerContext &C) const {
- ProgramStateRef State = C.getState();
+ ExplodedNode *N = checkErrorState(Call.getArgSVal(0), C, C.getPredecessor());
+
+ ProgramStateRef State = N->getState();
State = checkDoubleClose(Call, C, State);
if (State)
- C.addTransition(State);
+ C.addTransition(State, N);
}
void StreamChecker::evalFseek(const CallEvent &Call, CheckerContext &C) const {
+ ExplodedNode *N = checkErrorState(Call.getArgSVal(0), C, C.getPredecessor());
+
const Expr *AE2 = Call.getArgExpr(2);
if (!AE2)
return;
- ProgramStateRef State = C.getState();
+ ProgramStateRef State = N->getState();
State = checkNullStream(Call.getArgSVal(0), C, State);
if (!State)
@@ -228,15 +248,144 @@
if (!State)
return;
+ C.addTransition(State, N);
+}
+
+void StreamChecker::evalGetc(const CallEvent &Call, CheckerContext &C) const {
+ if (EOFv == 0) {
+ if (const llvm::Optional<int> OptInt =
+ tryExpandAsInteger("EOF", C.getPreprocessor()))
+ EOFv = *OptInt;
+ else
+ EOFv = -1;
+ }
+
+ ExplodedNode *N = checkErrorState(Call.getArgSVal(0), C, C.getPredecessor());
+
+ auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+ if (!CE)
+ return;
+
+ SymbolRef StreamSym = Call.getArgSVal(0).getAsSymbol();
+ if (!StreamSym)
+ return;
+
+ ProgramStateRef State = N->getState();
+ State = checkNullStream(Call.getArgSVal(0), C, State);
+ if (!State)
+ return;
+
+ const StreamState *SS = State->get<StreamMap>(StreamSym);
+ if (!SS)
+ return;
+
+ SValBuilder &SVB = C.getSValBuilder();
+ BasicValueFactory &BVF = SVB.getBasicValueFactory();
+
+ DefinedSVal RetVal =
+ SVB.conjureSymbolVal(nullptr, CE, C.getLocationContext(), C.blockCount())
+ .castAs<DefinedSVal>();
+ State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+
+ ProgramStateRef FailedState, NonFailedState;
+ const llvm::APSInt &EOFVal = BVF.getValue(EOFv, Call.getResultType());
+ std::tie(FailedState, NonFailedState) =
+ State->assumeInclusiveRange(RetVal, EOFVal, EOFVal);
+ if (FailedState) {
+ FailedState =
+ FailedState->set<StreamMap>(StreamSym, StreamState{SS->K, true});
+ C.addTransition(FailedState, N);
+ }
+ if (NonFailedState) {
+ C.addTransition(NonFailedState, N);
+ }
+}
+
+void StreamChecker::evalFeof(const CallEvent &Call, CheckerContext &C) const {
+ SVal StreamVal = Call.getArgSVal(0);
+ ProgramStateRef State = C.getState();
+ State = checkNullStream(StreamVal, C, State);
+ if (!State)
+ return;
+
+ State = resetErrorState(StreamVal, C, State, true, false);
+
C.addTransition(State);
}
-void StreamChecker::checkArgNullStream(const CallEvent &Call, CheckerContext &C,
- unsigned ArgI) const {
+void StreamChecker::evalFerror(const CallEvent &Call, CheckerContext &C) const {
+ SVal StreamVal = Call.getArgSVal(0);
ProgramStateRef State = C.getState();
- State = checkNullStream(Call.getArgSVal(ArgI), C, State);
- if (State)
- C.addTransition(State);
+ State = checkNullStream(StreamVal, C, State);
+ if (!State)
+ return;
+
+ State = resetErrorState(StreamVal, C, State, false, true);
+
+ C.addTransition(State);
+}
+
+void StreamChecker::evalDefault(const CallEvent &Call, CheckerContext &C,
+ unsigned StreamArgI) const {
+ ExplodedNode *N =
+ checkErrorState(Call.getArgSVal(StreamArgI), C, C.getPredecessor());
+ ProgramStateRef State = N->getState();
+ State = checkNullStream(Call.getArgSVal(StreamArgI), C, State);
+ if (!State)
+ return;
+
+ C.addTransition(State, N);
+}
+
+ProgramStateRef StreamChecker::resetErrorState(SVal SV, CheckerContext &C,
+ ProgramStateRef State,
+ bool FeofCalled,
+ bool FerrorCalled) const {
+ SymbolRef StreamSym = SV.getAsSymbol();
+ if (!StreamSym)
+ return State;
+ const StreamState *SS = State->get<StreamMap>(StreamSym);
+ if (!SS || !SS->isOpened())
+ return State;
+ if ((!SS->ShouldCallFeof || !FeofCalled) &&
+ (!SS->ShouldCallFerror || !FerrorCalled))
+ return State;
+
+ if (FeofCalled) {
+ State = State->set<StreamMap>(
+ StreamSym, StreamState{SS->K, false, SS->ShouldCallFerror});
+ }
+ if (FerrorCalled) {
+ State = State->set<StreamMap>(
+ StreamSym, StreamState{SS->K, SS->ShouldCallFeof, false});
+ }
+
+ return State;
+}
+
+ExplodedNode *StreamChecker::checkErrorState(SVal SV, CheckerContext &C,
+ ExplodedNode *N) const {
+ SymbolRef StreamSym = SV.getAsSymbol();
+ if (!StreamSym)
+ return N;
+ ProgramStateRef State = N->getState();
+ const StreamState *SS = State->get<StreamMap>(StreamSym);
+ if (!SS || !SS->isOpened())
+ return N;
+
+ if (SS->ShouldCallFeof || SS->ShouldCallFerror) {
+ State = State->set<StreamMap>(StreamSym, StreamState{SS->K, false});
+ if (ExplodedNode *N1 = C.generateNonFatalErrorNode(State)) {
+ if (!BT_missingerrorcheck)
+ BT_missingerrorcheck.reset(new BuiltinBug(
+ this, "Missing error check",
+ "Should call 'feof' and 'ferror' after failed character read."));
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ *BT_missingerrorcheck, BT_missingerrorcheck->getDescription(), N1));
+ return N1;
+ }
+ }
+ return N;
}
ProgramStateRef StreamChecker::checkNullStream(SVal SV, CheckerContext &C,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
