[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").

Author: Szelethus

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;

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));
   }
 

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