Page MenuHomePhabricator

[analyzer] Fix handle leak false positive when the handle dies too early
ClosedPublic

Authored by xazax.hun on Jan 21 2020, 5:29 PM.

Details

Summary

In case the handle symbol dies too early, even before we check the status, we might generate spurious leak warnings.

This pattern is not very common in production code but it is very common in unittests where we do know that certain syscall will fail.

Diff Detail

Event Timeline

xazax.hun created this revision.Jan 21 2020, 5:29 PM
xazax.hun updated this revision to Diff 239469.Jan 21 2020, 5:36 PM
  • Minor refactoring.
NoQ added inline comments.Jan 24 2020, 3:07 PM
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
133–135

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.

433

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.

clang/test/Analysis/fuchsia_handle.cpp
72

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.

xazax.hun marked 2 inline comments as done.Jan 24 2020, 3:21 PM
xazax.hun added inline comments.
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
133–135

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.

433

I see, yeah this makes sense to me. I will try this approach out and report back :)

xazax.hun marked 4 inline comments as done.Jan 24 2020, 5:27 PM
xazax.hun added inline comments.
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
133–135

Sorry, I was hasty and my head was full of another patch. It has nothing to do with returned handles. I used this to control when to let some symbols die. But with your suggested approach this factory is no longer needed.

xazax.hun updated this revision to Diff 240337.Jan 24 2020, 5:27 PM
  • Address review comments.
xazax.hun added a comment.EditedJan 27 2020, 8:32 AM

I've rerun this on Fuchsia and it got the same results as the previous approach :).

NoQ accepted this revision.Jan 27 2020, 9:39 AM

Yay, it actually worked!

This revision is now accepted and ready to land.Jan 27 2020, 9:39 AM
This revision was automatically updated to reflect the committed changes.
jroelofs added inline comments.
clang/test/Analysis/fuchsia_handle.cpp
80

@xazax.hun, @dcoughlin Is it expected that these diagnostics be non-deterministic? I'm seeing this test case fail sporadically [1] because sometimes the analyzer sees that the 3rd argument leaks, and other times it sees that the 2nd does.

The easy fix here would be to adjust the regex to be flexible against that difference, but I didn't want to cover up an underlying issue by committing that blindly:

// expected-note@-1 {{Handle allocated through (2nd|3rd) parameter}}

1: http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-expensive/17048/console

jroelofs added inline comments.Jul 28 2020, 4:30 PM
clang/test/Analysis/fuchsia_handle.cpp
80
NoQ added inline comments.Jul 28 2020, 6:47 PM
clang/test/Analysis/fuchsia_handle.cpp
80

Hmm, no, these diagnostics shouldn't be non-deterministic. It's a bug. Thanks a lot for noticing!

Non-determinism in *notes* shouldn't be too bad. Also investigation may get pretty long because diagnostic sorting and de-duplication is a highly automated process. I believe we should cover up the problem to suppress buildbot failures and investigate when we can. I'll commit the cover-up.

NoQ added inline comments.Jul 28 2020, 6:57 PM
clang/test/Analysis/fuchsia_handle.cpp
80