This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Extend the ObjCAutoreleaseWriteChecker to warn on captures as well
ClosedPublic

Authored by george.karpenkov on May 11 2018, 1:41 PM.

Details

Summary

A common pattern is that the code in the block does not write into the variable explicitly, but instead passes it to a helper function which performs the write.

Diff Detail

Event Timeline

NoQ added a comment.May 14 2018, 1:47 PM

Sounds pretty aggressive. Are we sure that it's never a good idea to pass the pointer around? I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.

The code looks reasonable.

lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp
118–119

/dance

*
 *
150–156

Hmm, do we need an equalsBoundNode here? Why don't we simply inline the matcher?

169

Would this be better with ignoringParenImpCasts?

Or we could as well stop on parentheses as a weird suppression mechanism for the users.

Sounds pretty aggressive

Actually, it's much less aggressive than the Clang warning, firing on *every* capture of autoreleasing variable in a block [unless you explicitly add an __autoreleasing lifetime specifier suppressing it]

Are we sure that it's never a good idea to pass the pointer around?

It's an almost guaranteed use-after-free if an autoreleasing pointer is written into inside the autoreleasing block (almost because maybe the pointer is never read from again).
It's OK to read it -- but then the user should pass *pointer, as that is sufficient. Passing just pointer almost guarantees a writing intent.

I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.

It is captured in a block: the warning fires when we use a wrapper to write to the autoreleasing variable *already inside the block running in the autoreleasing pool*.

NoQ added a comment.May 14 2018, 1:53 PM

I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.

It is captured in a block: the warning fires when we use a wrapper to write to the autoreleasing variable *already inside the block running in the autoreleasing pool*.

Aha, ok, yup, sounds reasonable!

george.karpenkov marked 2 inline comments as done.May 14 2018, 1:57 PM
george.karpenkov added inline comments.
lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp
150–156

Avoids mega-expressions, and we need the binding anyway.

NoQ added inline comments.May 14 2018, 2:05 PM
lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp
150–156

I mean, declRefExpr(to(DoublePointerParamM)) sounds pretty short.

And even if we need the binding, having equalsBoundNode() specifically is making things weirder because i'm starting to suspect that we're matching a pattern with backreferences while reading the code, even though in fact we don't.

george.karpenkov marked an inline comment as done.
NoQ accepted this revision.May 14 2018, 2:15 PM

Thx!

This revision is now accepted and ready to land.May 14 2018, 2:15 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.