[clang] efa8b6e - [Analyzer][StreamChecker] Add check for pointer escape.

[Balázs Kéri via cfe-commits]

Author: Balázs Kéri
Date: 2020-06-15T15:43:23+02:00
New Revision: efa8b6e884aa4c3ad74ab44489352c0f80ac9741

URL: https://github.com/llvm/llvm-project/commit/efa8b6e884aa4c3ad74ab44489352c0f80ac9741
DIFF: https://github.com/llvm/llvm-project/commit/efa8b6e884aa4c3ad74ab44489352c0f80ac9741.diff

LOG: [Analyzer][StreamChecker] Add check for pointer escape.

Summary:
After an escaped FILE* stream handle it is not possible to make
reliable checks on it because any function call can have effect
on it.

Reviewers: Szelethus, baloghadamsoftware, martong, NoQ

Reviewed By: NoQ

Subscribers: NoQ, rnkovacs, xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gamesh411, Charusso, martong, ASDenysPetrov, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D80699

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
    clang/test/Analysis/stream.c

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index bfb788c24e24..7e10b2aa4356 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -193,8 +193,8 @@ ProgramStateRef bindInt(uint64_t Value, ProgramStateRef State,
   return State;
 }
 
-class StreamChecker
-    : public Checker<check::PreCall, eval::Call, check::DeadSymbols> {
+class StreamChecker : public Checker<check::PreCall, eval::Call,
+                                     check::DeadSymbols, check::PointerEscape> {
   BuiltinBug BT_FileNull{this, "NULL stream pointer",
                          "Stream pointer might be NULL."};
   BuiltinBug BT_UseAfterClose{
@@ -223,6 +223,10 @@ class StreamChecker
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  ProgramStateRef checkPointerEscape(ProgramStateRef State,
+                                     const InvalidatedSymbols &Escaped,
+                                     const CallEvent *Call,
+                                     PointerEscapeKind Kind) const;
 
   /// If true, evaluate special testing stream functions.
   bool TestMode = false;
@@ -448,10 +452,14 @@ void StreamChecker::evalFreopen(const FnDescription *Desc,
 
   SymbolRef StreamSym = StreamVal->getAsSymbol();
   // Do not care about concrete values for stream ("(FILE *)0x12345"?).
-  // FIXME: Are stdin, stdout, stderr such values?
+  // FIXME: Can be stdin, stdout, stderr such values?
   if (!StreamSym)
     return;
 
+  // Do not handle untracked stream. It is probably escaped.
+  if (!State->get<StreamMap>(StreamSym))
+    return;
+
   // Generate state for non-failed case.
   // Return value is the passed stream pointer.
   // According to the documentations, the stream is closed first
@@ -918,6 +926,28 @@ void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
   }
 }
 
+ProgramStateRef StreamChecker::checkPointerEscape(
+    ProgramStateRef State, const InvalidatedSymbols &Escaped,
+    const CallEvent *Call, PointerEscapeKind Kind) const {
+  // Check for file-handling system call that is not handled by the checker.
+  // FIXME: The checker should be updated to handle all system calls that take
+  // 'FILE*' argument. These are now ignored.
+  if (Kind == PSK_DirectEscapeOnCall && Call->isInSystemHeader())
+    return State;
+
+  for (SymbolRef Sym : Escaped) {
+    // The symbol escaped.
+    // From now the stream can be manipulated in unknown way to the checker,
+    // it is not possible to handle it any more.
+    // Optimistically, assume that the corresponding file handle will be closed
+    // somewhere else.
+    // Remove symbol from state so the following stream calls on this symbol are
+    // not handled by the checker.
+    State = State->remove<StreamMap>(Sym);
+  }
+  return State;
+}
+
 void ento::registerStreamChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<StreamChecker>();
 }

diff  --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 554a234c20d5..dbbcc8715e0d 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -192,4 +192,51 @@ void check_freopen_3() {
     rewind(f1); // expected-warning {{Stream might be invalid}}
     fclose(f1);
   }
-}
\ No newline at end of file
+}
+
+extern FILE *GlobalF;
+extern void takeFile(FILE *);
+
+void check_escape1() {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  fwrite("1", 1, 1, F); // may fail
+  GlobalF = F;
+  fwrite("1", 1, 1, F); // no warning
+}
+
+void check_escape2() {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  fwrite("1", 1, 1, F); // may fail
+  takeFile(F);
+  fwrite("1", 1, 1, F); // no warning
+}
+
+void check_escape3() {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  takeFile(F);
+  F = freopen(0, "w", F);
+  if (!F)
+    return;
+  fwrite("1", 1, 1, F); // may fail
+  fwrite("1", 1, 1, F); // no warning
+}
+
+void check_escape4() {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  fwrite("1", 1, 1, F); // may fail
+
+  // no escape at (non-StreamChecker-handled) system call
+  // FIXME: all such calls should be handled by the checker
+  fprintf(F, "0");
+
+  fwrite("1", 1, 1, F); // expected-warning {{might be 'indeterminate'}}
+  fclose(F);
+}