[clang] fc4b09d - [analyzer] Add an ownership change visitor to StreamChecker (#94957)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 24 07:34:40 PDT 2024
Author: Kristóf Umann
Date: 2024-06-24T16:34:36+02:00
New Revision: fc4b09d16139348533f1a1c9c72c99dacba51417
URL: https://github.com/llvm/llvm-project/commit/fc4b09d16139348533f1a1c9c72c99dacba51417
DIFF: https://github.com/llvm/llvm-project/commit/fc4b09d16139348533f1a1c9c72c99dacba51417.diff
LOG: [analyzer] Add an ownership change visitor to StreamChecker (#94957)
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").
Added:
clang/test/Analysis/stream-notes-missing-close.cpp
Modified:
clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Removed:
################################################################################
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;
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 613c221de7b4c..9aee7f952ad2d 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/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"
@@ -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,18 @@ 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";
+ }
+ llvm_unreachable("Unknown StreamState!");
+ }
+
/// State of the error flags.
/// Ignored in non-opened stream state but must be NoError.
StreamErrorState const ErrorState;
@@ -146,6 +167,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);
@@ -183,6 +207,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 +332,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
/// If true, generate failure branches for cases that are often not checked.
bool PedanticMode = false;
+ const CallDescription FCloseDesc = {CDM::CLibrary, {"fclose"}, 1};
+
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
{{CDM::CLibrary, {"fopen"}, 2},
@@ -310,8 +344,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},
- {&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}},
@@ -696,6 +729,62 @@ struct StreamOperationEvaluator {
} // end anonymous namespace
+//===----------------------------------------------------------------------===//
+// Definition of NoStreamStateChangeVisitor.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class NoStreamStateChangeVisitor final : public NoOwnershipChangeVisitor {
+protected:
+ /// Syntactically checks whether the callee is a closing 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
+ /// closing function was called indirectly through a function pointer,
+ /// but we are not able to tell, so this is a best effort analysis.
+ bool isClosingCallAsWritten(const CallExpr &Call) const {
+ const auto *StreamChk = static_cast<const StreamChecker *>(&Checker);
+ return StreamChk->FCloseDesc.matchesAsWritten(Call);
+ }
+
+ bool doesFnIntendToHandleOwnership(const Decl *Callee,
+ ASTContext &ACtx) final {
+ 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 (isClosingCallAsWritten(*Call))
+ return true;
+ }
+ // TODO: Ownership might change with an attempt to store stream object, not
+ // only through closing it. Check for attempted stores as well.
+ return false;
+ }
+
+ 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 closing stream object or storing it for later "
+ "release");
+ }
+
+public:
+ NoStreamStateChangeVisitor(SymbolRef Sym, const StreamChecker *Checker)
+ : NoOwnershipChangeVisitor(Sym, Checker) {}
+};
+
+} // end anonymous namespace
+
const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
SymbolRef StreamSym,
CheckerContext &C) {
@@ -1872,6 +1961,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-notes-missing-close.cpp b/clang/test/Analysis/stream-notes-missing-close.cpp
new file mode 100644
index 0000000000000..272ca96c76ee1
--- /dev/null
+++ b/clang/test/Analysis/stream-notes-missing-close.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 closing stream object or storing it 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 closing stream object or storing it 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
More information about the cfe-commits
mailing list