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

[balazske]

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.

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

439–441

How does this interact with D78280?

952–954

Foreach?

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

439–441

That change needs to be updated after this. A new "escaped" stream state is needed to avoid recognizing an escaped stream again.

952–954

The foreach type loop does work too, will be updated (this code is taken from another checker).

[balazske]

Using range-based for loop.

[balazske]

Rebased.

[balazske]

Removed bad empty line.

[Szelethus]

@NoQ @baloghadamsoftware Escapes are more within your realm of expertise. Anything to add?

From my end, this looks okay.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

439–441

Why not take that approach now? I mean that instead of deleting the stream from the map set its state to "escaped".

[NoQ]

@NoQ @baloghadamsoftware Escapes are more within your realm of expertise. Anything to add?

This looks like a fairly standard approach to take, no objections.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

952–954

this code is taken from another checker

We were only allowed to use C++11 in LLVM very recently. Most of our code was written before that happened.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 
 ProgramStateRef bindInt(uint64_t Value, ProgramStateRef State,
                         CheckerContext &C, const CallExpr *CE) {
   State = State->BindExpr(CE, C.getLocationContext(),
                           C.getSValBuilder().makeIntVal(Value, false));
   return State;
 }
 
-class StreamChecker
-    : public Checker<check::PreCall, eval::Call, check::DeadSymbols> {
+class StreamChecker : public Checker<check::PreCall, eval::Call,
+                                     check::DeadSymbols, check::PointerEscape> {
   mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
       BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof,
       BT_IndeterminatePosition;
 
 public:
   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;
 
 private:
   CallDescriptionMap<FnDescription> FnDescriptions = {
       {{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
       {{"freopen", 3},
void StreamChecker::evalFreopen(const FnDescription *Desc,
 
   Optional<DefinedSVal> StreamVal =
       getStreamArg(Desc, Call).getAs<DefinedSVal>();
   if (!StreamVal)
     return;
 
   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
   // but any close error is ignored. The state changes to (or remains) opened.
   ProgramStateRef StateRetNotNull =
       State->BindExpr(CE, C.getLocationContext(), *StreamVal);
   // Generate state for NULL return value.
   // Stream switches to OpenFailed state.
if (!BT_ResourceLeak)
       BT_ResourceLeak.reset(
           new BuiltinBug(this, "Resource Leak",
                          "Opened File never closed. Potential Resource leak."));
     C.emitReport(std::make_unique<PathSensitiveBugReport>(
         *BT_ResourceLeak, BT_ResourceLeak->getDescription(), N));
   }
 }
 
+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 (InvalidatedSymbols::const_iterator I = Escaped.begin(),
+                                          E = Escaped.end();
+       I != E; ++I) {
+    SymbolRef Sym = *I;
+
+    // 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>();
 }
 
 bool ento::shouldRegisterStreamChecker(const CheckerManager &Mgr) {
   return true;
 }
 
 void ento::registerStreamTesterChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.getChecker<StreamChecker>();
   Checker->TestMode = true;
 }
 
 bool ento::shouldRegisterStreamTesterChecker(const CheckerManager &Mgr) {
   return true;
 }

clang/test/Analysis/stream.c

void check_freopen_3() {
   FILE *f1 = fopen("foo.c", "r");
   if (f1) {
     // Unchecked result of freopen.
     // The f1 may be invalid after this call.
     freopen(0, "w", f1);
     rewind(f1); // expected-warning {{Stream might be invalid}}
     fclose(f1);
   }
-}
+}
+
+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);
+}