[clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[balazske]

Summary

Recognize initial value of standard streams (stdin, ...)
and assume that a new opened stream is not equal to these.

[martong]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

215–236

Would it be possible to reuse (i.e put in a common place) the StdLibraryFunctionChecker's lookupTy? That also handles the cases when FILEType is null or when we have a typedef struct FILE FILE;.

[martong]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

215–236

Could you please consider the above question?

[Szelethus]

I like the idea, though I wonder whether evalAssume would be a better callback for this. That way, you'd only need to add an assumption when you reach a condition where one of the operands is standard.

Though it may be more trouble than its worth. I don't have strong feelings on this.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

238

How about getStdStreamSymbol?

[balazske]

Split some type lookup functions from StdLibraryFunctionsChecker into separate files.

[martong]

In D106644#2913676, @balazske wrote:

Split some type lookup functions from StdLibraryFunctionsChecker into separate files.

Thank you, this is awesome!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

501

We should be careful, to cache the results (either here, or deeper in the call stack).
I mean, we certainly don't want to do a lookup of "stdin" every time a function is evaluated. We should do this lazily, only once.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/ASTLookup.cpp

26 ↗

(On Diff #362775)

Decl *D -> const Decl *D. Same for the other loop.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

224

You calculate this 3 times now. I mean it's not a big deal, but we could save this to do it only once.

226–235

It will early return and uses one fewer continue.

501

I agree. We should do this only for the first top-level function, instead of doing this for every top-level function.

551

It might be a personal preference but I think lambdas should be pure in the sense that it takes something and produces something else.
Regardless of your choice, the trailing return type would highlight it.

555

SValBuilder::getConditionType(), oh they are the same under the hood. We should still probably prefer this one instead.
It might worth hoisting C.getSValBuilder() to a local variable though.
The symbol for StdStream also deserves a separate variable IMO.

556–559

I think you should be fine with castAs(). I'm not expecting this to fail.

clang/test/Analysis/stream.c

276–277

How about checking this way instead?

clang_analyzer_eval(F != stdin);  // TRUE
clang_analyzer_eval(F != stdout); // TRUE
clang_analyzer_eval(F != stderr); // TRUE

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

226–235

The intent was that if no FILEType is found we find just the first VarDecl and do not check the type. This recommended change returns always null if no FILEType is found.

501

I am not sure if the SymbolRef values are safe to store between top-level function analyses. The FILE type and std stream declarations are safe to cache, but the SymbolRef values of these variables probably not.

[balazske]

Init std declarations and FILE type only once.
Make a lambda not operate on captured data.
Simplify test.

[steakhal]

Let's see if we can cache the stream symbols across top-level analyses.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

226–235

Hm, it was not immediately clear to me.

501

I think it should be safe. But place there an assert and run the test suite. If it won't trigger, that means that we have high confidentiality that this is safe. I know it's not 100%, but pretty close.

504

508

549

This should be probably at the definition of StdinSym and getStdStreamSym().

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

501

I tried to check if these SymbolRef's are the same at checkBeginFunction after initialized once. The assertion for equality failed sometimes, or other crash happened when dump was called on the value. So it looks not safe to cache these. And should we add assumptions about that these StdinSym, StdoutSym, StderrSym are not equal to each other?

555

This lambda is possible to improve.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

501

Good to know. I don't think it's necessary to add assertions.

[balazske]

Simplified a lambda usage.
Moved some functions, changed documentation.

[steakhal]

It's fine on my part.

[martong]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

501

Okay, so we should not cache the SymbolRefs then. But we must cache the VarDecls. I mean, we should avoid calling

StdinDecl = findStdStreamDecl("stdin", C);

more than once for each TU.

I think you could use and Optional for StdinDecl (and the others) to achieve this.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 //===-- StreamChecker.cpp -----------------------------------------*- C++ -*--//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines checkers that model and check stream handling functions.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ParentMapContext.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"

 };
 
 } // namespace
 
 //===----------------------------------------------------------------------===//
 // StreamChecker class and utility functions.
 //===----------------------------------------------------------------------===//
 
+// This map holds the state of a stream.
+// The stream is identified with a SymbolRef that is created when a stream
+// opening function is modeled by the checker.
+REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
+
 namespace {
 
 class StreamChecker;
 using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
 
 using ArgNoTy = unsigned int;
 static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();

 
 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, check::PointerEscape> {
+const VarDecl *findStdStreamDecl(StringRef StdName, CheckerContext &C) {
+  ASTContext &ACtx = C.getASTContext();
+
+  IdentifierInfo &II = ACtx.Idents.get(StdName);
+  auto LookupRes = ACtx.getTranslationUnitDecl()->lookup(&II);
+  QualType FILEType = ACtx.getFILEType();
+  if (!FILEType.isNull())
+    FILEType = ACtx.getPointerType(FILEType);
+
+  for (Decl *D : LookupRes) {
+    D = D->getCanonicalDecl();
+    if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+      continue;
+    if (auto *VD = dyn_cast<VarDecl>(D)) {
+      if (!FILEType.isNull() && !ACtx.hasSameType(FILEType, VD->getType()))
+        continue;
+      return VD;
+    }
+  }
+
+  return nullptr;
+}
+
+SymbolRef findStdStream(StringRef StdName, CheckerContext &C) {
+  const LocationContext *LCtx = C.getLocationContext();
+
+  const VarDecl *StdDecl = findStdStreamDecl(StdName, C);
+  if (!StdDecl)
+    return nullptr;
+
+  const VarRegion *RegionOfStdStreamVar =
+      C.getState()->getRegion(StdDecl, LCtx);
+  if (!RegionOfStdStreamVar)
+    return nullptr;
+  SVal StdVal = C.getState()->getSVal(RegionOfStdStreamVar, StdDecl->getType());
+  return StdVal.getAsSymbol();
+}
+
+class StreamChecker
+    : public Checker<check::BeginFunction, check::PreCall, eval::Call,
   BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
                                 "Stream handling error"};
   BugType BT_IndeterminatePosition{this, "Invalid stream state",
                                    "Stream handling error"};
   BugType BT_IllegalWhence{this, "Illegal whence argument",
                            "Stream handling error"};
   BugType BT_StreamEof{this, "Stream already in EOF", "Stream handling error"};
   BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
                           /*SuppressOnSink =*/true};
 
 public:
+  void checkBeginFunction(CheckerContext &C) const;
   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;
 
CallDescriptionMap<FnDescription> FnTestDescriptions = {
         0}},
       {{"StreamTesterChecker_make_ferror_stream", 1},
        {nullptr,
         std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
                   ErrorFError),
         0}},
   };
 
+  mutable SymbolRef OrigStdin;
+  mutable SymbolRef OrigStdout;
+  mutable SymbolRef OrigStderr;
+
   void evalFopen(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
 
   void preFreopen(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
   void evalFreopen(const FnDescription *Desc, const CallEvent &Call,
                    CheckerContext &C) const;
 
private:
   /// StreamSym.
   static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
                                                 SymbolRef StreamSym,
                                                 CheckerContext &C);
 };
 
 } // end anonymous namespace
 
-// This map holds the state of a stream.
-// The stream is identified with a SymbolRef that is created when a stream
-// opening function is modeled by the checker.
-REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
-
 inline void assertStreamStateOpened(const StreamState *SS) {
   assert(SS->isOpened() &&
          "Previous create of error node for non-opened stream failed?");
 }
 
+//===----------------------------------------------------------------------===//
+// Methods of StreamChecker.
+//===----------------------------------------------------------------------===//
+
 const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
                                                       SymbolRef StreamSym,
                                                       CheckerContext &C) {
   ProgramStateRef State = N->getState();
   // When bug type is resource leak, exploded node N may not have state info
   // for leaked file descriptor, but predecessor should have it.
   if (!State->get<StreamMap>(StreamSym))
     N = N->getFirstPred();
 
   const ExplodedNode *Pred = N;
   while (N) {
     State = N->getState();
     if (!State->get<StreamMap>(StreamSym))
       return Pred;
     Pred = N;
     N = N->getFirstPred();
   }
 
   return nullptr;
 }
 
-//===----------------------------------------------------------------------===//
-// Methods of StreamChecker.
-//===----------------------------------------------------------------------===//
+void StreamChecker::checkBeginFunction(CheckerContext &C) const {
+  if (!C.inTopFrame())
+    return;
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);
+  OrigStderr = findStdStream("stderr", C);
+}
 
 void StreamChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
     return;
 
   Desc->PreFn(this, Desc, Call, C);
 }
 
 bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
   const FnDescription *Desc = lookupFn(Call);
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
   // with a NULL.
   ProgramStateRef StateNotNull, StateNull;
   std::tie(StateNotNull, StateNull) =
       C.getConstraintManager().assumeDual(State, RetVal);
 
   StateNotNull =
       StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
   StateNull =
       StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
 
+  auto AssumeRetValNotEq = [&C, &StateNotNull, RetVal](SymbolRef StdStream) {
+    SVal NotEqS = C.getSValBuilder().evalBinOp(
+        StateNotNull, BO_NE, RetVal,
+        C.getSValBuilder().makeSymbolVal(StdStream),
+        C.getASTContext().getLogicalOperationType());
+    Optional<DefinedOrUnknownSVal> NotEqDef =
+        NotEqS.getAs<DefinedOrUnknownSVal>();
+    if (!NotEqDef)
+      return;
+    StateNotNull = StateNotNull->assume(*NotEqDef, true);
+  };
+  AssumeRetValNotEq(OrigStdin);
+  AssumeRetValNotEq(OrigStdout);
+  AssumeRetValNotEq(OrigStderr);
+
   C.addTransition(StateNotNull,
                   constructNoteTag(C, RetSym, {&BT_ResourceLeak}));
   C.addTransition(StateNull, constructNoteTag(C, RetSym, {&BT_FileNull}));
 }
 
 void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   // Do not allow NULL as passed stream pointer but allow a closed stream.

 void ento::registerStreamTesterChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.getChecker<StreamChecker>();
   Checker->TestMode = true;
 }
 
 bool ento::shouldRegisterStreamTesterChecker(const CheckerManager &Mgr) {
   return true;
 }
 No newline at end of file

clang/test/Analysis/stream.c

if (!F1)
     return;
   if (Test == 1) {
     return; // no warning
   }
   rewind(F1);
 } // expected-warning {{Opened stream never closed. Potential resource leak}}
 // FIXME: This warning should be placed at the `return` above.
 // See https://reviews.llvm.org/D83120 about details.
+
+void check_fopen_is_not_std(FILE *F, int Std) {
+  if (!Std)
+    F = fopen("file", "r");
+  if (!F)
+    return;
+  if (F != stdin && F != stdout && F != stderr)
+    fclose(F); // no warning: the opened file can not be equal to std streams
+}