[PATCH] D122150: [clang][analyzer] Add checker for bad use of 'errno'.

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

I'm liking it. We need to improve the diagnostics and the user-facing docs as well.



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:545
+  Dependencies<[ErrnoModeling]>,
+  Documentation<HasAlphaDocumentation>;
+
----------------
Then we should have documentation, and examples for it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:47-49
+  mutable bool ErrnoInitialized = false;
+  mutable Optional<Loc> ErrnoLoc;
+  mutable const MemRegion *ErrnoRegion = nullptr;
----------------
One should not define member variables like this.
The analysis engine might explore exploded nodes in an unpredictable order, invoking the checker's handler callbacks in an indeterminate order.
You should have registered simple value traits for this purpose, associating the necessary information with a given State.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:68-71
+  if (ErrnoLoc) {
+    ErrnoRegion = ErrnoLoc->getAsRegion();
+    assert(ErrnoRegion && "The 'errno' location should be a memory region.");
+  }
----------------
`Loc` always wrap a memregion of some sort.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:114-116
+  if (auto L = Loc.getAs<ento::Loc>()) {
+    if (*ErrnoLoc != *L)
+      return;
----------------
I think an early return could have spared an indentation level in the outer scope.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:198
+  // allow read and write without bug reports.
+  if (llvm::find(Regions, ErrnoRegion) != Regions.end())
+    return setErrnoStateIrrelevant(State);
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:203-206
+  const MemSpaceRegion *GlobalSystemSpace =
+      State->getStateManager().getRegionManager().getGlobalsRegion(
+          MemRegion::GlobalSystemSpaceRegionKind);
+  if (llvm::find(Regions, GlobalSystemSpace) != Regions.end())
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:271-272
+    if (IdentifierInfo *II = FD->getIdentifier())
+      return llvm::find(ErrnoLocationFuncNames, II->getName()) !=
+             std::end(ErrnoLocationFuncNames);
+  return false;
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:24
 
+enum ErrnoCheckState : unsigned {
+  Errno_Irrelevant = 0,
----------------
Did you consider enum classes?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:35
+
+llvm::Optional<Loc> getErrnoLoc(ProgramStateRef State);
+
----------------
I would appreciate some notes about when it returns `None`.


================
Comment at: clang/test/Analysis/errno.c:73-75
+  int X = ErrnoTesterChecker_setErrnoCheckState();
+  if (X == 0) {
+    if (errno) { // expected-warning{{Value of 'errno' could be undefined after a call to a function that does not promise to not change 'errno' [alpha.unix.Errno]}}
----------------
We should have a note describing which function left the `errno` in an undefined/indeterminate state.
You can chain multiple NoteTags by using the ExplodedNode returned by the `addTransition()` and passing it along if needed.


================
Comment at: clang/test/Analysis/errno.c:170
+    clang_analyzer_eval(errno); // expected-warning{{TRUE}}
+    something();
+    clang_analyzer_eval(errno); // expected-warning{{UNKNOWN}}
----------------
So this is the case when `errno` gets indirectly invalidated by some opaque function call.  I see.
However, it will test that the value associated with the errno region gets invalidated, that's fine. However, you should test if the metadata (the enum value) attached to memregion also gets invalidated.
Please make sure you have tests for that as well.
A `ErrnoTesterChecker_dumpCheckState()` should be perfect for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122150