balazske updated this revision to Diff 364730.
balazske added a comment.
Using "joined" note tag messages to have bugtype independent note tag functions.
New note tags at `ferror` and `feof`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106262/new/
https://reviews.llvm.org/D106262
Files:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream-note.c
Index: clang/test/Analysis/stream-note.c
===================================================================
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -33,12 +33,12 @@
// expected-note@-2 {{Opened stream never closed. Potential resource leak}}
void check_note_freopen() {
- FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+ FILE *F = fopen("file", "r");
if (!F)
// expected-note@-1 {{'F' is non-null}}
// expected-note@-2 {{Taking false branch}}
return;
- F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
+ F = freopen(0, "w", F); // expected-note {{Stream opened here}}
if (!F)
// expected-note@-1 {{'F' is non-null}}
// expected-note@-2 {{Taking false branch}}
@@ -47,6 +47,33 @@
// expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
// expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+void check_note_fopen_fail() {
+ FILE *F = fopen("file", "r"); // expected-note {{Stream open fails here}} expected-note {{Assuming pointer value is null}} expected-note {{'F' initialized here}}
+ fclose(F); // expected-warning {{Stream pointer might be NULL}}
+ // expected-note@-1 {{Stream pointer might be NULL}}
+}
+
+void check_note_freopen_fail() {
+ FILE *F = fopen("file", "r");
+ if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}}
+ return;
+ freopen(0, "w", F); // expected-note {{Stream reopen fails here}}
+ fclose(F); // expected-warning {{Stream might be invalid after (re-)opening it has failed. Can cause undefined behaviour}}
+ // expected-note@-1 {{Stream might be invalid after (re-)opening it has failed. Can cause undefined behaviour}}
+}
+
+void check_note_freopen_fail_null() {
+ // FIXME: The following note should not be here.
+ FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+ if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}}
+ return;
+ // FIXME: Note about failing 'freopen' belongs here.
+ // Why is a note at 'fopen'?
+ F = freopen(0, "w", F); // expected-note {{Null pointer value stored to 'F'}}
+ fclose(F); // expected-warning {{Stream pointer might be NULL}}
+ // expected-note@-1 {{Stream pointer might be NULL}}
+}
+
void check_note_leak_2(int c) {
FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
if (!F1)
@@ -80,7 +107,7 @@
void check_track_null() {
FILE *F;
- F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
+ F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}} expected-note {{Stream open fails here}}
if (F != NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
fclose(F);
return;
@@ -99,8 +126,8 @@
fread(Buf, 1, 1, F);
if (feof(F)) { // expected-note {{Taking true branch}}
clearerr(F);
- fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
- if (feof(F)) { // expected-note {{Taking true branch}}
+ fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}}
+ if (feof(F)) { // expected-note {{Assuming the end-of-file flag is set on the stream}} expected-note {{Taking true branch}}
fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
// expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
}
@@ -123,8 +150,8 @@
fclose(F);
return;
}
- fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
- if (feof(F)) { // expected-note {{Taking true branch}}
+ fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}}
+ if (feof(F)) { // expected-note {{Assuming the end-of-file flag is set on the stream}} expected-note {{Taking true branch}}
fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
// expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
}
@@ -137,11 +164,71 @@
F = fopen("foo1.c", "r");
if (F == NULL) // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}}
return;
- int RRet = fread(Buf, 1, 1, F); // expected-note {{Assuming stream reaches end-of-file here}}
- if (ferror(F)) { // expected-note {{Taking false branch}}
+ int RRet = fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}}
+ if (ferror(F)) { // expected-note {{Assuming the error flag is not set on the stream}} expected-note {{Taking false branch}}
} else {
fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}}
// expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}}
}
fclose(F);
}
+
+void check_indeterminate_notes_only_at_last_failure() {
+ FILE *F;
+ char Buf[10];
+ F = fopen("foo1.c", "r");
+ if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+ return;
+ fread(Buf, 1, 1, F);
+ if (ferror(F)) { // expected-note {{Taking true branch}}
+ F = freopen(0, "w", F);
+ if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+ return;
+ fread(Buf, 1, 1, F); // expected-note {{Stream reaches end-of-file or operation fails here}}
+ if (ferror(F)) { // expected-note {{Assuming the error flag is set on the stream}} expected-note{{Taking true branch}}
+ fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ }
+ }
+ fclose(F);
+}
+
+void check_indeterminate_notes_fseek() {
+ FILE *F;
+ char Buf[10];
+ F = fopen("foo1.c", "r");
+ if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+ return;
+ fseek(F, 1, SEEK_SET); // expected-note {{Operation fails or stream reaches end-of-file here}}
+ if (!feof(F)) // expected-note{{Assuming the end-of-file flag is not set on the stream}} expected-note {{Taking true branch}}
+ fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ fclose(F);
+}
+
+void check_indeterminate_notes_fwrite() {
+ FILE *F;
+ char Buf[10];
+ F = fopen("foo1.c", "r");
+ if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+ return;
+ fwrite(Buf, 1, 1, F); // expected-note {{Operation fails here}}
+ if (ferror(F)) // expected-note {{Assuming the error flag is set on the stream}} expected-note {{Taking true branch}}
+ fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ fclose(F);
+}
+
+void check_indeterminate_notes_fseek_no_feof_no_ferror() {
+ FILE *F;
+ char Buf[10];
+ F = fopen("foo1.c", "r");
+ if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+ return;
+ fseek(F, 1, SEEK_SET); // expected-note {{Operation fails or stream reaches end-of-file here}}
+ if (!ferror(F) && !feof(F)) // expected-note {{Assuming the error flag is not set on the stream}} expected-note {{Assuming the end-of-file flag is not set on the stream}}
+ // expected-note@-1 {{Taking true branch}} expected-note@-1 {{Left side of '&&' is true}}
+ fread(Buf, 1, 1, F); // expected-warning{{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ // expected-note@-1{{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+ fclose(F);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -19,6 +19,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/DenseMap.h"
#include <functional>
using namespace clang;
@@ -231,8 +232,6 @@
/// If true, evaluate special testing stream functions.
bool TestMode = false;
- const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
-
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
{{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
@@ -256,11 +255,13 @@
{&StreamChecker::preDefault, &StreamChecker::evalClearerr, 0}},
{{"feof", 1},
{&StreamChecker::preDefault,
- std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof),
+ std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof,
+ FEofNoteMessages),
0}},
{{"ferror", 1},
{&StreamChecker::preDefault,
- std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError),
+ std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError,
+ FErrorNoteMessages),
0}},
{{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0}},
};
@@ -277,6 +278,15 @@
0}},
};
+ const char *FEofNoteMessages[2] = {
+ "Assuming the end-of-file flag is set on the stream",
+ "Assuming the end-of-file flag is not set on the stream",
+ };
+ const char *FErrorNoteMessages[2] = {
+ "Assuming the error flag is set on the stream",
+ "Assuming the error flag is not set on the stream",
+ };
+
void evalFopen(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;
@@ -309,8 +319,8 @@
CheckerContext &C) const;
void evalFeofFerror(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C,
- const StreamErrorState &ErrorKind) const;
+ CheckerContext &C, const StreamErrorState &ErrorKind,
+ const char *NoteMessages[2]) const;
void evalSetFeofFerror(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C,
@@ -376,37 +386,40 @@
return FnDescriptions.lookup(Call);
}
- /// Generate a message for BugReporterVisitor if the stored symbol is
- /// marked as interesting by the actual bug report.
- // FIXME: Use lambda instead.
- struct NoteFn {
- const BugType *BT_ResourceLeak;
- SymbolRef StreamSym;
- std::string Message;
-
- std::string operator()(PathSensitiveBugReport &BR) const {
- if (BR.isInteresting(StreamSym) && &BR.getBugType() == BT_ResourceLeak)
- return Message;
+ /// Create a NoteTag to display a note if a later bug report is generated.
+ /// A NoteTag is added at every stream operation that fails in some way or
+ /// causes a later failure (bug). Successful opening a stream is a "failure"
+ /// in this sense if a resource leak is detected later.
+ /// At a bug report the "failed operation" is always the last in the path
+ /// (where this note is displayed) and previous such notes are not displayed.
+ const NoteTag *constructFailureNoteTag(CheckerContext &C, SymbolRef StreamSym,
+ const char *Message) const {
+
+ return C.getNoteTag([StreamSym, Message](PathSensitiveBugReport &BR) {
+ if (!BR.isInteresting(StreamSym))
+ return "";
- return "";
- }
- };
+ // A failing operation is always the last failable backwards.
+ // Stop reporting the previous (failable) operations.
+ BR.markNotInteresting(StreamSym);
- const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym,
- const std::string &Message) const {
- return C.getNoteTag(NoteFn{&BT_ResourceLeak, StreamSym, Message});
+ return Message;
+ });
}
- const NoteTag *constructSetEofNoteTag(CheckerContext &C,
- SymbolRef StreamSym) const {
- return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
- if (!BR.isInteresting(StreamSym) ||
- &BR.getBugType() != this->getBT_StreamEof())
- return "";
+ /// Construct a NoteTag to display a message if any bug is detected later on
+ /// the path (if no other failing operation follows).
+ /// This note is inserted into places where something important about
+ /// the last failing operation is discovered.
+ const NoteTag *constructNonFailureNoteTag(CheckerContext &C,
+ SymbolRef StreamSym,
+ const char *Message) const {
- BR.markNotInteresting(StreamSym);
+ return C.getNoteTag([StreamSym, Message](PathSensitiveBugReport &BR) {
+ if (!BR.isInteresting(StreamSym))
+ return "";
- return "Assuming stream reaches end-of-file here";
+ return Message;
});
}
@@ -500,8 +513,9 @@
StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
C.addTransition(StateNotNull,
- constructNoteTag(C, RetSym, "Stream opened here"));
- C.addTransition(StateNull);
+ constructFailureNoteTag(C, RetSym, "Stream opened here"));
+ C.addTransition(StateNull,
+ constructFailureNoteTag(C, RetSym, "Stream open fails here"));
}
void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
@@ -557,8 +571,9 @@
StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc));
C.addTransition(StateRetNotNull,
- constructNoteTag(C, StreamSym, "Stream reopened here"));
- C.addTransition(StateRetNull);
+ constructFailureNoteTag(C, StreamSym, "Stream opened here"));
+ C.addTransition(StateRetNull, constructFailureNoteTag(
+ C, StreamSym, "Stream reopen fails here"));
}
void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
@@ -579,7 +594,7 @@
// and can not be used any more.
State = State->set<StreamMap>(Sym, StreamState::getClosed(Desc));
- C.addTransition(State);
+ C.addTransition(State, constructFailureNoteTag(C, Sym, "Stream closed here"));
}
void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call,
@@ -705,9 +720,13 @@
StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
if (IsFread && OldSS->ErrorState != ErrorFEof)
- C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+ C.addTransition(StateFailed,
+ constructFailureNoteTag(
+ C, StreamSym,
+ "Stream reaches end-of-file or operation fails here"));
else
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, constructFailureNoteTag(
+ C, StreamSym, "Operation fails here"));
}
void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
@@ -766,7 +785,10 @@
StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
C.addTransition(StateNotFailed);
- C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+ C.addTransition(
+ StateFailed,
+ constructFailureNoteTag(
+ C, StreamSym, "Operation fails or stream reaches end-of-file here"));
}
void StreamChecker::evalClearerr(const FnDescription *Desc,
@@ -792,7 +814,8 @@
void StreamChecker::evalFeofFerror(const FnDescription *Desc,
const CallEvent &Call, CheckerContext &C,
- const StreamErrorState &ErrorKind) const {
+ const StreamErrorState &ErrorKind,
+ const char *NoteMessages[2]) const {
ProgramStateRef State = C.getState();
SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
if (!StreamSym)
@@ -813,20 +836,24 @@
// Function returns true.
// From now on it is the only one error state.
ProgramStateRef TrueState = bindAndAssumeTrue(State, C, CE);
- C.addTransition(TrueState->set<StreamMap>(
+ TrueState = TrueState->set<StreamMap>(
StreamSym, StreamState::getOpened(Desc, ErrorKind,
SS->FilePositionIndeterminate &&
- !ErrorKind.isFEof())));
+ !ErrorKind.isFEof()));
+ C.addTransition(TrueState,
+ constructNonFailureNoteTag(C, StreamSym, NoteMessages[0]));
}
if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) {
// Execution path(s) with ErrorKind not set.
// Function returns false.
// New error state is everything before minus ErrorKind.
ProgramStateRef FalseState = bindInt(0, State, C, CE);
- C.addTransition(FalseState->set<StreamMap>(
+ FalseState = FalseState->set<StreamMap>(
StreamSym,
StreamState::getOpened(
- Desc, NewES, SS->FilePositionIndeterminate && !NewES.isFEof())));
+ Desc, NewES, SS->FilePositionIndeterminate && !NewES.isFEof()));
+ C.addTransition(FalseState,
+ constructNonFailureNoteTag(C, StreamSym, NoteMessages[1]));
}
}
@@ -901,9 +928,11 @@
// according to cppreference.com .
ExplodedNode *N = C.generateErrorNode();
if (N) {
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ auto R = std::make_unique<PathSensitiveBugReport>(
BT_UseAfterClose,
- "Stream might be already closed. Causes undefined behaviour.", N));
+ "Stream might be already closed. Causes undefined behaviour.", N);
+ R->markInteresting(Sym);
+ C.emitReport(std::move(R));
return nullptr;
}
@@ -917,12 +946,14 @@
// failed to open.
ExplodedNode *N = C.generateErrorNode();
if (N) {
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ auto R = std::make_unique<PathSensitiveBugReport>(
BT_UseAfterOpenFailed,
"Stream might be invalid after "
"(re-)opening it has failed. "
"Can cause undefined behaviour.",
- N));
+ N);
+ R->markInteresting(Sym);
+ C.emitReport(std::move(R));
return nullptr;
}
return State;
@@ -957,8 +988,10 @@
if (!N)
return nullptr;
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
- BT_IndeterminatePosition, BugMessage, N));
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT_IndeterminatePosition, BugMessage, N);
+ R->markInteresting(Sym);
+ C.emitReport(std::move(R));
return State->set<StreamMap>(
Sym, StreamState::getOpened(SS->LastOperation, ErrorFEof, false));
}
@@ -966,9 +999,12 @@
// Known or unknown error state without FEOF possible.
// Stop analysis, report error.
ExplodedNode *N = C.generateErrorNode(State);
- if (N)
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
- BT_IndeterminatePosition, BugMessage, N));
+ if (N) {
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT_IndeterminatePosition, BugMessage, N);
+ R->markInteresting(Sym);
+ C.emitReport(std::move(R));
+ }
return nullptr;
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
