[analyzer] Add an ownership change visitor to StreamChecker

[Szelethus]

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

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Kristóf Umann (Szelethus)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/94957.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+103-3)
• (added) clang/test/Analysis/stream-visitor.cpp (+179)

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

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[NagyDonat]

Nice improvement, LGTM overall. I only have one phrasing nitpick and one very minor question about a tangential change.

[NagyDonat]

Suggested change

	L, "Returning without freeing stream object or storing the pointer for "
	L, "Returning without freeing stream object or storing it for "

The current phrasing is technically correct, because the stream object is a FILE * pointer, but IMO "it" would be both shorter and easier to understand

[balazske]

I do not see this as problem, and exactly not the stream object itself is stored, only the pointer to it.

[NagyDonat]

What is the goal of this change?

[balazske]

I did not find big issues. But a description could be added to the NoOwnershipChangeVisitor to explain what it does, and to StreamChecker for what is it used. I did not find a similar test for MallocChecker but there could be one with similar test functions.

[balazske]

Why does this function appear here (it is not removed from other location)?

[balazske]

This file can have a better name (like "stream-notes-missing-close.cpp").

[steakhal]

LGTM. I only had some nits and some terminology flexing.

[steakhal]

Couldn't we rely on the CallGraph for determining this?

My problem with the approach that it only looks at direct callees of the given function.
What if we have the "freeing" function wrapped by some other function?
-- I guess, then we eventually get there and that's gonna be the direct callee. So it will only affect the place of the new note - which makes sense.

I guess I already answered my question :D
No actions expected.

[Szelethus]

Showing that a function intended to close a stream already relies on heuristics, and its easy to construct dumb counterexamples for it:

void absolutelyDoesntCloseItsParam(FILE *p) {
  if (coin()) {
    FILE *q = fopen(...);
    fclose(q); // oh, we saw an fclose, this must've been intended for the param...
  }
}

We can surely make this smarter, maybe we should, but in finite steps we get to the point where we start rewriting the analyzer.

Regarding the CallGraph: if we start to wander of the beaten path (meaning the path of execution the analyzer was on), especially if we inspect non-trivial functions in the graph, that will lead to a weaker heuristic in my view.

[Szelethus]

I did not find a similar test for MallocChecker but there could be one with similar test functions.

I'm not sure what tests you are referring to. I did fix your other observations.

[balazske]

I did not find a similar test for MallocChecker but there could be one with similar test functions.

I'm not sure what tests you are referring to. I did fix your other observations.

I meant another test file where the NoStateChangeFuncVisitor is tested (if there is any case for it).

[balazske]

At least the StreamChecker part looks correct, Probably we can test on the opensource projects if there appear too many bad results (but probably not many resource leak cases are found).

[Szelethus]

I did not find a similar test for MallocChecker but there could be one with similar test functions.

I'm not sure what tests you are referring to. I did fix your other observations.

I meant another test file where the NoStateChangeFuncVisitor is tested (if there is any case for it).

Oh, I see. The MallocChecker visitor is tested in clang/test/Analysis/NewDeleteLeaks.cpp. Evaluations are on the way.