balazske updated this revision to Diff 360124.
balazske added a comment.
Support multiple bug types in the note tag function.
Add the note tag to every place and update tests.
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,30 @@
// 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 {{Assuming opening the stream 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 {{Assuming opening the stream 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() {
+ FILE *F = fopen("file", "r");
+ if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}}
+ return;
+ 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 +104,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 {{Assuming opening the stream fails here}}
if (F != NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
fclose(F);
return;
@@ -145,3 +169,62 @@
}
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 {{Assuming this stream operation fails}}
+ if (ferror(F)) { // 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 {{Assuming this stream operation fails}}
+ if (!feof(F)) // 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 {{Assuming this stream operation fails}}
+ if (ferror(F)) // 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 {{Assuming this stream operation fails}}
+ if (!ferror(F) && !feof(F)) // expected-note {{Taking true branch}} // expected-note {{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,7 +232,14 @@
/// If true, evaluate special testing stream functions.
bool TestMode = false;
- const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
+ using BugMessageMap = llvm::DenseMap<const BugType *, const char *>;
+ const BugMessageMap BugMessages = {
+ {&BT_FileNull, "Assuming opening the stream fails here"},
+ {&BT_UseAfterClose, "Stream closed here"},
+ {&BT_UseAfterOpenFailed, "Assuming opening the stream fails here"},
+ {&BT_IndeterminatePosition, "Assuming this stream operation fails"},
+ {&BT_StreamEof, "Assuming stream reaches end-of-file here"},
+ {&BT_ResourceLeak, "Stream opened here"}};
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
@@ -376,37 +384,24 @@
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;
-
- return "";
- }
- };
-
- const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym,
- const std::string &Message) const {
- return C.getNoteTag(NoteFn{&BT_ResourceLeak, StreamSym, 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())
+ /// Create a NoteTag to display a note if a later bug report is generated.
+ /// The `BT` should contain all bug types that could be caused by the
+ /// operation at this location.
+ /// If later on the path a problematic event (reported bug) happens with the
+ /// same type, the last place is found with a note tag with that bug type.
+ const NoteTag *
+ constructNoteTag(CheckerContext &C, SymbolRef StreamSym,
+ llvm::SmallPtrSet<const BugType *, 2> &&BT) const {
+
+ return C.getNoteTag([this, StreamSym, BT](PathSensitiveBugReport &BR) {
+ if (!BR.isInteresting(StreamSym) || !BT.contains(&BR.getBugType()))
return "";
+ // This is done to make the report only at the last location with the same
+ // note tag.
BR.markNotInteresting(StreamSym);
- return "Assuming stream reaches end-of-file here";
+ return this->BugMessages.lookup(&BR.getBugType());
});
}
@@ -500,8 +495,8 @@
StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
C.addTransition(StateNotNull,
- constructNoteTag(C, RetSym, "Stream opened here"));
- C.addTransition(StateNull);
+ constructNoteTag(C, RetSym, {&BT_ResourceLeak}));
+ C.addTransition(StateNull, constructNoteTag(C, RetSym, {&BT_FileNull}));
}
void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
@@ -557,8 +552,10 @@
StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc));
C.addTransition(StateRetNotNull,
- constructNoteTag(C, StreamSym, "Stream reopened here"));
- C.addTransition(StateRetNull);
+ constructNoteTag(C, StreamSym, {&BT_ResourceLeak}));
+ C.addTransition(
+ StateRetNull,
+ constructNoteTag(C, StreamSym, {&BT_FileNull, &BT_UseAfterOpenFailed}));
}
void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
@@ -579,7 +576,7 @@
// and can not be used any more.
State = State->set<StreamMap>(Sym, StreamState::getClosed(Desc));
- C.addTransition(State);
+ C.addTransition(State, constructNoteTag(C, Sym, {&BT_UseAfterClose}));
}
void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call,
@@ -705,9 +702,12 @@
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, constructNoteTag(C, StreamSym,
+ {&BT_StreamEof,
+ &BT_IndeterminatePosition}));
else
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, constructNoteTag(C, StreamSym,
+ {&BT_IndeterminatePosition}));
}
void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
@@ -766,7 +766,9 @@
StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
C.addTransition(StateNotFailed);
- C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+ C.addTransition(StateFailed,
+ constructNoteTag(C, StreamSym,
+ {&BT_StreamEof, &BT_IndeterminatePosition}));
}
void StreamChecker::evalClearerr(const FnDescription *Desc,
@@ -901,9 +903,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 +921,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 +963,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 +974,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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
