Hi,
I wrote a patch for Clang Static Analyzer's StreamChecker. This patch:
1. adds a check for descriptor access (read/write) after it being closed
2. adds support of directory operations (opendir/closedir and some other)
3. fixes issue in double close check: descriptor that was not tracked
before was not marked as closed while calling a close function
4. adds fprintf and fscanf functions to track.
Is it OK to commit or have I done something wrong?
--
Best regards,
Alexey Sidorin,
Programming Tools Part, CSG, SMRC, Samsung Electronics
Index: StreamChecker.cpp
===================================================================
--- StreamChecker.cpp (revision 182465)
+++ StreamChecker.cpp (working copy)
@@ -56,20 +56,27 @@
}
};
+// TODO: add open/close functions to track.
class StreamChecker : public Checker<eval::Call,
check::DeadSymbols > {
mutable IdentifierInfo *II_fopen, *II_tmpfile, *II_fclose, *II_fread,
- *II_fwrite,
- *II_fseek, *II_ftell, *II_rewind, *II_fgetpos, *II_fsetpos,
- *II_clearerr, *II_feof, *II_ferror, *II_fileno;
+ *II_fwrite, *II_fscanf, *II_fprintf, *II_fseek, *II_ftell,
+ *II_rewind, *II_fgetpos, *II_fsetpos, *II_clearerr, *II_feof,
+ *II_ferror, *II_fileno, *II_opendir, *II_closedir,
+ *II_fdopendir, *II_scandir, *II_seekdir, *II_readdir,
+ *II_dirfd, *II_rewinddir, *II_telldir;
mutable OwningPtr<BuiltinBug> BT_nullfp, BT_illegalwhence,
- BT_doubleclose, BT_ResourceLeak;
+ BT_doubleclose, BT_ResourceLeak,
+ BT_accessAfterClose;
public:
StreamChecker()
- : II_fopen(0), II_tmpfile(0) ,II_fclose(0), II_fread(0), II_fwrite(0),
- II_fseek(0), II_ftell(0), II_rewind(0), II_fgetpos(0), II_fsetpos(0),
- II_clearerr(0), II_feof(0), II_ferror(0), II_fileno(0) {}
+ : II_fopen(0), II_tmpfile(0) ,II_fclose(0), II_fread(0), II_fwrite(0),
+ II_fscanf(0), II_fprintf(0), II_fseek(0), II_ftell(0), II_rewind(0),
+ II_fgetpos(0), II_fsetpos(0), II_clearerr(0), II_feof(0),
+ II_ferror(0), II_fileno(0), II_opendir(0), II_closedir(0),
+ II_fdopendir(0), II_scandir(0), II_seekdir(0), II_readdir(0),
+ II_dirfd(0), II_rewinddir(0), II_telldir(0) {}
bool evalCall(const CallExpr *CE, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
@@ -77,12 +84,23 @@
private:
void Fopen(CheckerContext &C, const CallExpr *CE) const;
void Tmpfile(CheckerContext &C, const CallExpr *CE) const;
+ void Opendir(CheckerContext &C, const CallExpr *CE) const;
+ void Fdopendir(CheckerContext &C, const CallExpr *CE) const;
void Fclose(CheckerContext &C, const CallExpr *CE) const;
+ void Closedir(CheckerContext &C, const CallExpr *CE) const;
void Fread(CheckerContext &C, const CallExpr *CE) const;
+ void Dirfd(CheckerContext &C, const CallExpr *CE) const;
+ void Scandir(CheckerContext &C, const CallExpr *CE) const;
+ void Fscanf(CheckerContext &C, const CallExpr *CE) const;
+ void Readdir(CheckerContext &C, const CallExpr *CE) const;
void Fwrite(CheckerContext &C, const CallExpr *CE) const;
+ void Fprintf(CheckerContext &C, const CallExpr *CE) const;
void Fseek(CheckerContext &C, const CallExpr *CE) const;
+ void Seekdir(CheckerContext &C, const CallExpr *CE) const;
void Ftell(CheckerContext &C, const CallExpr *CE) const;
+ void Telldir(CheckerContext &C, const CallExpr *CE) const;
void Rewind(CheckerContext &C, const CallExpr *CE) const;
+ void Rewinddir(CheckerContext &C, const CallExpr *CE) const;
void Fgetpos(CheckerContext &C, const CallExpr *CE) const;
void Fsetpos(CheckerContext &C, const CallExpr *CE) const;
void Clearerr(CheckerContext &C, const CallExpr *CE) const;
@@ -91,11 +109,14 @@
void Fileno(CheckerContext &C, const CallExpr *CE) const;
void OpenFileAux(CheckerContext &C, const CallExpr *CE) const;
-
+ void AccessAux(CheckerContext &C, const CallExpr *CE, int Argno) const;
+
ProgramStateRef CheckNullStream(SVal SV, ProgramStateRef state,
CheckerContext &C) const;
ProgramStateRef CheckDoubleClose(const CallExpr *CE, ProgramStateRef state,
CheckerContext &C) const;
+ bool CheckAlreadyClosed(SVal SV, ProgramStateRef state,
+ CheckerContext &C) const;
};
} // end anonymous namespace
@@ -111,20 +132,42 @@
ASTContext &Ctx = C.getASTContext();
if (!II_fopen)
II_fopen = &Ctx.Idents.get("fopen");
+ if (!II_opendir)
+ II_opendir = &Ctx.Idents.get("opendir");
+ if (!II_fdopendir)
+ II_fdopendir = &Ctx.Idents.get("fdopendir");
if (!II_tmpfile)
II_tmpfile = &Ctx.Idents.get("tmpfile");
if (!II_fclose)
II_fclose = &Ctx.Idents.get("fclose");
+ if (!II_closedir)
+ II_closedir = &Ctx.Idents.get("closedir");
if (!II_fread)
II_fread = &Ctx.Idents.get("fread");
+ if (!II_readdir)
+ II_readdir = &Ctx.Idents.get("readdir");
+ if (!II_scandir)
+ II_scandir = &Ctx.Idents.get("scandir");
+ if (!II_dirfd)
+ II_dirfd = &Ctx.Idents.get("dirfd");
if (!II_fwrite)
II_fwrite = &Ctx.Idents.get("fwrite");
+ if (!II_fscanf)
+ II_fscanf = &Ctx.Idents.get("fscanf");
+ if (!II_fprintf)
+ II_fprintf = &Ctx.Idents.get("fprintf");
if (!II_fseek)
II_fseek = &Ctx.Idents.get("fseek");
+ if (!II_seekdir)
+ II_seekdir = &Ctx.Idents.get("seekdir");
if (!II_ftell)
II_ftell = &Ctx.Idents.get("ftell");
+ if (!II_telldir)
+ II_telldir = &Ctx.Idents.get("telldir");
if (!II_rewind)
II_rewind = &Ctx.Idents.get("rewind");
+ if (!II_rewinddir)
+ II_rewinddir = &Ctx.Idents.get("rewinddir");
if (!II_fgetpos)
II_fgetpos = &Ctx.Idents.get("fgetpos");
if (!II_fsetpos)
@@ -142,6 +185,14 @@
Fopen(C, CE);
return true;
}
+ if (FD->getIdentifier() == II_fdopendir) {
+ Fdopendir(C, CE);
+ return true;
+ }
+ if (FD->getIdentifier() == II_opendir) {
+ Opendir(C, CE);
+ return true;
+ }
if (FD->getIdentifier() == II_tmpfile) {
Tmpfile(C, CE);
return true;
@@ -150,26 +201,62 @@
Fclose(C, CE);
return true;
}
+ if (FD->getIdentifier() == II_closedir) {
+ Closedir(C, CE);
+ return true;
+ }
if (FD->getIdentifier() == II_fread) {
Fread(C, CE);
return true;
}
+ if (FD->getIdentifier() == II_fscanf) {
+ Fscanf(C, CE);
+ return true;
+ }
+ if (FD->getIdentifier() == II_readdir) {
+ Readdir(C, CE);
+ return true;
+ }
+ if (FD->getIdentifier() == II_dirfd) {
+ Dirfd(C, CE);
+ return true;
+ }
+ if (FD->getIdentifier() == II_scandir) {
+ Scandir(C, CE);
+ return true;
+ }
if (FD->getIdentifier() == II_fwrite) {
Fwrite(C, CE);
return true;
}
+ if (FD->getIdentifier() == II_fprintf) {
+ Fprintf(C, CE);
+ return true;
+ }
if (FD->getIdentifier() == II_fseek) {
Fseek(C, CE);
return true;
}
+ if (FD->getIdentifier() == II_seekdir) {
+ Seekdir(C, CE);
+ return true;
+ }
if (FD->getIdentifier() == II_ftell) {
Ftell(C, CE);
return true;
}
+ if (FD->getIdentifier() == II_telldir) {
+ Telldir(C, CE);
+ return true;
+ }
if (FD->getIdentifier() == II_rewind) {
Rewind(C, CE);
return true;
}
+ if (FD->getIdentifier() == II_rewinddir) {
+ Rewinddir(C, CE);
+ return true;
+ }
if (FD->getIdentifier() == II_fgetpos) {
Fgetpos(C, CE);
return true;
@@ -202,6 +289,14 @@
OpenFileAux(C, CE);
}
+void StreamChecker::Fdopendir(CheckerContext &C, const CallExpr *CE) const {
+ OpenFileAux(C, CE);
+}
+
+void StreamChecker::Opendir(CheckerContext &C, const CallExpr *CE) const {
+ OpenFileAux(C, CE);
+}
+
void StreamChecker::Tmpfile(CheckerContext &C, const CallExpr *CE) const {
OpenFileAux(C, CE);
}
@@ -232,31 +327,64 @@
}
}
+void StreamChecker::AccessAux(CheckerContext &C, const CallExpr *CE,
+ int Argno) const {
+ ProgramStateRef state = C.getState();
+ SVal SV = state->getSVal(CE->getArg(Argno), C.getLocationContext());
+ if (!CheckNullStream(SV, state, C))
+ return;
+ CheckAlreadyClosed(SV, state, C);
+}
+
void StreamChecker::Fclose(CheckerContext &C, const CallExpr *CE) const {
ProgramStateRef state = CheckDoubleClose(CE, C.getState(), C);
if (state)
C.addTransition(state);
}
+void StreamChecker::Closedir(CheckerContext &C, const CallExpr *CE) const {
+ ProgramStateRef state = CheckDoubleClose(CE, C.getState(), C);
+ if (state)
+ C.addTransition(state);
+}
+
void StreamChecker::Fread(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(3), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 3);
}
void StreamChecker::Fwrite(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(3), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 3);
}
+void StreamChecker::Fscanf(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
+void StreamChecker::Fprintf(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
+void StreamChecker::Scandir(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
+void StreamChecker::Readdir(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
+void StreamChecker::Dirfd(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
void StreamChecker::Fseek(CheckerContext &C, const CallExpr *CE) const {
ProgramStateRef state = C.getState();
- if (!(state = CheckNullStream(state->getSVal(CE->getArg(0),
- C.getLocationContext()), state, C)))
+ SVal SV = state->getSVal(CE->getArg(0), C.getLocationContext());
+ if (!(state = CheckNullStream(SV, state, C)))
return;
+
+ if (CheckAlreadyClosed(SV, state, C))
+ return;
+
// Check the legality of the 'whence' argument of 'fseek'.
SVal Whence = state->getSVal(CE->getArg(2), C.getLocationContext());
Optional<nonloc::ConcreteInt> CI = Whence.getAs<nonloc::ConcreteInt>();
@@ -279,60 +407,48 @@
}
}
+void StreamChecker::Seekdir(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
void StreamChecker::Ftell(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
+void StreamChecker::Telldir(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
void StreamChecker::Rewind(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
+void StreamChecker::Rewinddir(CheckerContext &C, const CallExpr *CE) const {
+ AccessAux(C, CE, 0);
+}
+
void StreamChecker::Fgetpos(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
void StreamChecker::Fsetpos(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
void StreamChecker::Clearerr(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
void StreamChecker::Feof(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
void StreamChecker::Ferror(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
void StreamChecker::Fileno(CheckerContext &C, const CallExpr *CE) const {
- ProgramStateRef state = C.getState();
- if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()),
- state, C))
- return;
+ AccessAux(C, CE, 0);
}
ProgramStateRef StreamChecker::CheckNullStream(SVal SV, ProgramStateRef state,
@@ -365,21 +481,17 @@
state->getSVal(CE->getArg(0), C.getLocationContext()).getAsSymbol();
if (!Sym)
return state;
-
+
const StreamState *SS = state->get<StreamMap>(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()) {
+ if (SS && SS->isClosed()) {
ExplodedNode *N = C.generateSink();
if (N) {
if (!BT_doubleclose)
- BT_doubleclose.reset(new BuiltinBug("Double fclose",
- "Try to close a file Descriptor already"
+ BT_doubleclose.reset(new BuiltinBug("Double close",
+ "Try to close a descriptor already"
" closed. Cause undefined behaviour."));
BugReport *R = new BugReport(*BT_doubleclose,
BT_doubleclose->getDescription(), N);
@@ -389,9 +501,40 @@
}
// Close the File Descriptor.
+ // If it was not tracked before but gets closed we think it is closed too.
return state->set<StreamMap>(Sym, StreamState::getClosed(CE));
}
+bool StreamChecker::CheckAlreadyClosed(SVal SV, ProgramStateRef state,
+ CheckerContext &C) const {
+ SymbolRef Sym = SV.getAsSymbol();
+ if (!Sym)
+ return false;
+
+ const StreamState *SS = state->get<StreamMap>(Sym);
+
+ // If the file stream is not tracked, return.
+ if (!SS)
+ return false;
+
+ // Check: Access a File Descriptor after it being closed could cause
+ // undefined behaviour.
+ if (SS->isClosed()) {
+ ExplodedNode *N = C.generateSink();
+ if (N) {
+ if (!BT_accessAfterClose)
+ BT_accessAfterClose.reset(new BuiltinBug("Access after closing descriptor",
+ "Try to access a descriptor already"
+ " closed. Causes undefined behaviour."));
+ BugReport *R = new BugReport(*BT_accessAfterClose,
+ BT_accessAfterClose->getDescription(), N);
+ C.emitReport(R);
+ }
+ return true;
+ }
+ return false;
+}
+
void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
CheckerContext &C) const {
// TODO: Clean up the state.
@@ -408,7 +551,7 @@
if (N) {
if (!BT_ResourceLeak)
BT_ResourceLeak.reset(new BuiltinBug("Resource Leak",
- "Opened File never closed. Potential Resource leak."));
+ "Opened descriptor never closed. Potential Resource leak."));
BugReport *R = new BugReport(*BT_ResourceLeak,
BT_ResourceLeak->getDescription(), N);
C.emitReport(R);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
