[analyzer] Note last "fclose" call from "ensureStreamOpened"

[steakhal]

Patch by Arseniy Zaostrovnykh!

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Balazs Benics (steakhal)

Changes

Patch by Arseniy Zaostrovnykh!

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+45-4)
• (modified) clang/test/Analysis/stream-note.c (+9)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 8bb7880a3cc283..c0f3591662b418 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1835,6 +1835,46 @@ StreamChecker::ensureStreamNonNull(SVal StreamVal, const Expr *StreamE,
   return StateNotNull;
 }
 
+namespace {
+class StreamClosedVisitor final : public BugReporterVisitor {
+  const SymbolRef StreamSym;
+  bool Satisfied = false;
+
+public:
+  explicit StreamClosedVisitor(SymbolRef StreamSym) : StreamSym(StreamSym) {}
+
+  static void *getTag() {
+    static int Tag = 0;
+    return &Tag;
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    ID.AddPointer(getTag());
+    ID.AddPointer(StreamSym);
+  }
+
+  PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+                                   BugReporterContext &BRC,
+                                   PathSensitiveBugReport &BR) override {
+    if (Satisfied)
+      return nullptr;
+    const StreamState *PredSS =
+        N->getFirstPred()->getState()->get<StreamMap>(StreamSym);
+    if (PredSS && PredSS->isClosed())
+      return nullptr;
+
+    const Stmt *S = N->getStmtForDiagnostics();
+    if (!S)
+      return nullptr;
+    Satisfied = true;
+    PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+                               N->getLocationContext());
+    llvm::StringLiteral Msg = "Stream is closed here";
+    return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg);
+  }
+};
+} // namespace
+
 ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
                                                   CheckerContext &C,
                                                   ProgramStateRef State) const {
@@ -1849,11 +1889,12 @@ ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
   if (SS->isClosed()) {
     // Using a stream pointer after 'fclose' causes undefined behavior
     // according to cppreference.com .
-    ExplodedNode *N = C.generateErrorNode();
-    if (N) {
-      C.emitReport(std::make_unique<PathSensitiveBugReport>(
+    if (ExplodedNode *N = C.generateErrorNode()) {
+      auto R = std::make_unique<PathSensitiveBugReport>(
           BT_UseAfterClose,
-          "Stream might be already closed. Causes undefined behaviour.", N));
+          "Stream might be already closed. Causes undefined behaviour.", N);
+      R->addVisitor<StreamClosedVisitor>(Sym);
+      C.emitReport(std::move(R));
       return nullptr;
     }
 
diff --git a/clang/test/Analysis/stream-note.c b/clang/test/Analysis/stream-note.c
index 3aef707d50056e..02251e2e12d6a1 100644
--- a/clang/test/Analysis/stream-note.c
+++ b/clang/test/Analysis/stream-note.c
@@ -264,3 +264,12 @@ void error_fseek_read_eof(void) {
   fgetc(F); // no warning
   fclose(F);
 }
+
+void check_note_at_use_after_close(void) {
+  FILE *F = tmpfile();
+  if (!F) // expected-note {{'F' is non-null}} expected-note {{Taking false branch}}
+    return;
+  fclose(F); // expected-note {{Stream is closed here}}
+  rewind(F); // expected-warning {{Stream might be already closed. Causes undefined behaviour}}
+  // expected-note@-1 {{Stream might be already closed. Causes undefined behaviour}}
+}

[NagyDonat]

I like this improvement 😄, thanks for implementing it!

The code that you added LGTM, my only concern is the awkward warning message which was inherited from the earlier state of the codebase. I think it would be nice to slip in an "attack of opportunity" that improves it, but I can also change that message in a follow-up commit if you don't want to bother with it.

[github-actions]

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