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.
According to documentations, after an `fclose` call any other stream
operations cause undefined behaviour, regardless if the close failed
or not.
This change adds the check for the opened state before all other
(applicable) operations.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75614
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
@@ -3,9 +3,9 @@
typedef __typeof__(sizeof(int)) size_t;
typedef __typeof__(sizeof(int)) fpos_t;
typedef struct _IO_FILE FILE;
-#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 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. */
extern FILE *fopen(const char *path, const char *mode);
extern FILE *tmpfile(void);
extern int fclose(FILE *fp);
@@ -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);
}
-}
+}
\ No newline at end of file
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; }
@@ -74,7 +74,7 @@
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;
@@ -89,7 +89,7 @@
{&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
{{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
{{"fclose", 1},
- {&StreamChecker::preFclose, &StreamChecker::evalFclose, 0}},
+ {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
{{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3}},
{{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3}},
{{"fseek", 3}, {&StreamChecker::preFseek, nullptr, 0}},
@@ -111,8 +111,6 @@
void evalFreopen(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
- void preFclose(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const;
void evalFclose(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
@@ -130,12 +128,11 @@
ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
ProgramStateRef State) const;
- /// Check that the stream is not closed.
- /// Return a state where the stream is guaranteed to not in closed state
- /// (if data about it exists).
- /// Generate error if the stream is provable in closed state.
- ProgramStateRef ensureStreamNotClosed(SVal StreamVal, CheckerContext &C,
- ProgramStateRef State) const;
+ /// Check that the stream is the 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;
/// Check the legality of the 'whence' argument of 'fseek'.
/// Generate error and return nullptr if it is found to be illegal.
@@ -266,16 +263,6 @@
C.addTransition(StateRetNull);
}
-void StreamChecker::preFclose(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const {
- ProgramStateRef State = C.getState();
- State = ensureStreamNotClosed(getStreamArg(Desc, Call), C, State);
- if (!State)
- return;
-
- C.addTransition(State);
-}
-
void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
@@ -300,6 +287,9 @@
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);
@@ -314,6 +304,9 @@
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;
@@ -346,9 +339,9 @@
return StateNotNull;
}
-ProgramStateRef
-StreamChecker::ensureStreamNotClosed(SVal StreamVal, CheckerContext &C,
- ProgramStateRef State) const {
+ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
+ CheckerContext &C,
+ ProgramStateRef State) const {
SymbolRef Sym = StreamVal.getAsSymbol();
if (!Sym)
return State;
@@ -357,23 +350,43 @@
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<PathSensitiveBugReport>(
- *BT_doubleclose, BT_doubleclose->getDescription(), N));
+ *BT_UseAfterClose, BT_UseAfterClose->getDescription(), N));
return nullptr;
}
return State;
}
+ 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<PathSensitiveBugReport>(
+ *BT_UseAfterOpenFailed, BT_UseAfterOpenFailed->getDescription(), N));
+ return nullptr;
+ }
+ return State;
+ }
+
return State;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
