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?= <dkszelet...@gmail.com> Date: Fri, 7 Jun 2024 12:07:35 +0200 Subject: [PATCH 1/4] [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@-1{{Stream opened here}} +} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} +// expected-note@-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@-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@-1{{Taking false branch}} + return; + if (coin()) { // expected-note{{Assuming the condition is true}} + // expected-note@-1{{Taking true branch}} + expectedClose(f); // expected-note{{Calling 'expectedClose'}} + // expected-note@-1{{Returning from 'expectedClose'}} + + return; // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} + // expected-note@-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@-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@-1{{Taking false branch}} + return; + sink(f); // expected-note {{Calling 'sink'}} + // expected-note@-1 {{Returning from 'sink'}} +} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} +// expected-note@-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@-1{{Taking false branch}} + return; + expectedClose(p); // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} + // expected-note@-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@-1{{Taking false branch}} + return; + expectedClose(f, &p); +} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} + // expected-note@-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@-1{{Taking false branch}} + return; + expectedClose(f); + +} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} + // expected-note@-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@-1{{Taking false branch}} + return; + expectedClose(f); + +} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} + // expected-note@-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@-1{{Returning from 'fopenWrapper'}} + +} // expected-warning{{Opened stream never closed. Potential resource leak [unix.Stream]}} + // expected-note@-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?= <dkszelet...@gmail.com> Date: Mon, 10 Jun 2024 13:41:13 +0200 Subject: [PATCH 2/4] 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?= <dkszelet...@gmail.com> Date: Mon, 10 Jun 2024 13:50:46 +0200 Subject: [PATCH 3/4] 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; From 7b1bb5b479badc6b31061d7aa20d8ede889aa08e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelet...@gmail.com> Date: Tue, 11 Jun 2024 10:23:26 +0200 Subject: [PATCH 4/4] Correct the test file Change-Id: I4991fe77c9444cc47a0fee4948937f64c59f0028 --- clang/test/Analysis/stream-visitor.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/test/Analysis/stream-visitor.cpp b/clang/test/Analysis/stream-visitor.cpp index 8b25e2a7905bc..cc54b1d9131c2 100644 --- a/clang/test/Analysis/stream-visitor.cpp +++ b/clang/test/Analysis/stream-visitor.cpp @@ -40,7 +40,7 @@ void expectedClose(FILE *f) { printf("%s", log); fclose(f); } -} // expected-note{{Returning without freeing stream object or storing the pointer for later release}} +} // expected-note{{Returning without freeing stream object or storing it for later release}} void f() { FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}} @@ -67,7 +67,7 @@ void sink(FILE *f) { // expected-note@-1 {{Taking false branch}} fclose(f); (void)Q; -} // expected-note{{Returning without freeing stream object or storing the pointer for later release}} +} // expected-note{{Returning without freeing stream object or storing it for later release}} void foo() { FILE *f = fopen("input.txt", "w"); // expected-note{{Stream opened here}} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
