[PATCH] D73151: [analyzer] Fix handle leak false positive when the handle dies too early

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

xazax.hun marked 2 inline comments as done.
xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:133-135
+  static HandleState getWithoutError(HandleState S) {
+    return HandleState(S.K, nullptr);
+  }
----------------
NoQ wrote:
> It already makes me mildly uncomfortable that our data is not "normalized", i.e. you can indicate the Schrödinger state by either adding an error symbol or changing from `Allocated` to `MaybeAllocated`. Do i understand it correctly that you're now adding a special state where the handle is still `MaybeAllocated` but there's no error symbol? Please comment this up.
Yeah, the reason is that, when the handle is a return value, we have no idea where the error symbol is. This would only happen if someone do not follow the API guidelines (in Fuchsia). I'll add a comment.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:415
+    if (CurItem.second.getErrorSym())
+      SymReaper.markLive(CurItem.first);
+  }
----------------
NoQ wrote:
> I think it should be possible to implement it without `checkLiveSymbols`, because i don't have any reasons to believe that you can find the handle symbol by looking at the error symbol, or vice versa, so i think they aren't supposed to keep each other alive.
> 
> I.e., i think you can implement this by intentionally leaving zombie handles in your state until the respective error symbol dies. After all, you don't rely on any other metadata attached to your handles (eg., range constraints), you only need your map, right?
> 
> I'm open to discussion here. I understand that intentional zombies sound scary, but they also look slightly more correct to me. //"The handle is definitely dead by now, but the successfulness of its birth is still a 'known unknown', so let's delay our warning and keep the corpse frozen until we find out if it was ever born to begin with"// - sounds about right :/ It's probably not a big deal in any case, but i'm curious WDYT.
I see, yeah this makes sense to me. I will try this approach out and report back :)


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

https://reviews.llvm.org/D73151