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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:133-135
+  static HandleState getWithoutError(HandleState S) {
+    return HandleState(S.K, nullptr);
+  }
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:415
+    if (CurItem.second.getErrorSym())
+      SymReaper.markLive(CurItem.first);
+  }
----------------
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.


================
Comment at: clang/test/Analysis/fuchsia_handle.cpp:69
+  zx_status_t status = zx_channel_create(0, &sa, &sb);
+  if (status < 0)
+    return;
----------------
I'd like to see how do notes look when the warning *is* emitted (i.e., same test but replace `status < 0` with `status >= 0`).

We don't have a note for the collapse point of the Schrödinger state, right? (i think it was an unfortunate inevitable use case for a bug visitor because you can't have tags in `evalAssume`)

That is, how comfortable would it be for the user to see that the leak warning is emitted significantly later than the handle is dead? We already aren't great at positioning our leak warnings, but it makes it even more unobvious. I guess it's probably fine, but i want to see it.


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

https://reviews.llvm.org/D73151