[clang] [analyzer] Add an ownership change visitor to StreamChecker (PR #94957)
Kristóf Umann via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 10 04:50:59 PDT 2024
https://github.com/Szelethus updated https://github.com/llvm/llvm-project/pull/94957
>From faf00d0e1286e053ba9fb457513bd8309eb541ed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelethus at gmail.com>
Date: Fri, 7 Jun 2024 12:07:35 +0200
Subject: [PATCH 1/3] [analyzer] Add an ownership change visitor to
StreamChecker
This is very similar to https://reviews.llvm.org/D105553, in fact, I
barely made any changes from MallocChecker's ownership visitor to this
one.
The new visitor emits a diagnostic note for function where a change in
stream ownership was expected (for example, it had a fclose() call), but
the ownership remained unchanged. This is similar to messages regarding
ordinary values ("Returning without writing to x").
Change-Id: I7621c178dd35713d860d27bfc644fb56a42b0946
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 106 ++++++++++-
clang/test/Analysis/stream-visitor.cpp | 179 ++++++++++++++++++
2 files changed, 282 insertions(+), 3 deletions(-)
create mode 100644 clang/test/Analysis/stream-visitor.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index d4e020f7a72a0..d726ab5eaa599 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -10,6 +10,9 @@
//
//===----------------------------------------------------------------------===//
+#include "NoOwnershipChangeVisitor.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -74,6 +77,12 @@ struct StreamErrorState {
/// Returns if the StreamErrorState is a valid object.
operator bool() const { return NoError || FEof || FError; }
+ LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
+ LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &os) const {
+ os << "NoError: " << NoError << ", FEof: " << FEof
+ << ", FError: " << FError;
+ }
+
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddBoolean(NoError);
ID.AddBoolean(FEof);
@@ -98,6 +107,19 @@ struct StreamState {
OpenFailed /// The last open operation has failed.
} State;
+ StringRef getKindStr() const {
+ switch (State) {
+ case Opened:
+ return "Opened";
+ case Closed:
+ return "Closed";
+ case OpenFailed:
+ return "OpenFailed";
+ default:
+ llvm_unreachable("Unknown StreamState!");
+ }
+ }
+
/// State of the error flags.
/// Ignored in non-opened stream state but must be NoError.
StreamErrorState const ErrorState;
@@ -146,6 +168,9 @@ struct StreamState {
return StreamState{L, OpenFailed, {}, false};
}
+ LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
+ LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &os) const;
+
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddPointer(LastOperation);
ID.AddInteger(State);
@@ -168,8 +193,9 @@ REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
namespace {
class StreamChecker;
-using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
- const CallEvent &, CheckerContext &)>;
+using FnCheckTy = void(const StreamChecker *, const FnDescription *,
+ const CallEvent &, CheckerContext &);
+using FnCheck = std::function<FnCheckTy>;
using ArgNoTy = unsigned int;
static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
@@ -183,6 +209,14 @@ struct FnDescription {
ArgNoTy StreamArgNo;
};
+LLVM_DUMP_METHOD void StreamState::dumpToStream(llvm::raw_ostream &os) const {
+ os << "{Kind: " << getKindStr() << ", Last operation: " << LastOperation
+ << ", ErrorState: ";
+ ErrorState.dumpToStream(os);
+ os << ", FilePos: " << (FilePositionIndeterminate ? "Indeterminate" : "OK")
+ << '}';
+}
+
/// Get the value of the stream argument out of the passed call event.
/// The call should contain a function that is described by Desc.
SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) {
@@ -300,6 +334,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
/// If true, generate failure branches for cases that are often not checked.
bool PedanticMode = false;
+ CallDescription FCloseDesc = {CDM::CLibrary, {"fclose"}, 1};
+
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
{{CDM::CLibrary, {"fopen"}, 2},
@@ -310,7 +346,7 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
{&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
{{CDM::CLibrary, {"tmpfile"}, 0},
{nullptr, &StreamChecker::evalFopen, ArgNone}},
- {{CDM::CLibrary, {"fclose"}, 1},
+ {FCloseDesc,
{&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
{{CDM::CLibrary, {"fread"}, 4},
{&StreamChecker::preRead,
@@ -696,6 +732,69 @@ struct StreamOperationEvaluator {
} // end anonymous namespace
+//===----------------------------------------------------------------------===//
+// Definition of NoStreamStateChangeVisitor.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class NoStreamStateChangeVisitor final : public NoOwnershipChangeVisitor {
+protected:
+ /// Syntactically checks whether the callee is a freeing function. Since
+ /// we have no path-sensitive information on this call (we would need a
+ /// CallEvent instead of a CallExpr for that), its possible that a
+ /// freeing function was called indirectly through a function pointer,
+ /// but we are not able to tell, so this is a best effort analysis.
+ bool isFreeingCallAsWritten(const CallExpr &Call) const {
+ const auto *StreamChk = static_cast<const StreamChecker *>(&Checker);
+ if (StreamChk->FCloseDesc.matchesAsWritten(Call))
+ return true;
+
+ return false;
+ }
+
+ bool doesFnIntendToHandleOwnership(const Decl *Callee,
+ ASTContext &ACtx) override {
+ using namespace clang::ast_matchers;
+ const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
+
+ auto Matches =
+ match(findAll(callExpr().bind("call")), *FD->getBody(), ACtx);
+ for (BoundNodes Match : Matches) {
+ if (const auto *Call = Match.getNodeAs<CallExpr>("call"))
+ if (isFreeingCallAsWritten(*Call))
+ return true;
+ }
+ // TODO: Ownership might change with an attempt to store stream object, not
+ // only through freeing it. Check for attempted stores as well.
+ return false;
+ }
+
+ virtual bool hasResourceStateChanged(ProgramStateRef CallEnterState,
+ ProgramStateRef CallExitEndState) final {
+ return CallEnterState->get<StreamMap>(Sym) !=
+ CallExitEndState->get<StreamMap>(Sym);
+ }
+
+ PathDiagnosticPieceRef emitNote(const ExplodedNode *N) override {
+ PathDiagnosticLocation L = PathDiagnosticLocation::create(
+ N->getLocation(),
+ N->getState()->getStateManager().getContext().getSourceManager());
+ return std::make_shared<PathDiagnosticEventPiece>(
+ L, "Returning without freeing stream object or storing the pointer for "
+ "later release");
+ }
+
+public:
+ NoStreamStateChangeVisitor(SymbolRef Sym, const StreamChecker *Checker)
+ : NoOwnershipChangeVisitor(Sym, Checker) {}
+};
+
+} // end anonymous namespace
+
+inline void assertStreamStateOpened(const StreamState *SS) {
+ assert(SS->isOpened() && "Stream is expected to be opened");
+}
+
const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
SymbolRef StreamSym,
CheckerContext &C) {
@@ -1758,6 +1857,7 @@ StreamChecker::reportLeaks(const SmallVector<SymbolRef, 2> &LeakedSyms,
LocUsedForUniqueing,
StreamOpenNode->getLocationContext()->getDecl());
R->markInteresting(LeakSym);
+ R->addVisitor<NoStreamStateChangeVisitor>(LeakSym, this);
C.emitReport(std::move(R));
}
diff --git a/clang/test/Analysis/stream-visitor.cpp b/clang/test/Analysis/stream-visitor.cpp
new file mode 100644
index 0000000000000..8b25e2a7905bc
--- /dev/null
+++ b/clang/test/Analysis/stream-visitor.cpp
@@ -0,0 +1,179 @@
+// RUN: %clang_analyze_cc1 -verify %s -analyzer-output=text \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=unix.Stream
+
+
+#include "Inputs/system-header-simulator.h"
+char *logDump();
+bool coin();
+
+[[noreturn]] void halt();
+
+void assert(bool b) {
+ if (!b)
+ halt();
+}
+
+//===----------------------------------------------------------------------===//
+// Report for which we expect NoOwnershipChangeVisitor to add a new note.
+//===----------------------------------------------------------------------===//
+
+namespace stream_opened_in_fn_call {
+// TODO: AST analysis of sink would reveal that it doesn't intent to free the
+// allocated memory, but in this instance, its also the only function with
+// the ability to do so, we should see a note here.
+void sink(FILE *f) {
+}
+
+void f() {
+ sink(fopen("input.txt", "w"));
+ // expected-note at -1{{Stream opened here}}
+} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+// expected-note at -1{{Opened stream never closed. Potential resource leak}}
+} // namespace stream_opened_in_fn_call
+
+namespace stream_passed_to_fn_call {
+
+void expectedClose(FILE *f) {
+ if (char *log = logDump()) { // expected-note{{Assuming 'log' is null}}
+ // expected-note at -1{{Taking false branch}}
+ printf("%s", log);
+ fclose(f);
+ }
+} // expected-note{{Returning without freeing stream object or storing the pointer for later release}}
+
+void f() {
+ FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}}
+ if (!f) // expected-note{{'f' is non-null}}
+ // expected-note at -1{{Taking false branch}}
+ return;
+ if (coin()) { // expected-note{{Assuming the condition is true}}
+ // expected-note at -1{{Taking true branch}}
+ expectedClose(f); // expected-note{{Calling 'expectedClose'}}
+ // expected-note at -1{{Returning from 'expectedClose'}}
+
+ return; // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+ // expected-note at -1{{Opened stream never closed. Potential resource leak}}
+ }
+ fclose(f);
+}
+} // namespace stream_passed_to_fn_call
+
+namespace stream_shared_with_ptr_of_shorter_lifetime {
+
+void sink(FILE *f) {
+ FILE *Q = f;
+ if (coin()) // expected-note {{Assuming the condition is false}}
+ // expected-note at -1 {{Taking false branch}}
+ fclose(f);
+ (void)Q;
+} // expected-note{{Returning without freeing stream object or storing the pointer for later release}}
+
+void foo() {
+ FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}}
+ if (!f) // expected-note{{'f' is non-null}}
+ // expected-note at -1{{Taking false branch}}
+ return;
+ sink(f); // expected-note {{Calling 'sink'}}
+ // expected-note at -1 {{Returning from 'sink'}}
+} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+// expected-note at -1{{Opened stream never closed. Potential resource leak}}
+
+} // namespace stream_shared_with_ptr_of_shorter_lifetime
+
+//===----------------------------------------------------------------------===//
+// Report for which we *do not* expect NoOwnershipChangeVisitor add a new note,
+// nor do we want it to.
+//===----------------------------------------------------------------------===//
+
+namespace stream_not_passed_to_fn_call {
+
+void expectedClose(FILE *f) {
+ if (char *log = logDump()) {
+ printf("%s", log);
+ fclose(f);
+ }
+}
+
+void f(FILE *p) {
+ FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}}
+ if (!f) // expected-note{{'f' is non-null}}
+ // expected-note at -1{{Taking false branch}}
+ return;
+ expectedClose(p); // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+ // expected-note at -1{{Opened stream never closed. Potential resource leak}}
+}
+} // namespace stream_not_passed_to_fn_call
+
+namespace stream_shared_with_ptr_of_same_lifetime {
+
+void expectedClose(FILE *f, FILE **p) {
+ // NOTE: Not a job of NoOwnershipChangeVisitor, but maybe this could be
+ // highlighted still?
+ *p = f;
+}
+
+void f() {
+ FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}}
+ FILE *p = NULL;
+ if (!f) // expected-note{{'f' is non-null}}
+ // expected-note at -1{{Taking false branch}}
+ return;
+ expectedClose(f, &p);
+} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+ // expected-note at -1{{Opened stream never closed. Potential resource leak}}
+} // namespace stream_shared_with_ptr_of_same_lifetime
+
+namespace stream_passed_into_fn_that_doesnt_intend_to_free {
+void expectedClose(FILE *f) {
+}
+
+void f() {
+ FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}}
+ if (!f) // expected-note{{'f' is non-null}}
+ // expected-note at -1{{Taking false branch}}
+ return;
+ expectedClose(f);
+
+} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+ // expected-note at -1{{Opened stream never closed. Potential resource leak}}
+} // namespace stream_passed_into_fn_that_doesnt_intend_to_free
+
+namespace stream_passed_into_fn_that_doesnt_intend_to_free2 {
+void bar();
+
+void expectedClose(FILE *f) {
+ // Correctly realize that calling bar() doesn't mean that this function would
+ // like to deallocate anything.
+ bar();
+}
+
+void f() {
+ FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}}
+ if (!f) // expected-note{{'f' is non-null}}
+ // expected-note at -1{{Taking false branch}}
+ return;
+ expectedClose(f);
+
+} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+ // expected-note at -1{{Opened stream never closed. Potential resource leak}}
+} // namespace stream_passed_into_fn_that_doesnt_intend_to_free2
+
+namespace streamstate_from_closed_to_open {
+
+// StreamState of the symbol changed from nothing to Allocated. We don't want to
+// emit notes when the RefKind changes in the stack frame.
+static FILE *fopenWrapper() {
+ FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}}
+ assert(f);
+ return f;
+}
+void use_ret() {
+ FILE *v;
+ v = fopenWrapper(); // expected-note {{Calling 'fopenWrapper'}}
+ // expected-note at -1{{Returning from 'fopenWrapper'}}
+
+} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}}
+ // expected-note at -1{{Opened stream never closed. Potential resource leak}}
+
+} // namespace streamstate_from_closed_to_open
>From fd885784f570c80659732223d1bf83edd64b017a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelethus at gmail.com>
Date: Mon, 10 Jun 2024 13:41:13 +0200
Subject: [PATCH 2/3] Format and respond to reviewer comments
Change-Id: I77f3e6efee4f235933ab1116ae303d3eab61a183
---
.../lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index d726ab5eaa599..b90a4f1302e38 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -11,8 +11,8 @@
//===----------------------------------------------------------------------===//
#include "NoOwnershipChangeVisitor.h"
-#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -193,9 +193,8 @@ REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
namespace {
class StreamChecker;
-using FnCheckTy = void(const StreamChecker *, const FnDescription *,
- const CallEvent &, CheckerContext &);
-using FnCheck = std::function<FnCheckTy>;
+using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
+ const CallEvent &, CheckerContext &)>;
using ArgNoTy = unsigned int;
static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
@@ -346,8 +345,7 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
{&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
{{CDM::CLibrary, {"tmpfile"}, 0},
{nullptr, &StreamChecker::evalFopen, ArgNone}},
- {FCloseDesc,
- {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
+ {FCloseDesc, {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
{{CDM::CLibrary, {"fread"}, 4},
{&StreamChecker::preRead,
std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, true), 3}},
@@ -780,8 +778,8 @@ class NoStreamStateChangeVisitor final : public NoOwnershipChangeVisitor {
N->getLocation(),
N->getState()->getStateManager().getContext().getSourceManager());
return std::make_shared<PathDiagnosticEventPiece>(
- L, "Returning without freeing stream object or storing the pointer for "
- "later release");
+ L, "Returning without freeing stream object or storing it for later "
+ "release");
}
public:
>From 53cd6db3883a83e0a84d36b1854f6f28d88cd6de Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelethus at gmail.com>
Date: Mon, 10 Jun 2024 13:50:46 +0200
Subject: [PATCH 3/3] Move OwnershipBindingsHandler to an anonymous namespace
Change-Id: Ic92b15e14c3c4aee26ae55d2fe91b1dc4c4b73e8
---
clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
index 2ff76679b5ebf..22b5ebfd6fab0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
@@ -18,6 +18,7 @@ using namespace clang;
using namespace ento;
using OwnerSet = NoOwnershipChangeVisitor::OwnerSet;
+namespace {
// Collect which entities point to the allocated memory, and could be
// responsible for deallocating it.
class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
@@ -46,6 +47,7 @@ class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
out << "}\n";
}
};
+} // namespace
OwnerSet NoOwnershipChangeVisitor::getOwnersAtNode(const ExplodedNode *N) {
OwnerSet Ret;
More information about the cfe-commits
mailing list