[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

To summarize, we're moving every stream check (illegal call to a stream management function with a null stream, or a closed stream, etc) to `preCall`, and leave only the modeling portions in `evalCall`. Makes sense!

You did an amazing job here! I like the new infrastructure and everything.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:76-92
+      {{"fopen"}, {{}, &StreamChecker::evalFopen, ArgNone}},
+      {{"freopen", 3},
+       {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
+      {{"tmpfile"}, {{}, &StreamChecker::evalFopen, ArgNone}},
+      {{"fclose", 1},
+       {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
+      {{"fread", 4}, {&StreamChecker::preDefault, {}, 3}},
----------------
Hmm, all of these braces are kinda hard to navigate, but its not too bad. Could you check whether changing them to `nullptr` improves readability? I'll leave it to your judgement.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:109-114
+  ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
+                                      ProgramStateRef State) const;
+  ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
+                                           ProgramStateRef State) const;
+  ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C,
+                                     ProgramStateRef State) const;
----------------
Could we add/move some comments for these? :)

```
Checks whether the stream is non-null. If it is null, an fatal bug report is emitted and the return value is nullptr. Otherwise in the returned state StreamVal is non-null.
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:161
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = Call.getArgSVal(Desc->StreamArgNo);
+  State = ensureStreamNonNull(StreamVal, C, State);
----------------
This would assert in `Expr::getArg` if `Desc->StreamArgNo == ArgNone`, but that shouldn't ever happen in this function, correct? Could we add an assert here as well as a poor man's documentation?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:192-194
+  // Do not allow NULL as passed stream pointer.
+  // This is not specified in the man page but may crash on some system.
+  // But allow a closed stream, is this correct?
----------------
According to my research, yes, closed streams are okay. Also, the rest of the comment can be removed as well -- it is for sure not okay to pass `NULL` to `freopen`, even if it doesn't crash on some systems.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:281-284
+  // Close the File Descriptor.
+  // Regardless if the close fails or not, stream becomes "closed"
+  // and can not be used any more.
+  State = State->set<StreamMap>(Sym, StreamState::getClosed());
----------------
Ah, we're moving the closing of the stream from `checkDoubleClose` to here. Makes much more sense!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:341-345
+// Check:
+// Using a stream pointer after 'fclose' causes undefined behavior
+// according to cppreference.com .
+// Using a stream that has failed to open is likely to cause problems, but not
+// explicitly mentioned in documentation.
----------------
Could we move this to the declaration and start it with `///`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:349-355
+  SymbolRef Sym = StreamVal.getAsSymbol();
   if (!Sym)
-    return State;
+    return nullptr;
 
   const StreamState *SS = State->get<StreamMap>(Sym);
-
-  // If the file stream is not tracked, return.
   if (!SS)
+    return nullptr;
----------------
Previously, we only returned a `nullptr` after generating a fatal error node. Why do we need to change this?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:363-364
+            new BuiltinBug(this, "Stream used after close",
+                           "File descriptor is used after it was closed. "
+                           "Cause undefined behaviour."));
       C.emitReport(std::make_unique<PathSensitiveBugReport>(
----------------
How about

"Closing an already closed file descriptor."


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:381-382
+            new BuiltinBug(this, "Stream used after failed open",
+                           "(Re-)Opening the file descriptor has failed. "
+                           "Using it can cause undefined behaviour."));
+      C.emitReport(std::make_unique<PathSensitiveBugReport>(
----------------
How about

"Using a file descriptor after (re-)opening it failed."


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75163/new/

https://reviews.llvm.org/D75163