[Checkers] Added support for freopen to StreamChecker.

[balazske]

Summary

Extend StreamChecker with a new evaluation function for API call 'freopen'.

[baloghadamsoftware]

Thank you for working on this. Please add comments to the code and maybe also to the tests. I could not find anything in the standards about freopen() with null-pointer for the stream parameter.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

219

The four lines above are typical cases for using auto, since the type is duplicated in the line. See: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

263

Please use a more elaborated error message here.

clang/test/Analysis/stream.c

120

I wonder if this test belongs into this particular patch. It has nothing to do with freopen().

[balazske]

From the description The original stream (if it exists) is closed. I think it is possible that the original stream does "not exist". But a test program crashed with NULL argument (but not with a closed file). So null argument is not permitted or at least does not work always.

[balazske]

• Do not allow null stream to freopen.
• Added comments.

[balazske]

I am still not sure in the auto type, I did not see that way of auto usage often in clang code.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

219

This is not totally clear if we can trust the pattern that at auto X = Y.getXxx() type of X will be Xxx.

clang/test/Analysis/stream.c

120

I wanted to see if "aliasing" works because it happens in the freopen tests too. Probably this test is not needed?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

219

@balazske +1. I'm personally in favor of using auto here, but the community's stance is to use auto only for dyn_cast/getAs etc. calls where type is explicitly spelled out, and also for iterators where the type is going to be too long and annoying to spell out (but in the latter case i'm suddenly in favor of spelling it out, as i always get confused about how many *s do i need to add in order to properly dereference whatever's in there).

232–233

That's actually one more good reason to move all checkNullStream to PreCall: less spaghetti. The same checker should feel free to subscribe for the same function in multiple different callbacks and do different things in them.

Rule of a thumb (i'm not sure if it's always applicable but it's applicable to most "typical" checkers even if they aren't currently written this way):

1. Pre-condition checking goes in PreCall;
2. Modeling that affects everybody goes in evalCall;
3. Checker's internal bookkeeping goes in PostCall.

235–247

It looks like you're creating a symbol only to assume that it's null. You should bind a concrete null (i.e., loc::ConcreteInt) instead.

251

The null StreamVal is going to be caught by checkNullStream(). What are the other possibilities here? Concrete FILE * that isn't null?

I guess it's possible but it probably deserves a comment describing that this is actually a very rare scenario. Maybe a test that involves a value like ((FILE *)0x12345).

262–263

What does really happen when you freopen a stream that isn't already open? It sounds like it's doing a fclose blindly, so we might as well report a double close in this case, and then we don't need to update the program state because it's already opened(?) Though later we still might want to update the program state if we start keeping track of the filename or the mode.

[balazske]

• Simplified the code.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

232–233

Precondition (check for NULL stream) can go into PreCall. Probably the bookkeeping task is more difficult to do, if the state is split we have already the different states available in evalCall but need to check the state in postCall.

262–263

The documentation says that the stream is closed but error is ignored. So the double close should be no problem. If the file was already open the state should not be changed (the set call handles that case and does not generate new state?).

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

70

I would move this freopen line and everything else (evalFreopen header and body) directly after freopen. To me it seems more logical.

219

SValBuilder is explicitly spelled out here, also DefinedSVal below, which comes from a getAs(). Btw, is it right to call the actual variable SValBuilder as well? I would never do that. I is usually called SVB.

232–233

@NoQ +1 I completely agree: All precondition checks in this checker should be moved into PreCall. I would even go further: separate the checker into a StreamModeling which models the calls using evalCall and StreamChecker which does the checks. However, these NFCs should be done in subsequent patches, not this one.

[balazske]

• Moved freopen after fopen, removed 'SValBuilder'.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

232–233

Does the setting of new state (State->set<StreamMap>) belong to the "modeling" (evalCall) or "checker" (in the postCall) part?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

244

Mmm, wait, this doesn't look right to me. You cannot deduce from the presence of freopen() in the code that the argument may be null, so the split over the argument is not well-justified.

The right thing to do here will be to produce a "Schrödinger file descriptor" that's both Opened and OpenFailed until we observe the return value to be constrained to null or non-null later during analysis (cf. D32449), otherwise conservatively assume that the open succeeded when queried for the first time (because that's how non-paranoid code under analysis will behave).

Or you could drop the failed path immediately if the argument isn't known to be zero. That'll drop the coverage slightly but won't cause anything bad to happen.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

244

Where does this comment belong? I got this in the email:

Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:201
+    std::tie(StateRetNotNull, StateRetNull) =
+        CM.assumeDual(StateStreamNull, RetVal);
+    if (StateRetNull) {

Is the problem with the null-check of argument of freopen? Why is this different than the other calls where checkNullStream is used?
Or is the problem with the case of NULL return value from freopen (in this case the argument was non-null, for the null case we assume program crash)? In this case we really do not know what happened with the file descriptor argument (but the value of it is not changed), so the code assumes now that is will be OpenFailed. This is not accurate always (it may remain open or become closed). We could have another state split here (for these cases)?

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

206

I think it would be better to rearrange these lines to exactly match evalFopen().

244

The argument is checked against NULL, becuase it must not be NULL. That is the "checker" part. The return value is split like in the existing code for modeling fopen(). I do not see anything wrong here.

[martong]

clang/test/Analysis/stream.c

160

This comment is confusing for me. Maybe there are typos here?
Isn't the 'freopen' failed? fopen was successful and it's return value was checked in line 153.
f1 seems to be ok here (still associated with "foo.c"), only f2 is bad, as we see in line 162.

Or f1 is already closed here, even though it could not open the new file?

If a new filename is specified, the function first attempts to close any file already associated with stream (third parameter) and disassociates it. Then, independently of whether that stream was successfuly closed or not, freopen opens the file specified by filename and associates it with the stream just as fopen would do using the specified mode.

https://stackoverflow.com/questions/20908740/check-the-return-value-of-freopen-in-c

[balazske]

• rearrangement in evelFreopen, updated comment in test

[balazske]

clang/test/Analysis/stream.c

160

The freopen has failed, not the fopen. I think we can not know if f1 points to any file after this, or is in closed state (but we know that is not safe to use). Probably both the close and the re-open can fail.

[martong]

LGTM, but wait for others please.

[baloghadamsoftware]

LGTM.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

244

The final code looks correct to me, thanks!~

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

public:
   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)},
       {{"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 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;
   bool checkNullStream(SVal SV, CheckerContext &C,
                        ProgramStateRef &State) const;
   void checkFseekWhence(SVal SV, CheckerContext &C,
void StreamChecker::evalFopen(const CallEvent &Call, CheckerContext &C) const {
   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::evalFreopen(const CallEvent &Call,
+                                CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+
+  auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
+  Optional<DefinedSVal> StreamVal = Call.getArgSVal(2).getAs<DefinedSVal>();
+  if (!StreamVal)
+    return;
+  // Do not allow NULL as passed stream pointer.
+  // This is not specified in the man page but may crash on some system.
+  checkNullStream(*StreamVal, C, State);
+  // Check if error was generated.
+  if (C.isDifferent())
+    return;
+
+  SymbolRef StreamSym = StreamVal->getAsSymbol();
+  // Do not care about special values for stream ("(FILE *)0x12345"?).
+  if (!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.
+  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);
+}
+
 void StreamChecker::evalFclose(const CallEvent &Call, CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   if (checkDoubleClose(Call, C, State))
     C.addTransition(State);
 }
 
 void StreamChecker::evalFseek(const CallEvent &Call, CheckerContext &C) const {
   const Expr *AE2 = Call.getArgExpr(2);
   if (!AE2)
     return;
 
   ProgramStateRef State = C.getState();
 
   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'.
   checkFseekWhence(State->getSVal(AE2, C.getLocationContext()), C, State);
 
   if (!C.isDifferent() && StateChanged)
     C.addTransition(State);
 
   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);
 }
 
 bool StreamChecker::checkNullStream(SVal SV, CheckerContext &C,
                                     ProgramStateRef &State) const {
   Optional<DefinedSVal> DV = SV.getAs<DefinedSVal>();
   if (!DV)
     return false;
 
   ConstraintManager &CM = C.getConstraintManager();
   ProgramStateRef StateNotNull, StateNull;
   std::tie(StateNotNull, StateNull) = CM.assumeDual(C.getState(), *DV);
 
   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 false;
   }
 
   if (StateNotNull) {
     State = StateNotNull;
     return true;
   }
 
   return false;
 }
 
 void StreamChecker::checkFseekWhence(SVal SV, CheckerContext &C,

clang/test/Analysis/stream.c

 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);
+extern FILE *freopen(const char *pathname, const char *mode, FILE *stream);
 
 void check_fread() {
   FILE *fp = tmpfile();
   fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
 void check_fwrite() {

 }
 
 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 f_double_close_alias(void) {
+  FILE *p1 = fopen("foo", "r");
+  FILE *p2 = p1;
+  fclose(p1);
+  fclose(p2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}}
+}
+
 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 *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);
 }
+
+void check_freopen_1() {
+  FILE *f1 = freopen("foo.c", "r", (FILE *)0); // expected-warning {{Stream pointer might be NULL}}
+  f1 = freopen(0, "w", (FILE *)0x123456);      // Do not report this as error.
+}
+
+void check_freopen_2() {
+  FILE *f1 = fopen("foo.c", "r");
+  if (f1) {
+    FILE *f2 = freopen(0, "w", f1);
+    if (f2) {
+      // Check if f1 and f2 point to the same stream.
+      fclose(f1);
+      fclose(f2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}}
+    } else {
+      // Reopen failed.
+      // f1 points now to a possibly invalid stream but this condition is currently not checked.
+      // f2 is NULL.
+      rewind(f1);
+      rewind(f2); // expected-warning {{Stream pointer might be NULL}}
+    }
+  }
+}
+
+void check_freopen_3() {
+  FILE *f1 = fopen("foo.c", "r");
+  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);
+  }
+}