[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yay, i see the "Schrödinger handle" pattern: until the return value of release is checked, the handle is believed to be both dead and alive at the same time.

We usually use this pattern because a lot of code that's already written doesn't check their return values. But do you really need this for a brand-new API? Maybe aggressively assume that the release may always fail and fix all the places in your code where the return value is not checked before use?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:99-100
+
+static const StringRef HandleTypeName = "zx_handle_t";
+static const StringRef ErrorTypeName = "zx_status_t";
+
----------------
So you plan to unhardcode these as well eventually?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:123
+
+  // Only use this in checkDeadSymbols!
+  bool hasError(ProgramStateRef State) const {
----------------
For now it doesn't seem to be used at all.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:141-146
+#define CASE(ID)                                                               \
+  case ID:                                                                     \
+    OS << #ID;                                                                 \
+    break;
+      CASE(Kind::Allocated)
+      CASE(Kind::Released)
----------------
This macro has literally saved exactly as many lines as it has caused :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:156-168
+  std::unique_ptr<BugType> LeakBugType;
+  std::unique_ptr<BugType> DoubleReleaseBugType;
+  std::unique_ptr<BugType> UseAfterFreeBugType;
+
+public:
+  FuchsiaHandleChecker()
+      : LeakBugType(new BugType(this, "Fuchsia handle leak",
----------------
We've recently dumbed down this idiom to
```lang=c++
class FuchsiaHandleChecker : ... {
  LeakBugType{this, "Fuchsia handle leak",
              "Fuchsia Handle Error", /*SuppressOnSink=*/true};
  ...
};
```
:)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:207
+  int PtrToHandleLevel = 0;
+  const Type *T = QT.getTypePtr();
+  while (T->isAnyPointerType() || T->isReferenceType() || T->isArrayType()) {
----------------
This is never necessary, just call all the methods directly on `QT` - it has an overloaded operator `->()` for this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:208
+  const Type *T = QT.getTypePtr();
+  while (T->isAnyPointerType() || T->isReferenceType() || T->isArrayType()) {
+    ++PtrToHandleLevel;
----------------
Ok, so what's the deal with arrays here? If the function receives an array of handles, do you ultimately plan to return multiple symbols - one for each element of the array?

Also, in the generic non-Fuchsia case, what if the handle itself is a pointer?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:212
+  }
+  if (const auto *HandleType = dyn_cast<TypedefType>(T)) {
+    if (HandleType->getDecl()->getName() != HandleTypeName)
----------------
Here `QT->getAs<TypedefType>()` respectively.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:302-303
+                                   Pred);
+        // Avoid further reports on this symbol.
+        State = State->remove<HStateMap>(HandleSym);
+      } else
----------------
Why not generate a fatal error instead?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:336-338
+ProgramStateRef FuchsiaHandleChecker::evalAssume(ProgramStateRef State,
+                                                 SVal Cond,
+                                                 bool Assumption) const {
----------------
Ok, so you're basically saying that whenever the code under analysis does anything to check the return value, this callback will fire immediately? Thus guaranteeing that whenever the Schrödinger state is accessed, it's either already collapsed or indeed not checked yet?

This sounds right but i wish i had your confidence :D

My usual approach to Schrödinger states will be to do the same thing but in `checkDeadSymbols()`. I.e., when the return value symbol dies, see if it's constrained and collapse the state accordingly. If the access occurs earlier than symbol death, update the state according to the current constraints and forget about the symbol. If current constraints are insufficient, conservatively mark it as a successful release (as that's how non-paranoid code under analysis would behave).

Regardless of the approach, you still need to update the state upon access as if the release has succeeded (when the current state is a Schrödinger state). But with your approach you can avoid checking the constraints upon access, and instead blindly assume that the symbol is underconstrained.

I like this! Can we prove that it works by adding an assertion that the return value symbol is underconstrained upon every access to the potentially Schrödinger state?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:343
+    SymbolRef ErrorSym = CurItem.second.getErrorSym();
+    if (Cmr.isNull(State, ErrorSym).isConstrainedFalse()) {
+      if (CurItem.second.isReleased())
----------------
If you want this check to be universally applicable, you will probably also need an annotation that explains which statuses indicate an error.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:348
+      else
+        State = State->remove<HStateMap>(CurItem.first);
+    }
----------------
I request comments about this path. When does this happen?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:366
+        for (unsigned Arg = 0; Arg < Call->getNumArgs(); ++Arg) {
+          Optional<unsigned> ParamIdx = Call->getAdjustedParameterIndex(Arg);
+          if (!ParamIdx)
----------------
I strongly recommend testing this (i.e., by annotating an overloaded member operator with the use-annotation). This code looks correct to me but it's just too easy to make a mistake here :/


================
Comment at: clang/test/Analysis/fuchsia_handle.c:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=fuchsia.FuchsiaHandleChecker -verify %s
+
----------------
`core,`!


================
Comment at: clang/test/Analysis/fuchsia_handle.c:76
+
+void checkNoLeak07(int tag) {
+  zx_handle_t sa, sb;
----------------
This test doesn't really test the pointer escape mechanism because both handles are closed correctly, so there's no leak in this test regardless of escapes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70470