[Analyzer][StreamChecker] Add note tags for file opening.
Summary: Bug reports of resource leak are now improved. If there are multiple resource leak paths for the same stream, only one wil be reported. Reviewers: Szelethus, xazax.hun, baloghadamsoftware, NoQ Reviewed By: Szelethus, NoQ Subscribers: NoQ, rnkovacs, xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gamesh411, Charusso, martong, ASDenysPetrov, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D81407
This commit is contained in:
parent
792786e34d
commit
e935a540ea
|
@ -216,8 +216,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
|
|||
"Read function called when stream is in EOF state. "
|
||||
"Function has no effect."};
|
||||
BuiltinBug BT_ResourceLeak{
|
||||
this, "Resource Leak",
|
||||
"Opened File never closed. Potential Resource leak."};
|
||||
this, "Resource leak",
|
||||
"Opened stream never closed. Potential resource leak."};
|
||||
|
||||
public:
|
||||
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
|
||||
|
@ -365,6 +365,33 @@ private:
|
|||
|
||||
return FnDescriptions.lookup(Call);
|
||||
}
|
||||
|
||||
/// Generate a message for BugReporterVisitor if the stored symbol is
|
||||
/// marked as interesting by the actual bug report.
|
||||
struct NoteFn {
|
||||
const CheckerNameRef CheckerName;
|
||||
SymbolRef StreamSym;
|
||||
std::string Message;
|
||||
|
||||
std::string operator()(PathSensitiveBugReport &BR) const {
|
||||
if (BR.isInteresting(StreamSym) &&
|
||||
CheckerName == BR.getBugType().getCheckerName())
|
||||
return Message;
|
||||
|
||||
return "";
|
||||
}
|
||||
};
|
||||
|
||||
const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym,
|
||||
const std::string &Message) const {
|
||||
return C.getNoteTag(NoteFn{getCheckerName(), StreamSym, Message});
|
||||
}
|
||||
|
||||
/// Searches for the ExplodedNode where the file descriptor was acquired for
|
||||
/// StreamSym.
|
||||
static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
|
||||
SymbolRef StreamSym,
|
||||
CheckerContext &C);
|
||||
};
|
||||
|
||||
} // end anonymous namespace
|
||||
|
@ -376,6 +403,27 @@ inline void assertStreamStateOpened(const StreamState *SS) {
|
|||
"Previous create of error node for non-opened stream failed?");
|
||||
}
|
||||
|
||||
const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
|
||||
SymbolRef StreamSym,
|
||||
CheckerContext &C) {
|
||||
ProgramStateRef State = N->getState();
|
||||
// When bug type is resource leak, exploded node N may not have state info
|
||||
// for leaked file descriptor, but predecessor should have it.
|
||||
if (!State->get<StreamMap>(StreamSym))
|
||||
N = N->getFirstPred();
|
||||
|
||||
const ExplodedNode *Pred = N;
|
||||
while (N) {
|
||||
State = N->getState();
|
||||
if (!State->get<StreamMap>(StreamSym))
|
||||
return Pred;
|
||||
Pred = N;
|
||||
N = N->getFirstPred();
|
||||
}
|
||||
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
void StreamChecker::checkPreCall(const CallEvent &Call,
|
||||
CheckerContext &C) const {
|
||||
const FnDescription *Desc = lookupFn(Call);
|
||||
|
@ -421,7 +469,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
|
|||
StateNull =
|
||||
StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
|
||||
|
||||
C.addTransition(StateNotNull);
|
||||
C.addTransition(StateNotNull,
|
||||
constructNoteTag(C, RetSym, "Stream opened here"));
|
||||
C.addTransition(StateNull);
|
||||
}
|
||||
|
||||
|
@ -476,7 +525,8 @@ void StreamChecker::evalFreopen(const FnDescription *Desc,
|
|||
StateRetNull =
|
||||
StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc));
|
||||
|
||||
C.addTransition(StateRetNotNull);
|
||||
C.addTransition(StateRetNotNull,
|
||||
constructNoteTag(C, StreamSym, "Stream reopened here"));
|
||||
C.addTransition(StateRetNull);
|
||||
}
|
||||
|
||||
|
@ -921,8 +971,38 @@ void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
|
|||
if (!N)
|
||||
continue;
|
||||
|
||||
C.emitReport(std::make_unique<PathSensitiveBugReport>(
|
||||
BT_ResourceLeak, BT_ResourceLeak.getDescription(), N));
|
||||
// Do not warn for non-closed stream at program exit.
|
||||
ExplodedNode *Pred = C.getPredecessor();
|
||||
if (Pred && Pred->getCFGBlock() &&
|
||||
Pred->getCFGBlock()->hasNoReturnElement())
|
||||
continue;
|
||||
|
||||
// Resource leaks can result in multiple warning that describe the same kind
|
||||
// of programming error:
|
||||
// void f() {
|
||||
// FILE *F = fopen("a.txt");
|
||||
// if (rand()) // state split
|
||||
// return; // warning
|
||||
// } // warning
|
||||
// While this isn't necessarily true (leaking the same stream could result
|
||||
// from a different kinds of errors), the reduction in redundant reports
|
||||
// makes this a worthwhile heuristic.
|
||||
// FIXME: Add a checker option to turn this uniqueing feature off.
|
||||
|
||||
const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C);
|
||||
assert(StreamOpenNode && "Could not find place of stream opening.");
|
||||
PathDiagnosticLocation LocUsedForUniqueing =
|
||||
PathDiagnosticLocation::createBegin(
|
||||
StreamOpenNode->getStmtForDiagnostics(), C.getSourceManager(),
|
||||
StreamOpenNode->getLocationContext());
|
||||
|
||||
std::unique_ptr<PathSensitiveBugReport> R =
|
||||
std::make_unique<PathSensitiveBugReport>(
|
||||
BT_ResourceLeak, BT_ResourceLeak.getDescription(), N,
|
||||
LocUsedForUniqueing,
|
||||
StreamOpenNode->getLocationContext()->getDecl());
|
||||
R->markInteresting(Sym);
|
||||
C.emitReport(std::move(R));
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -0,0 +1,48 @@
|
|||
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -analyzer-output text -verify %s
|
||||
|
||||
#include "Inputs/system-header-simulator.h"
|
||||
|
||||
void check_note_at_correct_open() {
|
||||
FILE *F1 = tmpfile(); // expected-note {{Stream opened here}}
|
||||
if (!F1)
|
||||
// expected-note@-1 {{'F1' is non-null}}
|
||||
// expected-note@-2 {{Taking false branch}}
|
||||
return;
|
||||
FILE *F2 = tmpfile();
|
||||
if (!F2) {
|
||||
// expected-note@-1 {{'F2' is non-null}}
|
||||
// expected-note@-2 {{Taking false branch}}
|
||||
fclose(F1);
|
||||
return;
|
||||
}
|
||||
rewind(F2);
|
||||
fclose(F2);
|
||||
rewind(F1);
|
||||
}
|
||||
// expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
|
||||
// expected-note@-2 {{Opened stream never closed. Potential resource leak}}
|
||||
|
||||
void check_note_fopen() {
|
||||
FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
|
||||
if (!F)
|
||||
// expected-note@-1 {{'F' is non-null}}
|
||||
// expected-note@-2 {{Taking false branch}}
|
||||
return;
|
||||
}
|
||||
// expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
|
||||
// expected-note@-2 {{Opened stream never closed. Potential resource leak}}
|
||||
|
||||
void check_note_freopen() {
|
||||
FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
|
||||
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}}
|
||||
if (!F)
|
||||
// expected-note@-1 {{'F' is non-null}}
|
||||
// expected-note@-2 {{Taking false branch}}
|
||||
return;
|
||||
}
|
||||
// expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
|
||||
// expected-note@-2 {{Opened stream never closed. Potential resource leak}}
|
|
@ -1,4 +1,4 @@
|
|||
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
|
||||
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -verify %s
|
||||
|
||||
#include "Inputs/system-header-simulator.h"
|
||||
|
||||
|
@ -139,7 +139,7 @@ void f_leak(int c) {
|
|||
if (!p)
|
||||
return;
|
||||
if(c)
|
||||
return; // expected-warning {{Opened File never closed. Potential Resource leak}}
|
||||
return; // expected-warning {{Opened stream never closed. Potential resource leak}}
|
||||
fclose(p);
|
||||
}
|
||||
|
||||
|
@ -240,3 +240,28 @@ void check_escape4() {
|
|||
fwrite("1", 1, 1, F); // expected-warning {{might be 'indeterminate'}}
|
||||
fclose(F);
|
||||
}
|
||||
|
||||
int Test;
|
||||
_Noreturn void handle_error();
|
||||
|
||||
void check_leak_noreturn_1() {
|
||||
FILE *F1 = tmpfile();
|
||||
if (!F1)
|
||||
return;
|
||||
if (Test == 1) {
|
||||
handle_error(); // no warning
|
||||
}
|
||||
rewind(F1);
|
||||
} // expected-warning {{Opened stream never closed. Potential resource leak}}
|
||||
|
||||
// Check that "location uniqueing" works.
|
||||
// This results in reporting only one occurence of resource leak for a stream.
|
||||
void check_leak_noreturn_2() {
|
||||
FILE *F1 = tmpfile();
|
||||
if (!F1)
|
||||
return;
|
||||
if (Test == 1) {
|
||||
return; // expected-warning {{Opened stream never closed. Potential resource leak}}
|
||||
}
|
||||
rewind(F1);
|
||||
} // no warning
|
||||
|
|
|
@ -1,4 +1,4 @@
|
|||
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
|
||||
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -verify %s
|
||||
|
||||
typedef struct _IO_FILE FILE;
|
||||
extern FILE *fopen(const char *path, const char *mode);
|
||||
|
@ -19,4 +19,4 @@ void f1() {
|
|||
|
||||
void f2() {
|
||||
FILE *f = fopen("file", "r");
|
||||
} // expected-warning {{Opened File never closed. Potential Resource leak}}
|
||||
} // expected-warning {{Opened stream never closed. Potential resource leak}}
|
||||
|
|
Loading…
Reference in New Issue