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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 13 inline comments as done.
xazax.hun added a comment.

Thanks for the review!

I am not very well versed in Fuchsia's syscalls yet but my understanding is that not all of the syscalls can fail so we do not need all the users to check for errors. But this is something I will verify with the rest of the team, so please treat my answer with a grain of salt for now. An alternative would be to introduce an annotation to tell which APIs will never fail, but I am afraid that is also sometimes subjective. For example it is pretty much possible to have no available port in a system but is very unlikely to happen so some non mission critical applications might not check for errors there. But this is also something that I will follow up with the team.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:99-100
+
+static const StringRef HandleTypeName = "zx_handle_t";
+static const StringRef ErrorTypeName = "zx_status_t";
+
----------------
NoQ wrote:
> So you plan to unhardcode these as well eventually?
I have no specific plans at this point. I do not see these types changing frequently, so I have no urge to unhardcode them. Are there any downsides I am not being aware of?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:123
+
+  // Only use this in checkDeadSymbols!
+  bool hasError(ProgramStateRef State) const {
----------------
NoQ wrote:
> For now it doesn't seem to be used at all.
Wow, indeed! This is from an eariler iteration :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:141-146
+#define CASE(ID)                                                               \
+  case ID:                                                                     \
+    OS << #ID;                                                                 \
+    break;
+      CASE(Kind::Allocated)
+      CASE(Kind::Released)
----------------
NoQ wrote:
> This macro has literally saved exactly as many lines as it has caused :)
Good point! Initially I had a separate escaped state, but now I guess I should just expand them :)


================
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",
----------------
NoQ wrote:
> We've recently dumbed down this idiom to
> ```lang=c++
> class FuchsiaHandleChecker : ... {
>   LeakBugType{this, "Fuchsia handle leak",
>               "Fuchsia Handle Error", /*SuppressOnSink=*/true};
>   ...
> };
> ```
> :)
+1 :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:207
+  int PtrToHandleLevel = 0;
+  const Type *T = QT.getTypePtr();
+  while (T->isAnyPointerType() || T->isReferenceType() || T->isArrayType()) {
----------------
NoQ wrote:
> This is never necessary, just call all the methods directly on `QT` - it has an overloaded operator `->()` for this.
`getPointeeOrArrayElementType` returns a `Type *`, so I cannot really continue to use `QualType` and I am not interested in the qualifiers at all.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:208
+  const Type *T = QT.getTypePtr();
+  while (T->isAnyPointerType() || T->isReferenceType() || T->isArrayType()) {
+    ++PtrToHandleLevel;
----------------
NoQ wrote:
> 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?
I do not have a real plan with arrays just yet. Creating eagerly a symbol for all the elements might look a bit wasteful but also users are probably not expected to have large arrays of handles? Probably I should remove that code for now.

I would expect non-Fuchsia checkers to introduce their own `getHandleSymbol` logic. Does that make sense? Maybe I should rename this to `getFuchsiaHandleSymbol`. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:302-303
+                                   Pred);
+        // Avoid further reports on this symbol.
+        State = State->remove<HStateMap>(HandleSym);
+      } else
----------------
NoQ wrote:
> Why not generate a fatal error instead?
The motivation was to not to reduce coverage, but I have no strong feelings about making this a sink. But in case of leaks I certainly prefer non-fatal errors.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:336-338
+ProgramStateRef FuchsiaHandleChecker::evalAssume(ProgramStateRef State,
+                                                 SVal Cond,
+                                                 bool Assumption) const {
----------------
NoQ wrote:
> 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?
Yeah, you got my intention right! I was also thinking about doing this in `checkDeadSymbols`, but I was wondering if that is ultimately the right thing to do. It might be possible to write code where the symbol for the error code becomes dead too late, so we keep the Schrödinger state longer than necessary. Redoing this when we access the handle makes sense though! I think that could probably perform better compared to my approach with `evalAssume`.

My only problem at this point do not really know when is a handle in a Schrödinger state.  Do you recommend specifying a distinct state for that?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:343
+    SymbolRef ErrorSym = CurItem.second.getErrorSym();
+    if (Cmr.isNull(State, ErrorSym).isConstrainedFalse()) {
+      if (CurItem.second.isReleased())
----------------
NoQ wrote:
> If you want this check to be universally applicable, you will probably also need an annotation that explains which statuses indicate an error.
I think the main reason why we use annotations for release/acquire/use because of the potentially large number of calls that deals with handles. The status for success, however, is not a big amount of information, so I think this is easier to be hard coded. It is also less prone to changes. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:348
+      else
+        State = State->remove<HStateMap>(CurItem.first);
+    }
----------------
NoQ wrote:
> I request comments about this path. When does this happen?
Oh, this is to model that maybe a handle allocation failed. I will definitely add a comment and also make the code more narrow to that specific case! 


================
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)
----------------
NoQ wrote:
> 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 :/
I do agree, I also hate this part.


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


================
Comment at: clang/test/Analysis/fuchsia_handle.c:76
+
+void checkNoLeak07(int tag) {
+  zx_handle_t sa, sb;
----------------
NoQ wrote:
> 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.
Indeed! I will make one of the handles "leak".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70470