[clang][analyzer] Using CallDescription in StreamChecker.

[balazske]

Summary

Recognization of function names is done now with the CallDescription
class instead of using IdentifierInfo. This means function name and
argument count is compared too.
A new check for filtering not global-C-functions was added.
Test was updated.

[NoQ]

Thanks! Woohoo, tests!

You can go one step further to have a CallDescriptionMap, like in D62557. This would replace the whole chain of ifs with a map lookup (which is currently still a chain of ifs under the hood, but a lot less code anyway and we'll probably optimize it under the hood later).

[balazske]

Using CallDescriptionMap.

[Szelethus]

It seems like this patch is diffed against your latest commit, not the master branch.

[NoQ]

In D67706#1685963, @Szelethus wrote:

It seems like this patch is diffed against your latest commit, not the master branch.

Yeah, seems so. The code looks great tho, thanks!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

57–58

Feel free to omit the constructor entirely. We almost never have constructors in our checkers.

65–78

Are you sure these functions are actually sometimes implemented as builtins? I think they're required to be regular functions.

81–97

For most purposes it's more convenient to pass CallEvent around. The only moment when you actually need the CallExpr is when you're doing the binding for the return value, which happens once, so it's fine to call .getOriginExpr() directly at this point.

112–114

(not your fault but before i forget) This check is actually redundant.

212

(not your fault but before i forget) This condition is actually always true here.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

81–97

The CallExpr is used at many places to get arguments and other data. There is a CallEvent::getArgSVal that can be used instead but at some places CallExpr is used in other ways. I do not see much benefit of change the passed CallExpr to CallEvent.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

81–97

at some places CallExpr is used in other ways

Can we add more convenient getters to CallEvent to help with those? 'Cause CallEvent has strictly more information than CallExpr (combining syntactic information with path-sensitive information), it's all about convenience.

I also see that CallExpr is stored in StreamState (which is something you can't do with a CallEvent because it's short-lived) but i suspect that it's actually never read from there and i doubt that it was the right solution anyway. I.e., no other checker needs that and i don't have a reason to believe that StreamChecker is special.

[balazske]

• Various code cleanups. Eval functions use CallEvent, CallExpr is removed from state.

[Szelethus]

A few nits inline, otherwise the patch is awesome, thank you!!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

62–104

Prefer using. When I wrote D68165, I spent about 10 minutes figuring out how to do it... Ah, the joys of the C++ syntax.

using FnCheck = void (StreamChecker::*)(const CallEvent &,
                                        CheckerContext &) const;

112–113

Isn't this redundant with my other inline about parameter types?

135–139

I'm not sure why we need this, is it true that *all* stream related functions return a pointer or a numerical value? Are we actually checking whether this really is a library function? If so, this looks pretty arbitrary.

154

Why the need for (void)? CheckNullSteam doesn't seem to have an LLVM_NODISCARD attribute.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

112–113

Probably change to isInSystemHeader or use both?

135–139

This comes from code of CStringChecker:

// Pro-actively check that argument types are safe to do arithmetic upon.
// We do not want to crash if someone accidentally passes a structure
// into, say, a C++ overload of any of these functions. We could not check
// that for std::copy because they may have arguments of other types.

Still I am not sure that the checker works correct with code that contains similar named but "arbitrary" functions.

154

I wanted to be sure to get no buildbot compile errors (where -Werror is used).

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

135–139

Oops, meant to write that ", is it true that *all* stream related functions have only pointer or a numerical parameters?".

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

112–113

Actually, this looks fine. How about preserving this...

135–139

...and removing this one, or changing it to an assert?

154

They actually break on this?? Let me guess, is it the windows one? :D

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

112–113

Should be the "stream functions" always in system header? If no isInSystemHeader call is used any global function called "fopen" with 2 arguments is recognized as stream opening function. If it returns a pointer and no "fclose" is called on that we will get a resource leak warning for that code. (But for this case the user will probably not enable the checker?).

135–139

This can not be an assert because it is possible to have global functions with same name but different signature. The check can be kept to filter out such cases. The wanted stream functions have only integral or enum or pointer arguments.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

62–104

It's actually very easy to remember, it's just an alien kiss smiley ::*)

135–139

Clang shouldn't crash even when the library definition is incorrect.

154

I'd rather not discard the return value, but allow callbacks indicate an error when something goes wrong, so that to allow them to abort evalCall().

But more importantly, note how CheckNullStream actually returns a ProgramStateRef that was meant to be transitioned into. Which means that the primary execution path actually gets cut off.

So even if buildbots didn't warn on this, i wish they did.

[balazske]

• Re-design of eval functions.
• Added a C++ test with fopen-looking function.

[balazske]

Ping

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

197–199

Why is there a need to use an Optional? Why not just return a nullptr? As I saw it, each time we check both whether the optional has a value and whether the state within it is null.

203–205

Aha, so we're no longer checking whether Sym is null, because why would we, we literally made created it as a symbol a couple lines up. Is it worth assert()ing though?

247–250

And here too?

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

197–199

The idea was that there are 3 kind of return values: Change to a new state, do not change to new state, or error was found. In the first and last case we should return true from evalCall but not if there is no state to change into and the analysis can proceed in default way.

203–205

Probably better to have the assert.

[balazske]

Ping again

[Charusso]

Could you please mark resolved issues as resolved? I would like to see what we miss, and what has been done.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

63

I prefer std::function, because it is modern.

using StreamCheck = 
    std::function<void(const StreamChecker *, const CallEvent &,
                       CheckerContext &)>;

I think it is fine with pointers, but at some point we need to modernize this.

66

Because evalFopen() is basically the OpenFileAux(Call, C);, I think we could simplify the API by removing unnecessary one-liners so here you could write {{"fopen"}, &StreamChecker::OpenFileAux, that is why the CallEvent parameter is so generic and useful.

101

This Optional is not necessary. When the state is changed, you can rely on CheckerContext::isDifferent() to see whether the modeling was succeeded. Therefore you could revert the bool return values as well.

120–123

Why do you inject this?

130

I would move this tiny identifyCall() into evalCall() as the purpose of evalCall() is the validation of the Callback in this case.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

158–159

This is getting pretty messy, given that you've already made a transition in this function. If you do addTransition and then generateNonFatalErrorNode, then you'll get two parallel successor nodes, but you only need one. You should either delay the transition that happens immediately after CheckNullStream, or chain the two transitions together sequentially (as opposed to in parallel).

[balazske]

• Redesign again.

[Szelethus]

Its becoming a bit difficult to navigate inlines, could you please mark them as done as you address them?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

63

But its also a lot more expensive. https://blog.demofox.org/2015/02/25/avoiding-the-performance-hazzards-of-stdfunction/

std::function is able to wrap lambdas with different captures and all sorts of things like that, which comes at a cost.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

63

Now std::function and std::bind is used. Probably more expensive but it is called once in a evalCall, that should be no problem?

81–97

In the new code CallEvent is passed.

101

Optional is not used now. The functions only return if the passed state was modified (if applicable). To detect if error was generated the isDifferent is used.

130

identifyCall is removed now.

158–159

evalFseek has now a transition to new (non-null stream) state or transition to (non-fatal) error.

[NoQ]

Let's land this patch, given that it grew way too big for a refactoring pass, and let @balazske further clean things up later in his follow-up patches. This is already a large improvement and i'm very grateful!

For the Optional vs. bool debate: let's see. For now we have roughly the following:

bool checkNullStream(..., ProgramStateRef *State) {
  if (!NonBuggyState && BuggyState) {
    C.emitReport(..., C.generateErrorNode(BuggyState));
    return false;
  }

  if (NonBuggyState) {
    *State = NonBuggyState;
    return true;
  }

  return false;
}

void checkFseekWhence(..., ProgramStateRef *State) const {
  if (!isBuggy(State))
    return;

  C.emitReport(..., C.generateNonFatalErrorNode(State));
}

void evalFseek(...) {
  ProgramStateRef State = C.getState();
  bool StateChanged = checkNullStream(..., &State);
  if (C.isDifferent())
    return;

  checkFseekWhence(..., &State);
  if (!C.isDifferent() && StateChanged)
    C.addTransition(State);
}

In this code functions checkNullStream and checkFseekWhence are very similar. Both of them do some checking and potentially emit a report. In high-level terms, all you're trying to do is conduct two checks at the same program point.

How did these two functions end up being so completely different? What will you do if you have not 2 but, say, 10 different checks that you need to conduct? That's not a random question; checkers do tend to grow this way, so i'm pretty sure you'll eventually need to answer this question.

In ExprEngine, the class that's responsible for modeling all statements that have effect on the program state, we have a very common idiom for chaining pieces of work together. It looks roughly like this:

void modelSomethingPart1(ExplodedNode *Pred, set<ExplodedNode> *Succs);
void modelSomethingPart2(ExplodedNode *Pred, set<ExplodedNode> *Succs);
void modelSomethingPart3(ExplodedNode *Pred, set<ExplodedNode> *Succs);
...
void modelSomethingPartK(ExplodedNode *Pred, set<ExplodedNode> *Succs);


void modelSomething(ExplodedNode *Pred) {
  set<ExplodedNode> Set0 = { Pred };

  set<ExplodedNode> Set1;
  for (P : Set0)
    modelSomethingPart1(P, &Set1);
  
  set<ExplodedNode> Set2;
  for (P : Set1)
    modelSomethingPart2(P, &Set2);
  
  set<ExplodedNode> Set3;
  for (P : Set2)
    modelSomethingPart3(P, &Set3);

  ...

  set<ExplodedNode> SetK;
  for (P : SetKMinus1)
    modelSomethingPartK(P, &SetK);

  for (N : SetK)
    addTransition(N);
}

As you see, it scales nicely for any number of sub-tasks and you don't need to juggle boolean flags or optionals when you add another sub-task. Moreover, you can further split each of your sub-tasks into even smaller sub-sub-tasks in a similar manner.

In ExprEngine this idiom is completely essential due to how arbitrary most of the effects-of-statements tend to be. In checkers, however, this level of complexity has never been necessary so far because most sub-methods either produce exactly one new state, or sink completely. For checkers like this one, which do multiple checks, i recommend passing a single node around:

ExplodedNode *checkNullStream(..., ExplodedNode *Pred) {
  if (!NonBuggyState && BuggyState)
    C.emitReport(..., C.generateErrorNode(BuggyState, Pred));

  return C.addTransition(NonBuggyState, Pred);
}

ExplodedNode *checkFseekWhence(..., ExplodedNode *Pred) const {
  State = Pred->getState();
  if (!isBuggy(State))
    return Pred;
  ExplodedNode *N = C.generateNonFatalErrorNode(State);
  if (N)
    C.emitReport(..., N);
  return N;
}

void evalFseek(...) {
  ExplodedNode *N = C.getPredecessor();

  N = checkNullStream(..., N);
  if (!N)
    return;

  N = checkFseekWhence(..., N);
  if (!N)
    return;

  N = checkSomethingElse1(..., N);
  if (!N)
    return;

  N = checkSomethingElse2(..., N);
  if (!N)
    return;

  ...

  checkSomethingElseK(..., N);
}

This wastes exploded nodes a little bit but that's fine. It's much less scary than accidentally adding unnecessary state splits.

[Charusso]

My concern was the too heavy Optional and bool usage. Cool patch!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

63

There is no real overhead, yes.

[Szelethus]

Let's make it 3! Thank you so much for sticking by, I guess one of the reasons why a patch so obviously great and desired took so long is that we still didn't figure out how we imagine the CallDescriptionMap to be integrated into larger checkers :) In any case, this is a major step in the right direction, MallocChecker and some of the others should take notes.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 #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"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include <functional>
 
 using namespace clang;
 using namespace ento;
+using namespace std::placeholders;
 
 namespace {
 
 struct StreamState {
   enum Kind { Opened, Closed, OpenFailed, Escaped } K;
-  const Stmt *S;
 
-  StreamState(Kind k, const Stmt *s) : K(k), S(s) {}
+  StreamState(Kind k) : K(k) {}
 
   bool isOpened() const { return K == Opened; }
   bool isClosed() const { return K == Closed; }
   //bool isOpenFailed() const { return K == OpenFailed; }
   //bool isEscaped() const { return K == Escaped; }
 
-  bool operator==(const StreamState &X) const {
-    return K == X.K && S == X.S;
-  }
+  bool operator==(const StreamState &X) const { return K == X.K; }
 
-  static StreamState getOpened(const Stmt *s) { return StreamState(Opened, s); }
-  static StreamState getClosed(const Stmt *s) { return StreamState(Closed, s); }
-  static StreamState getOpenFailed(const Stmt *s) {
-    return StreamState(OpenFailed, s);
-  }
-  static StreamState getEscaped(const Stmt *s) {
-    return StreamState(Escaped, s);
-  }
+  static StreamState getOpened() { return StreamState(Opened); }
+  static StreamState getClosed() { return StreamState(Closed); }
+  static StreamState getOpenFailed() { return StreamState(OpenFailed); }
+  static StreamState getEscaped() { return StreamState(Escaped); }
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(K);
-    ID.AddPointer(S);
   }
 };
 
 class StreamChecker : public Checker<eval::Call,
                                      check::DeadSymbols > {
-  mutable IdentifierInfo *II_fopen, *II_tmpfile, *II_fclose, *II_fread,
-                 *II_fwrite,
-                 *II_fseek, *II_ftell, *II_rewind, *II_fgetpos, *II_fsetpos,
-                 *II_clearerr, *II_feof, *II_ferror, *II_fileno;
   mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
       BT_doubleclose, BT_ResourceLeak;
 
 public:
-  StreamChecker()
-    : II_fopen(nullptr), II_tmpfile(nullptr), II_fclose(nullptr),
-      II_fread(nullptr), II_fwrite(nullptr), II_fseek(nullptr),
-      II_ftell(nullptr), II_rewind(nullptr), II_fgetpos(nullptr),
-      II_fsetpos(nullptr), II_clearerr(nullptr), II_feof(nullptr),
-      II_ferror(nullptr), II_fileno(nullptr) {}
-
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 
 private:
-  void Fopen(CheckerContext &C, const CallExpr *CE) const;
-  void Tmpfile(CheckerContext &C, const CallExpr *CE) const;
-  void Fclose(CheckerContext &C, const CallExpr *CE) const;
-  void Fread(CheckerContext &C, const CallExpr *CE) const;
-  void Fwrite(CheckerContext &C, const CallExpr *CE) const;
-  void Fseek(CheckerContext &C, const CallExpr *CE) const;
-  void Ftell(CheckerContext &C, const CallExpr *CE) const;
-  void Rewind(CheckerContext &C, const CallExpr *CE) const;
-  void Fgetpos(CheckerContext &C, const CallExpr *CE) const;
-  void Fsetpos(CheckerContext &C, const CallExpr *CE) const;
-  void Clearerr(CheckerContext &C, const CallExpr *CE) const;
-  void Feof(CheckerContext &C, const CallExpr *CE) const;
-  void Ferror(CheckerContext &C, const CallExpr *CE) const;
-  void Fileno(CheckerContext &C, const CallExpr *CE) const;
-
-  void OpenFileAux(CheckerContext &C, const CallExpr *CE) const;
-
-  ProgramStateRef CheckNullStream(SVal SV, ProgramStateRef state,
-                                 CheckerContext &C) const;
-  ProgramStateRef CheckDoubleClose(const CallExpr *CE, ProgramStateRef state,
-                                 CheckerContext &C) const;
+  using FnCheck = std::function<void(const StreamChecker *, const CallEvent &,
+                                     CheckerContext &)>;
+
+  CallDescriptionMap<FnCheck> Callbacks = {
+      {{"fopen"}, &StreamChecker::evalFopen},
+      {{"tmpfile"}, &StreamChecker::evalFopen},
+      {{"fclose", 1}, &StreamChecker::evalFclose},
+      {{"fread", 4},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 3)},
+      {{"fwrite", 4},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 3)},
+      {{"fseek", 3}, &StreamChecker::evalFseek},
+      {{"ftell", 1},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+      {{"rewind", 1},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+      {{"fgetpos", 2},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+      {{"fsetpos", 2},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+      {{"clearerr", 1},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+      {{"feof", 1},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+      {{"ferror", 1},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+      {{"fileno", 1},
+       std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)},
+  };
+
+  void evalFopen(const CallEvent &Call, CheckerContext &C) const;
+  void evalFclose(const CallEvent &Call, CheckerContext &C) const;
+  void evalFseek(const CallEvent &Call, CheckerContext &C) const;
+
+  void checkArgNullStream(const CallEvent &Call, CheckerContext &C,
+                          unsigned ArgI) const;
+  bool checkNullStream(SVal SV, CheckerContext &C,
+                       ProgramStateRef &State) const;
+  void checkFseekWhence(SVal SV, CheckerContext &C,
+                        ProgramStateRef &State) const;
+  bool checkDoubleClose(const CallEvent &Call, CheckerContext &C,
+                        ProgramStateRef &State) const;
 };
 
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
 
 
 bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
   const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FD || FD->getKind() != Decl::Function)
     return false;
 
-  const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
+  // Recognize "global C functions" with only integral or pointer arguments
+  // (and matching name) as stream functions.
+  if (!Call.isGlobalCFunction())
+    return false;
+  for (auto P : Call.parameters()) {
+    QualType T = P->getType();
+    if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
       return false;
-
-  ASTContext &Ctx = C.getASTContext();
-  if (!II_fopen)
-    II_fopen = &Ctx.Idents.get("fopen");
-  if (!II_tmpfile)
-    II_tmpfile = &Ctx.Idents.get("tmpfile");
-  if (!II_fclose)
-    II_fclose = &Ctx.Idents.get("fclose");
-  if (!II_fread)
-    II_fread = &Ctx.Idents.get("fread");
-  if (!II_fwrite)
-    II_fwrite = &Ctx.Idents.get("fwrite");
-  if (!II_fseek)
-    II_fseek = &Ctx.Idents.get("fseek");
-  if (!II_ftell)
-    II_ftell = &Ctx.Idents.get("ftell");
-  if (!II_rewind)
-    II_rewind = &Ctx.Idents.get("rewind");
-  if (!II_fgetpos)
-    II_fgetpos = &Ctx.Idents.get("fgetpos");
-  if (!II_fsetpos)
-    II_fsetpos = &Ctx.Idents.get("fsetpos");
-  if (!II_clearerr)
-    II_clearerr = &Ctx.Idents.get("clearerr");
-  if (!II_feof)
-    II_feof = &Ctx.Idents.get("feof");
-  if (!II_ferror)
-    II_ferror = &Ctx.Idents.get("ferror");
-  if (!II_fileno)
-    II_fileno = &Ctx.Idents.get("fileno");
-
-  if (FD->getIdentifier() == II_fopen) {
-    Fopen(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_tmpfile) {
-    Tmpfile(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_fclose) {
-    Fclose(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_fread) {
-    Fread(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_fwrite) {
-    Fwrite(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_fseek) {
-    Fseek(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_ftell) {
-    Ftell(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_rewind) {
-    Rewind(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_fgetpos) {
-    Fgetpos(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_fsetpos) {
-    Fsetpos(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_clearerr) {
-    Clearerr(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_feof) {
-    Feof(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_ferror) {
-    Ferror(C, CE);
-    return true;
-  }
-  if (FD->getIdentifier() == II_fileno) {
-    Fileno(C, CE);
-    return true;
   }
 
+  const FnCheck *Callback = Callbacks.lookup(Call);
+  if (!Callback)
     return false;
-}
 
-void StreamChecker::Fopen(CheckerContext &C, const CallExpr *CE) const {
-  OpenFileAux(C, CE);
-}
+  (*Callback)(this, Call, C);
 
-void StreamChecker::Tmpfile(CheckerContext &C, const CallExpr *CE) const {
-  OpenFileAux(C, CE);
+  return C.isDifferent();
 }
 
-void StreamChecker::OpenFileAux(CheckerContext &C, const CallExpr *CE) const {
+void StreamChecker::evalFopen(const CallEvent &Call, CheckerContext &C) const {
   ProgramStateRef state = C.getState();
   SValBuilder &svalBuilder = C.getSValBuilder();
   const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
-  DefinedSVal RetVal = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx,
-                                                    C.blockCount())
+  auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
+  DefinedSVal RetVal =
+      svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount())
           .castAs<DefinedSVal>();
   state = state->BindExpr(CE, C.getLocationContext(), RetVal);
 
   ConstraintManager &CM = C.getConstraintManager();
   // Bifurcate the state into two: one with a valid FILE* pointer, the other
   // with a NULL.
   ProgramStateRef stateNotNull, stateNull;
   std::tie(stateNotNull, stateNull) = CM.assumeDual(state, RetVal);
 
-  if (SymbolRef Sym = RetVal.getAsSymbol()) {
-    // if RetVal is not NULL, set the symbol's state to Opened.
-    stateNotNull =
-      stateNotNull->set<StreamMap>(Sym,StreamState::getOpened(CE));
-    stateNull =
-      stateNull->set<StreamMap>(Sym, StreamState::getOpenFailed(CE));
+  SymbolRef Sym = RetVal.getAsSymbol();
+  assert(Sym && "RetVal must be a symbol here.");
+  stateNotNull = stateNotNull->set<StreamMap>(Sym, StreamState::getOpened());
+  stateNull = stateNull->set<StreamMap>(Sym, StreamState::getOpenFailed());
 
-    C.addTransition(stateNotNull);
-    C.addTransition(stateNull);
-  }
-}
 
-void StreamChecker::Fclose(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = CheckDoubleClose(CE, C.getState(), C);
-  if (state)
-    C.addTransition(state);
+void StreamChecker::evalFclose(const CallEvent &Call, CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  if (checkDoubleClose(Call, C, State))
+    C.addTransition(State);
 }
 
-void StreamChecker::Fread(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(3)), state, C))
+void StreamChecker::evalFseek(const CallEvent &Call, CheckerContext &C) const {
+  const Expr *AE2 = Call.getArgExpr(2);
+  if (!AE2)
     return;
-}
 
-void StreamChecker::Fwrite(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(3)), state, C))
-    return;
-}
+  ProgramStateRef State = C.getState();
 
-void StreamChecker::Fseek(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!(state = CheckNullStream(C.getSVal(CE->getArg(0)), state, C)))
+  bool StateChanged = checkNullStream(Call.getArgSVal(0), C, State);
+  // Check if error was generated.
+  if (C.isDifferent())
     return;
+
   // Check the legality of the 'whence' argument of 'fseek'.
-  SVal Whence = state->getSVal(CE->getArg(2), C.getLocationContext());
-  Optional<nonloc::ConcreteInt> CI = Whence.getAs<nonloc::ConcreteInt>();
+  checkFseekWhence(State->getSVal(AE2, C.getLocationContext()), C, State);
 
-  if (!CI)
-    return;
+  if (!C.isDifferent() && StateChanged)
+    C.addTransition(State);
 
-  int64_t x = CI->getValue().getSExtValue();
-  if (x >= 0 && x <= 2)
-    return;
-
-  if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) {
-    if (!BT_illegalwhence)
-      BT_illegalwhence.reset(
-          new BuiltinBug(this, "Illegal whence argument",
-                         "The whence argument to fseek() should be "
-                         "SEEK_SET, SEEK_END, or SEEK_CUR."));
-    C.emitReport(std::make_unique<PathSensitiveBugReport>(
-        *BT_illegalwhence, BT_illegalwhence->getDescription(), N));
-  }
 }
 
-void StreamChecker::Ftell(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
-    return;
+void StreamChecker::checkArgNullStream(const CallEvent &Call, CheckerContext &C,
+                                       unsigned ArgI) const {
+  ProgramStateRef State = C.getState();
+  if (checkNullStream(Call.getArgSVal(ArgI), C, State))
+    C.addTransition(State);
 }
 
-void StreamChecker::Rewind(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
-    return;
-}
+bool StreamChecker::checkNullStream(SVal SV, CheckerContext &C,
+                                    ProgramStateRef &State) const {
+  Optional<DefinedSVal> DV = SV.getAs<DefinedSVal>();
+  if (!DV)
+    return false;
 
-void StreamChecker::Fgetpos(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
-    return;
-}
+  ConstraintManager &CM = C.getConstraintManager();
+  ProgramStateRef StateNotNull, StateNull;
+  std::tie(StateNotNull, StateNull) = CM.assumeDual(C.getState(), *DV);
 
-void StreamChecker::Fsetpos(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
-    return;
+  if (!StateNotNull && StateNull) {
+    if (ExplodedNode *N = C.generateErrorNode(StateNull)) {
+      if (!BT_nullfp)
+        BT_nullfp.reset(new BuiltinBug(this, "NULL stream pointer",
+                                       "Stream pointer might be NULL."));
+      C.emitReport(std::make_unique<PathSensitiveBugReport>(
+          *BT_nullfp, BT_nullfp->getDescription(), N));
     }
-
-void StreamChecker::Clearerr(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
-    return;
+    return false;
   }
 
-void StreamChecker::Feof(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
-    return;
+  if (StateNotNull) {
+    State = StateNotNull;
+    return true;
   }
 
-void StreamChecker::Ferror(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
-    return;
+  return false;
 }
 
-void StreamChecker::Fileno(CheckerContext &C, const CallExpr *CE) const {
-  ProgramStateRef state = C.getState();
-  if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C))
+void StreamChecker::checkFseekWhence(SVal SV, CheckerContext &C,
+                                     ProgramStateRef &State) const {
+  Optional<nonloc::ConcreteInt> CI = SV.getAs<nonloc::ConcreteInt>();
+  if (!CI)
     return;
-}
-
-ProgramStateRef StreamChecker::CheckNullStream(SVal SV, ProgramStateRef state,
-                                    CheckerContext &C) const {
-  Optional<DefinedSVal> DV = SV.getAs<DefinedSVal>();
-  if (!DV)
-    return nullptr;
 
-  ConstraintManager &CM = C.getConstraintManager();
-  ProgramStateRef stateNotNull, stateNull;
-  std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
+  int64_t X = CI->getValue().getSExtValue();
+  if (X >= 0 && X <= 2)
+    return;
 
-  if (!stateNotNull && stateNull) {
-    if (ExplodedNode *N = C.generateErrorNode(stateNull)) {
-      if (!BT_nullfp)
-        BT_nullfp.reset(new BuiltinBug(this, "NULL stream pointer",
-                                       "Stream pointer might be NULL."));
-      C.emitReport(std::make_unique<PathSensitiveBugReport>(
-          *BT_nullfp, BT_nullfp->getDescription(), N));
-    }
-    return nullptr;
+  if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+    if (!BT_illegalwhence)
+      BT_illegalwhence.reset(
+          new BuiltinBug(this, "Illegal whence argument",
+                         "The whence argument to fseek() should be "
+                         "SEEK_SET, SEEK_END, or SEEK_CUR."));
+        *BT_illegalwhence, BT_illegalwhence->getDescription(), N));
   }
-  return stateNotNull;
 }
 
-ProgramStateRef StreamChecker::CheckDoubleClose(const CallExpr *CE,
-                                               ProgramStateRef state,
-                                               CheckerContext &C) const {
-  SymbolRef Sym = C.getSVal(CE->getArg(0)).getAsSymbol();
+bool StreamChecker::checkDoubleClose(const CallEvent &Call, CheckerContext &C,
+                                     ProgramStateRef &State) const {
+  SymbolRef Sym = Call.getArgSVal(0).getAsSymbol();
   if (!Sym)
-    return state;
+    return false;
 
-  const StreamState *SS = state->get<StreamMap>(Sym);
+  const StreamState *SS = State->get<StreamMap>(Sym);
 
   // If the file stream is not tracked, return.
   if (!SS)
-    return state;
+    return false;
 
   // Check: Double close a File Descriptor could cause undefined behaviour.
   // Conforming to man-pages
   if (SS->isClosed()) {
     ExplodedNode *N = C.generateErrorNode();
     if (N) {
       if (!BT_doubleclose)
         BT_doubleclose.reset(new BuiltinBug(
             this, "Double fclose", "Try to close a file Descriptor already"
                                    " closed. Cause undefined behaviour."));
       C.emitReport(std::make_unique<PathSensitiveBugReport>(
           *BT_doubleclose, BT_doubleclose->getDescription(), N));
     }
-    return nullptr;
+    return false;
   }
 
   // Close the File Descriptor.
-  return state->set<StreamMap>(Sym, StreamState::getClosed(CE));
+  State = State->set<StreamMap>(Sym, StreamState::getClosed());
+
+  return true;
 }
 
 void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                      CheckerContext &C) const {
-  ProgramStateRef state = C.getState();
+  ProgramStateRef State = C.getState();
 
   // TODO: Clean up the state.
-  const StreamMapTy &Map = state->get<StreamMap>();
+  const StreamMapTy &Map = State->get<StreamMap>();
   for (const auto &I: Map) {
     SymbolRef Sym = I.first;
     const StreamState &SS = I.second;
     if (!SymReaper.isDead(Sym) || !SS.isOpened())
       continue;
 
     ExplodedNode *N = C.generateErrorNode();
     if (!N)
-      return;
+      continue;
 
     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));
   }

clang/test/Analysis/stream.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__(sizeof(int)) fpos_t;
 typedef struct _IO_FILE FILE;
 #define SEEK_SET  0 /* Seek from beginning of file.  */
 #define SEEK_CUR  1 /* Seek from current position.  */
 #define SEEK_END  2 /* Seek from end of file.  */
 extern FILE *fopen(const char *path, const char *mode);
 extern FILE *tmpfile(void);
 extern int fclose(FILE *fp);
 extern size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
+extern size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
 extern int fseek (FILE *__stream, long int __off, int __whence);
 extern long int ftell (FILE *__stream);
 extern void rewind (FILE *__stream);
+extern int fgetpos(FILE *stream, fpos_t *pos);
+extern int fsetpos(FILE *stream, const fpos_t *pos);
+extern void clearerr(FILE *stream);
+extern int feof(FILE *stream);
+extern int ferror(FILE *stream);
+extern int fileno(FILE *stream);
 
-void f1(void) {
-  FILE *p = fopen("foo", "r");
-  char buf[1024];
-  fread(buf, 1, 1, p); // expected-warning {{Stream pointer might be NULL}}
-  fclose(p);
+void check_fread() {
+  FILE *fp = tmpfile();
+  fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
 }
 
-void f2(void) {
-  FILE *p = fopen("foo", "r");
-  fseek(p, 1, SEEK_SET); // expected-warning {{Stream pointer might be NULL}}
-  fclose(p);
+void check_fwrite() {
+  FILE *fp = tmpfile();
+  fwrite(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
 }
 
-void f3(void) {
-  FILE *p = fopen("foo", "r");
-  ftell(p); // expected-warning {{Stream pointer might be NULL}}
-  fclose(p);
+void check_fseek() {
+  FILE *fp = tmpfile();
+  fseek(fp, 0, 0); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_ftell() {
+  FILE *fp = tmpfile();
+  ftell(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_rewind() {
+  FILE *fp = tmpfile();
+  rewind(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_fgetpos() {
+  FILE *fp = tmpfile();
+  fpos_t pos;
+  fgetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_fsetpos() {
+  FILE *fp = tmpfile();
+  fpos_t pos;
+  fsetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_clearerr() {
+  FILE *fp = tmpfile();
+  clearerr(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_feof() {
+  FILE *fp = tmpfile();
+  feof(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void check_ferror() {
+  FILE *fp = tmpfile();
+  ferror(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
 }
 
-void f4(void) {
+void check_fileno() {
+  FILE *fp = tmpfile();
+  fileno(fp); // expected-warning {{Stream pointer might be NULL}}
+  fclose(fp);
+}
+
+void f_open(void) {
   FILE *p = fopen("foo", "r");
-  rewind(p); // expected-warning {{Stream pointer might be NULL}}
+  char buf[1024];
+  fread(buf, 1, 1, p); // expected-warning {{Stream pointer might be NULL}}
   fclose(p);
 }
 
-void f5(void) {
+void f_seek(void) {
   FILE *p = fopen("foo", "r");
   if (!p)
     return;
   fseek(p, 1, SEEK_SET); // no-warning
   fseek(p, 1, 3); // expected-warning {{The whence argument to fseek() should be SEEK_SET, SEEK_END, or SEEK_CUR}}
   fclose(p);
 }
 
-void f6(void) {
+void f_double_close(void) {
   FILE *p = fopen("foo", "r");
   fclose(p); 
   fclose(p); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}}
 }
 
-void f7(void) {
-  FILE *p = tmpfile();
-  ftell(p); // expected-warning {{Stream pointer might be NULL}}
-  fclose(p);
-}
-
-void f8(int c) {
+void f_leak(int c) {
   FILE *p = fopen("foo.c", "r");
   if(c)
     return; // expected-warning {{Opened File never closed. Potential Resource leak}}
   fclose(p);
 }
 
-FILE *f9(void) {
+FILE *f_null_checked(void) {
   FILE *p = fopen("foo.c", "r");
   if (p)
     return p; // no-warning
   else
     return 0;
 }
 
 void pr7831(FILE *fp) {
   fclose(fp); // no-warning
 }
 
 // PR 8081 - null pointer crash when 'whence' is not an integer constant
 void pr8081(FILE *stream, long offset, int whence) {
   fseek(stream, offset, whence);
 }
-

clang/test/Analysis/stream.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
+
+typedef struct _IO_FILE FILE;
+extern FILE *fopen(const char *path, const char *mode);
+
+struct X {
+  int A;
+  int B;
+};
+
+void *fopen(X x, const char *mode) {
+  return new char[4];
+}
+
+void f1() {
+  X X1;
+  void *p = fopen(X1, "oo");
+} // no-warning
+
+void f2() {
+  FILE *f = fopen("file", "r");
+} // expected-warning {{Opened File never closed. Potential Resource leak}}