[PATCH] D120310: [clang][analyzer] Add modeling of 'errno' (work-in-progress).

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

steakhal added a comment.

Good start!

However, I'm not a big fan of coupling the testing checker with the actual modeling checker.
IMO we should have a distinct checker, similarly to `TaintTester`. That way you could do even fancier things like:
define `mylib_may_fail()`, bifurcate and return `true` for the success case, on which it wouldn't touch `errno`; and on the failure case return `false` and set the `errno` to some concrete value that we could check against.

But I'm also fine with the current approach, providing the `set_errno(SVal)` introspection function.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:767-768
            s->getType()->isBlockPointerType());
-    assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg));
+    assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg) ||
+           isa<GlobalSystemSpaceRegion>(sreg));
   }
----------------
At this point, I'm not even sure if we should assert any of these.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1378
 
   /// Retrieve or create a "symbolic" memory region.
+  const SymbolicRegion *
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/Errno.h:22
+namespace ento {
+namespace errno_check {
+
----------------
I think we can settle on something better. What about calling it simply `errno`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Errno.h:24-26
+/// Returns if modeling of 'errno' is available.
+/// If not, the other functions here should not be used.
+bool isErrnoAvailable(ProgramStateRef State);
----------------
I don't think we need this.
In `getErrnoValue()` we should simply return `UnknownVal` if we don't have 'errno.
And in `setErrnoValue()` we should return the incoming `State` unmodified.

THat being said, only a top-level `Check::BeginFunction` callback could see an 'errno' uninitialized, which I don't think is a real issue.
All the rest of the callbacks would run after it's initialized, thus would behave as expected.
And in case the translation unit doesn't have nor 'errno' variable nor 'errno_location_ish' functions, ignoring these `set/get` functions is actually the expected behavior.

Having to check `isErrnoAvailable()` would be really an anti-pattern.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:32
+// FIXME: Is there a system where it is not called "errno" but is a variable?
+const char *ErrnoVarName = "errno";
+// Names of functions that return a location of the "errno" value.
----------------
Why don't you use `StringRef` here as well?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:35
+// FIXME: Check if there are other similar function names.
+llvm::StringRef ErrnoLocationFuncNames[] = {"__errno_location"};
+
----------------
`compiler-rt/lib/sanitizer_common/sanitizer_errno.h` has a list of the possible names for this API.:
`__error`, `__errno`, `___errno`, `_errno` or `__errno_location`

That being said, you should `eval::Call` all of these, and bind the return value to the memorized errno region.
At this point, this should be a `CallDescriptionSet` within the checker.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:60
+/// Returns null if there isn't any associated memory region.
+inline const MemRegion *getErrnoRegion(ProgramStateRef State) {
+  return reinterpret_cast<const MemRegion *>(State->get<ErrnoRegion>());
----------------
For function definitions, we should use `static` instead of anonymous namespaces.
That being said, I don't think `inline` will do anything in this context.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:66
+
+void ErrnoChecker::checkBeginFunction(CheckerContext &C) const {
+  if (!C.inTopFrame())
----------------
I would expect a comment stating that:
- We inspect if we have a VarDecl naming "errno", it returns that Decl.
- Otherwise, it will look for some common `errno_location` function names and return that Decl. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:88
+
+  auto GetErrnoFunc = [&ACtx]() -> const FunctionDecl * {
+    SmallVector<const Decl *> LookupRes;
----------------
I think you can hoist these lambdas into static functions.
This handler is already big enough.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:99
+               FD->isExternC() && FD->getNumParams() == 0 &&
+               FD->getReturnType() == ACtx.getPointerType(ACtx.IntTy);
+      return false;
----------------
I don't think any of these actually return the canonic versions.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:118
+    assert(ErrnoR && "Memory region should exist for the 'errno' variable.");
+    State = State->set<ErrnoRegion>(reinterpret_cast<const void *>(ErrnoR));
+  } else if (GetErrnoFunc()) {
----------------
Ah, I've seen and done these reinterpret casts.
Could you please check why don't we have a partial specialization allowing any const pointers besides void pointers?
That way it would look much better.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:134
+    const SymbolConjured *Sym =
+        SVB.conjureSymbol(nullptr, C.getLocationContext(), ACtx.IntTy, 1);
+
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:146-149
+  // Errno is initialized to 0 at program start.
+  State = errno_check::setErrnoValue(State, C, 0);
+
+  C.addTransition(State);
----------------
You should sink these lines to the corresponding branches.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:180-181
+  // The special errno region should never garbage collected.
+  const auto *ErrnoR = getErrnoRegion(State);
+  if (ErrnoR)
+    SR.markLive(ErrnoR);
----------------
Fuse these two lines.


================
Comment at: clang/test/Analysis/Inputs/errno_var.h:3
+
+// Define 'errno' as an extern variable in a system header.
+// This may be not allowed in C99.
----------------



================
Comment at: clang/test/Analysis/errno.c:22
+
+  FILE *F = fopen("/a/b", "r");
+
----------------
Please call a different function, which is definitely not from glibc.
We might model this in the future using `eval::Call` in which case no invalidation will happen breaking the test.


================
Comment at: clang/test/Analysis/global-region-invalidation.c:6
 // Note, we do need to include headers here, since the analyzer checks if the function declaration is located in a system header.
+#include "Inputs/errno_var.h"
 #include "Inputs/system-header-simulator.h"
----------------
If you were `eval::Calling` the `errno_location` functions, and used the corresponding header, would the tests pass?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120310