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@-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 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@-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 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@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
