[analyzer] Improved check of `fgetc` in StreamChecker.

[balazske]

Summary

A warning is produced if the getc call fails (returns EOF) and
the next called stream function is not feof and ferror
(both are needed).
This check is inspired by CERT rule FIO34-C "Distinguish between characters read from a file and EOF or WEOF".
https://wiki.sei.cmu.edu/confluence/display/c/FIO34-C.+Distinguish+between+characters+read+from+a+file+and+EOF+or+WEOF

Other similar functions (and wide versions) are to be added.

[martong]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

356

Maybe adding a comment like false /*Feof*/ could make this more readable. Even better, using two different strong types instead of the bool args could make it even more safe, but if that's too much hassle then it is may be not worth (I don't know if LLVM has a proper infrastructure for strong types).

clang/test/Analysis/stream.c

182

Do you have tests for getc as well? Maybe we could have at least one for getc too.

218

Perhaps Should call ferror is more informative for the readers of the tests.

[Szelethus]

I'm a bit confused as to what this patch aims to do. Once again, I'd kindly ask you to discuss for a bit before updating this patch.

---

The rule states that after reaching EOF both ferror and feof should be called. I'm a bit confused here -- isn't this problem a property of EOF rather then getc() specifically? Also, expecting each maintainer of this code to call checkErrorState whenever a new stream function is implemented as well as keeping in mind that state transitions should be made against a new ExplodedNode makes me think that we could do better.

I think we should do some ground work before progressing further. The checker currently doesn't implement the state where we know that the stream is an error state (we got EOF from it). Shouldn't we do that first? After that, the rule you want to enforce may be more appropriate to originate from checkDeadSymbols when a stream in said state isn't checked properly, which seems to be a lot more in line with the rule.

---

The test file contains tests where EOF isn't encountered once but we're emitting a warning that ferror and feof should've been called, yet the rule states that

Calling both functions on each iteration of a loop adds significant overhead, so a good strategy is to temporarily trust EOF and WEOF within the loop but verify them with feof() and ferror() following the loop.

Could we have the test cases from the CERT site, or something similar with a loop?

---

One thing that worries me is compliant non-portable solution:

#include <assert.h>
#include <stdio.h>
#include <limits.h>
 
void func(void) {
  int c;
  static_assert(UCHAR_MAX < UINT_MAX, "FIO34-C violation");
 
  do {
    c = getchar();
  } while (c != EOF);
}

We would totally emit errors for this. There are some solutions to this:

• Assume that the user will turn off this checker in this case.
• Check the TranslationUnitDecl for static asserts. If not present, suggest to put it in. This would make this warning more appropriate to come from the portability package.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

255–261

I'm 99% sure that the Preprocessor is retrievable in the checker registry function (register*Checker), and this code should be moved there, and EOFv be made non-mutable.

clang/test/Analysis/stream.c

188

Are we sure that this is the error message we want to emit here? Sure, not checking whether the stream is in an error state is a recipe for disaster, but this seems to clash with

Should call 'feof' and 'ferror' after failed character read.

as error here is not even checking the return value. Shouldn't we say

Should check the return value of fgetc before ...

instead?

246–247

I bet this isn't how we envision how these function to be used. Maybe ReturnValueChecker could be deployed here? In any case, that would be a topic of a different revision.

[balazske]

An improvement can be made if the check runs only when size of char and int are equal. And it is possible to avoid the warning if fgetc is called after fgetc (with no functions in between). (More complicated thing is to test for loop construct and maybe not more efficient.)

The code can be improved somewhat but there are now too many special cases and there is already a default eval function. Probably it is better to have every "check" function take and return ExplodedNode instead of state.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

255–261

I am not sure if this would be good. The Preprocessor is not available easily and the "register" function is probably there to register a checker and do nothing else. Probably the source code is not even known when it is called. And how to pass the EOF value to the created checker in the register function (if not a static variable is used)?

clang/test/Analysis/stream.c

182

getc is not done yet. It can need more change in StreamChecker to handle functions on std streams (at least stdin).

188

No, it is not enough to check the return value of fgetc, exactly this is the reason for the checker. "Should call 'feof' and 'ferror' after failed character read." is more exact: The read was suspected to fail because EOF was returned but should call feof (...) to verify it. A better message can be:

If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error.

246–247

If fgetc returns non-EOF we can be sure that there is no error, so no need to call ferror and feof. If it returns EOF we must "double-check" that it was a real error or an EOF-looking character that was read.

[NoQ]

In D75045#1891032, @balazske wrote:

An improvement can be made if the check runs only when size of char and int are equal.

How many such platforms can you name? Please correct me if i'm wrong but it sounds like we're adding a lot of logic to maintain for a very very low-value check.

[NoQ]

In D75045#1891055, @NoQ wrote:

How many such platforms can you name?

I mean, does Clang even support such targets?

[balazske]

In D75045#1891056, @NoQ wrote:

In D75045#1891055, @NoQ wrote:

How many such platforms can you name?

I mean, does Clang even support such targets?

The problem can occur with the "wide" variants of the getc functions when wchar_t is same width as wint_t, this can be more often (but I am not sure). Even then the character set should contain a character similar to WEOF, that is not true for UTF-16 and 32. So there may be low probability for the problem to occur. Is this reason to not have this check at all?

[Szelethus]

In D75045#1891064, @balazske wrote:

In D75045#1891056, @NoQ wrote:

In D75045#1891055, @NoQ wrote:

How many such platforms can you name?

I mean, does Clang even support such targets?

The problem can occur with the "wide" variants of the getc functions when wchar_t is same width as wint_t, this can be more often (but I am not sure). Even then the character set should contain a character similar to WEOF, that is not true for UTF-16 and 32. So there may be low probability for the problem to occur. Is this reason to not have this check at all?

If we can detect that the code is non-portable, it doesn't really matter in my opinion if we don't support the target on which the problem would occur. Also, I can't come up with a specific platform that would have an issue like this (maybe in a dishwasher? (: ), but CERT is known to create rules based on real vulnerabilities. A quick glance lead me to this page, though these vulnerabilities may not be directly related to this rule:

https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO/IECTS17961-2013

In D75045#1891032, @balazske wrote:

An improvement can be made if the check runs only when size of char and int are equal.

Since the rule specifically states that the problem here is portability related, I think we shouldn't do that.

The code can be improved somewhat but there are now too many special cases and there is already a default eval function. Probably it is better to have every "check" function take and return ExplodedNode instead of state.

This really ties into my main concern, that this isn't a stream management function specific problem, but rather an unchecked stream problem after we know that it returns EOF. I think we should first emit warnings on operations on EOF-returning streams, and implement this rule after that. WDYT?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

255–261

CheckerManager::registerChecker in fact returns the non-const checker object. The registry function is responsible for the registration and initialization of the checker object.

Though you have a point in the sense that this isn't terribly well defined (I should really finish D58065...).

I had a look, and my usual motto of "Any manager can be retrieved in clang at arbitrary locations if you try hard enough" seems to have been wrong. I don't see how I could get my hands on a Preprocessor in the checker registry function. So, feel free to disregard this comment.

clang/test/Analysis/stream.c

188

The compliant solution in the rule description has the following example:

#include <stdio.h>
 
void func(void) {
  int c;
 
  do {
    c = getchar();
  } while (c != EOF);
  if (feof(stdin)) {
    /* Handle end of file */
  } else if (ferror(stdin)) {
    /* Handle file error */
  } else {
    /* Received a character that resembles EOF; handle error */
  }
}

because if the character resembles EOF that is an error too, and also

Calling both functions on each iteration of a loop adds significant overhead, so a good strategy is to temporarily trust EOF and WEOF within the loop but verify them with feof() and ferror() following the loop.

Which makes me thing that the error here is not checking against EOF, first and foremost.

The warning message

If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error.

sounds great, I would just prefer to see it after we know that the stream returned EOF.

246–247

Yea, but shouldn't we check the return values of ferror and feof? Otherwise whats the point?

[balazske]

So, I think a different approach can be to store the stream state for the stream instead of ShouldCallX variables. (The state can be no-error, eof-error, other-error.) Optionally a warning can be made if any (modeled) stream function is called and the stream is in error state (and handle feof and ferror specially). (This is optional because it is not necessary to check the error always, it should not cause crash or undefined behavior. If a function is called in already failed stream state, it should simply fail again, or even not. Retrying a read for example can be correct. To detect if there is any error checking in the code the "ErrorReturnChecker" in https://reviews.llvm.org/D72705 is the better solution.) A special case is if the last called function was getc and the stream is in error state, here we know that it is necessary to call ferror or feof.

clang/test/Analysis/stream.c

246–247

Yes but this is job of an other checker (return value not used). This here is to ensure that the functions are called at least.

[baloghadamsoftware]

clang/test/Analysis/stream.c

188

I agree. The checker should emit a warning if and only if the return value of fgetc() equals to EOF. Another checker should ensure that the return value of fgetc() is indeed compared to EOF. However: are we sure that there are no cases were no other error may happen than EOF thus we intentionally omit further checking? (Imagine a simple read from the keyboard in a command line utility.)

[balazske]

Something like this can be added to StreamChecker but it has changed in many places so a new review would be needed. The main problem is with this fgetc check is that it would be good to check the standard stream version getc too, but that is hard to implement because global nature of the standard streams (stdin).

[balazske]

The idea of this change still applies but because checker and other code changes it is better to start again.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 //===----------------------------------------------------------------------===//
 
 #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/CheckerHelpers.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;
+  bool ShouldCallFeof{false};
+  bool ShouldCallFerror{false};
 
-  StreamState(Kind k) : K(k) {}
+  StreamState(Kind k, bool Feof, bool Ferror)
+      : K(k), ShouldCallFeof{Feof}, ShouldCallFerror{Ferror} {}
+  StreamState(Kind k, bool MakeError = false)
+      : K(k), ShouldCallFeof{MakeError}, ShouldCallFerror{MakeError} {}
 
   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; }
+  bool operator==(const StreamState &X) const {
+    return K == X.K && ShouldCallFeof == X.ShouldCallFeof &&
+           ShouldCallFerror == X.ShouldCallFerror;
+  }
 
   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.AddBoolean(ShouldCallFeof);
+    ID.AddBoolean(ShouldCallFerror);
   }
 };
 
 class StreamChecker : public Checker<eval::Call,
                                      check::DeadSymbols > {
   mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
-      BT_doubleclose, BT_ResourceLeak;
+      BT_doubleclose, BT_ResourceLeak, BT_missingerrorcheck;
+
+  mutable int EOFv{0};
 
 public:
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 
 private:
   using FnCheck = std::function<void(const StreamChecker *, const CallEvent &,
                                      CheckerContext &)>;
 
   CallDescriptionMap<FnCheck> Callbacks = {
       {{"fopen"}, &StreamChecker::evalFopen},
       {{"freopen", 3}, &StreamChecker::evalFreopen},
       {{"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)},
+      {{"fread", 4}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 3)},
+      {{"fwrite", 4}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 3)},
+      {{"fgetc", 1}, &StreamChecker::evalGetc},
       {{"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)},
+      {{"ftell", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+      {{"rewind", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+      {{"fgetpos", 2}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+      {{"fsetpos", 2}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+      {{"clearerr", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
+      {{"feof", 1}, &StreamChecker::evalFeof},
+      {{"ferror", 1}, &StreamChecker::evalFerror},
+      {{"fileno", 1}, std::bind(&StreamChecker::evalDefault, _1, _2, _3, 0)},
   };
 
   void evalFopen(const CallEvent &Call, CheckerContext &C) const;
   void evalFreopen(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;
-
+  void evalGetc(const CallEvent &Call, CheckerContext &C) const;
+  void evalFeof(const CallEvent &Call, CheckerContext &C) const;
+  void evalFerror(const CallEvent &Call, CheckerContext &C) const;
+  void evalDefault(const CallEvent &Call, CheckerContext &C,
+                   unsigned StreamArgI) const;
+
+  ProgramStateRef resetErrorState(SVal SV, CheckerContext &C,
+                                  ProgramStateRef State, bool FeofCalled,
+                                  bool FerrorCalled) const;
+  ExplodedNode *checkErrorState(SVal SV, CheckerContext &C,
+                                ExplodedNode *N) const;
   ProgramStateRef checkNullStream(SVal SV, CheckerContext &C,
                                   ProgramStateRef State) const;
   ProgramStateRef checkFseekWhence(SVal SV, CheckerContext &C,
                                    ProgramStateRef State) const;
   ProgramStateRef checkDoubleClose(const CallEvent &Call, CheckerContext &C,
                                    ProgramStateRef State) const;
 };
 
void StreamChecker::evalFreopen(const CallEvent &Call,
   if (!State)
     return;
 
   SymbolRef StreamSym = StreamVal->getAsSymbol();
   // Do not care about special values for stream ("(FILE *)0x12345"?).
   if (!StreamSym)
     return;
 
+  ExplodedNode *N = C.addTransition(State);
+  N = checkErrorState(*StreamVal, C, N);
+  State = N->getState();
+
   // 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.
   ProgramStateRef StateRetNull = State->BindExpr(CE, C.getLocationContext(),
                                                  C.getSValBuilder().makeNull());
 
   StateRetNotNull =
       StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened());
   StateRetNull =
       StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed());
 
-  C.addTransition(StateRetNotNull);
-  C.addTransition(StateRetNull);
+  C.addTransition(StateRetNotNull, N);
+  C.addTransition(StateRetNull, N);
 }
 
 void StreamChecker::evalFclose(const CallEvent &Call, CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
+  ExplodedNode *N = checkErrorState(Call.getArgSVal(0), C, C.getPredecessor());
+
+  ProgramStateRef State = N->getState();
   State = checkDoubleClose(Call, C, State);
   if (State)
-    C.addTransition(State);
+    C.addTransition(State, N);
 }
 
 void StreamChecker::evalFseek(const CallEvent &Call, CheckerContext &C) const {
+  ExplodedNode *N = checkErrorState(Call.getArgSVal(0), C, C.getPredecessor());
+
   const Expr *AE2 = Call.getArgExpr(2);
   if (!AE2)
     return;
 
-  ProgramStateRef State = C.getState();
+  ProgramStateRef State = N->getState();
 
   State = checkNullStream(Call.getArgSVal(0), C, State);
   if (!State)
     return;
 
   State =
       checkFseekWhence(State->getSVal(AE2, C.getLocationContext()), C, State);
   if (!State)
     return;
 
+  C.addTransition(State, N);
+}
+
+void StreamChecker::evalGetc(const CallEvent &Call, CheckerContext &C) const {
+  if (EOFv == 0) {
+    if (const llvm::Optional<int> OptInt =
+            tryExpandAsInteger("EOF", C.getPreprocessor()))
+      EOFv = *OptInt;
+    else
+      EOFv = -1;
+  }
+
+  ExplodedNode *N = checkErrorState(Call.getArgSVal(0), C, C.getPredecessor());
+
+  auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
+  SymbolRef StreamSym = Call.getArgSVal(0).getAsSymbol();
+  if (!StreamSym)
+    return;
+
+  ProgramStateRef State = N->getState();
+  State = checkNullStream(Call.getArgSVal(0), C, State);
+  if (!State)
+    return;
+
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
+  if (!SS)
+    return;
+
+  SValBuilder &SVB = C.getSValBuilder();
+  BasicValueFactory &BVF = SVB.getBasicValueFactory();
+
+  DefinedSVal RetVal =
+      SVB.conjureSymbolVal(nullptr, CE, C.getLocationContext(), C.blockCount())
+          .castAs<DefinedSVal>();
+  State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+
+  ProgramStateRef FailedState, NonFailedState;
+  const llvm::APSInt &EOFVal = BVF.getValue(EOFv, Call.getResultType());
+  std::tie(FailedState, NonFailedState) =
+      State->assumeInclusiveRange(RetVal, EOFVal, EOFVal);
+  if (FailedState) {
+    FailedState =
+        FailedState->set<StreamMap>(StreamSym, StreamState{SS->K, true});
+    C.addTransition(FailedState, N);
+  }
+  if (NonFailedState) {
+    C.addTransition(NonFailedState, N);
+  }
+}
+
+void StreamChecker::evalFeof(const CallEvent &Call, CheckerContext &C) const {
+  SVal StreamVal = Call.getArgSVal(0);
+  ProgramStateRef State = C.getState();
+  State = checkNullStream(StreamVal, C, State);
+  if (!State)
+    return;
+
+  State = resetErrorState(StreamVal, C, State, true, false);
+
   C.addTransition(State);
 }
 
-void StreamChecker::checkArgNullStream(const CallEvent &Call, CheckerContext &C,
-                                       unsigned ArgI) const {
+void StreamChecker::evalFerror(const CallEvent &Call, CheckerContext &C) const {
+  SVal StreamVal = Call.getArgSVal(0);
   ProgramStateRef State = C.getState();
-  State = checkNullStream(Call.getArgSVal(ArgI), C, State);
-  if (State)
-    C.addTransition(State);
+  State = checkNullStream(StreamVal, C, State);
+  if (!State)
+    return;
+
+  State = resetErrorState(StreamVal, C, State, false, true);
+
 }
 
+void StreamChecker::evalDefault(const CallEvent &Call, CheckerContext &C,
+                                unsigned StreamArgI) const {
+  ExplodedNode *N =
+      checkErrorState(Call.getArgSVal(StreamArgI), C, C.getPredecessor());
+  ProgramStateRef State = N->getState();
+  State = checkNullStream(Call.getArgSVal(StreamArgI), C, State);
+  if (!State)
+    return;
+
+  C.addTransition(State, N);
+}
+
+ProgramStateRef StreamChecker::resetErrorState(SVal SV, CheckerContext &C,
+                                               ProgramStateRef State,
+                                               bool FeofCalled,
+                                               bool FerrorCalled) const {
+  SymbolRef StreamSym = SV.getAsSymbol();
+  if (!StreamSym)
+    return State;
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
+  if (!SS || !SS->isOpened())
+    return State;
+  if ((!SS->ShouldCallFeof || !FeofCalled) &&
+      (!SS->ShouldCallFerror || !FerrorCalled))
+    return State;
+
+  if (FeofCalled) {
+    State = State->set<StreamMap>(
+        StreamSym, StreamState{SS->K, false, SS->ShouldCallFerror});
+  }
+  if (FerrorCalled) {
+    State = State->set<StreamMap>(
+        StreamSym, StreamState{SS->K, SS->ShouldCallFeof, false});
+  }
+
+  return State;
+}
+
+ExplodedNode *StreamChecker::checkErrorState(SVal SV, CheckerContext &C,
+                                             ExplodedNode *N) const {
+  SymbolRef StreamSym = SV.getAsSymbol();
+  if (!StreamSym)
+    return N;
+  ProgramStateRef State = N->getState();
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
+  if (!SS || !SS->isOpened())
+    return N;
+
+  if (SS->ShouldCallFeof || SS->ShouldCallFerror) {
+    State = State->set<StreamMap>(StreamSym, StreamState{SS->K, false});
+    if (ExplodedNode *N1 = C.generateNonFatalErrorNode(State)) {
+      if (!BT_missingerrorcheck)
+        BT_missingerrorcheck.reset(new BuiltinBug(
+            this, "Missing error check",
+            "Should call 'feof' and 'ferror' after failed character read."));
+      C.emitReport(std::make_unique<PathSensitiveBugReport>(
+          *BT_missingerrorcheck, BT_missingerrorcheck->getDescription(), N1));
+      return N1;
+    }
+  }
+  return N;
+}
+
 ProgramStateRef StreamChecker::checkNullStream(SVal SV, CheckerContext &C,
                                                ProgramStateRef State) const {
   Optional<DefinedSVal> DV = SV.getAs<DefinedSVal>();
   if (!DV)
     return State;
 
   ConstraintManager &CM = C.getConstraintManager();
   ProgramStateRef StateNotNull, StateNull;

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.  */
+#define EOF (-1)
 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 fgetc(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);
void check_freopen_3() {
   if (f1) {
     // Unchecked result of freopen.
     // The f1 may be invalid after this call (not checked by the checker).
     freopen(0, "w", f1);
     rewind(f1);
     fclose(f1);
   }
 }
+
+void check_fgetc_error() {
+  FILE *fp = fopen("foo.c", "r");
+  if (fp) {
+    int C;
+    fpos_t pos;
+    C = fgetc(fp);
+    fread(0, 0, 0, fp); // expected-warning {{Should call}}
+    fread(0, 0, 0, fp);
+    C = fgetc(fp);
+    fwrite(0, 0, 0, fp); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    fseek(fp, 1, SEEK_SET); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    ftell(fp); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    rewind(fp); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    fgetpos(fp, &pos); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    fsetpos(fp, 0); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    clearerr(fp); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    C = fgetc(fp);            // expected-warning {{Should call}}
+    fp = freopen(0, "w", fp); // expected-warning {{Should call}}
+    C = fgetc(fp);
+    fclose(fp); // expected-warning {{Should call}}
+  }
+}
+
+void check_fgetc_error1() {
+  FILE *fp = fopen("foo.c", "r");
+  if (fp) {
+    int C;
+    C = fgetc(fp);
+    feof(fp);
+    fclose(fp); // expected-warning {{Should call}}
+  }
+  fp = fopen("foo.c", "r");
+  if (fp) {
+    int C;
+    C = fgetc(fp);
+    ferror(fp);
+    fclose(fp); // expected-warning {{Should call}}
+  }
+}
+
+void check_fgetc_noerror() {
+  FILE *fp = fopen("foo.c", "r");
+  if (fp) {
+    int C;
+    C = fgetc(fp);
+    feof(fp);
+    ferror(fp);
+    fclose(fp);
+  }
+}
+
+void check_fgetc_checked() {
+  FILE *fp = fopen("foo.c", "r");
+  if (fp) {
+    int C;
+    C = fgetc(fp);
+    if (C == EOF) {
+      feof(fp);
+      ferror(fp);
+    }
+    fclose(fp);
+  }
+}