[clang] ce1a862 - [Analyzer][StreamChecker] Check for opened stream before operations.
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 9 03:05:52 PDT 2020
Author: Balázs Kéri
Date: 2020-03-09T11:00:03+01:00
New Revision: ce1a86251bb42fbbc7cc21feb416ed7a0bf411b7
URL: https://github.com/llvm/llvm-project/commit/ce1a86251bb42fbbc7cc21feb416ed7a0bf411b7
DIFF: https://github.com/llvm/llvm-project/commit/ce1a86251bb42fbbc7cc21feb416ed7a0bf411b7.diff
LOG: [Analyzer][StreamChecker] Check for opened stream before operations.
Summary:
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.
Reviewers: Szelethus
Reviewed By: Szelethus
Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gamesh411, Charusso, martong, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D75614
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream.c
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 9d8d276bd701..7e5bf7c376f0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -34,7 +34,7 @@ struct StreamState {
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 @@ SVal getStreamArg(const FnDescription *Desc, const CallEvent &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;
@@ -88,7 +88,7 @@ class StreamChecker
{&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}},
@@ -110,8 +110,6 @@ class StreamChecker
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;
@@ -128,12 +126,11 @@ class StreamChecker
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.
@@ -263,16 +260,6 @@ void StreamChecker::evalFreopen(const FnDescription *Desc,
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();
@@ -297,6 +284,9 @@ void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
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);
@@ -311,6 +301,9 @@ void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call,
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;
@@ -343,9 +336,9 @@ StreamChecker::ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
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;
@@ -354,23 +347,43 @@ StreamChecker::ensureStreamNotClosed(SVal StreamVal, CheckerContext &C,
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;
}
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index e1db6780d90a..6a22e9ee722e 100644
--- a/clang/test/Analysis/stream.c
+++ b/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_seek(void) {
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 @@ void check_freopen_2() {
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 @@ void check_freopen_3() {
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
More information about the cfe-commits
mailing list