This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
ClosedPublic

Authored by rnkovacs on Jul 22 2018, 8:56 PM.

Details

Summary

According to the standard, pointers referring to the elements of a basic_string sequence may also be invalidated if they are used as an argument to any standard library function taking a reference to non-const basic_string as an argument. This patch makes InnerPointerChecker warn for these cases as well.

Diff Detail

Event Timeline

rnkovacs created this revision.Jul 22 2018, 8:56 PM
xazax.hun requested changes to this revision.Jul 23 2018, 9:01 AM

Some comments, mostly nits inline.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
131–132

Nit: This return is redundant.

194–198

Nit: no need for braces here.

200

Nit: redundant return.

210

Shouldn't we also check if the function is a standard library function? Or do we assume that user functions also invalidate the strings?

211

I am not sure if we always have a Decl here, I am afraid this might return null sometimes. Please add a test case with a function pointer (received as an argument in a top level function).

220

Nit: redundant return.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2934

I think getDirectCallee might fail and return nullptr. One more reason to test function pointers :)

This revision now requires changes to proceed.Jul 23 2018, 9:01 AM
NoQ added a comment.Jul 23 2018, 9:54 AM

I've just one thing to add.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
131–132

Because of how easy it is to accidentally split the state, i'm on a brink of declaring return after addTransition a good practice.

205–217

I believe that this should also go into PostCall. Symbols aren't released until some point within the call.

210

That's right, it's an important thing to check.

xazax.hun added inline comments.Jul 23 2018, 10:04 AM
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
205–217

Oh, I see. But is it guaranteed that the symbols are not garbage collected until post call? Also, the environment will always contain all the bindings for the arguments?

rnkovacs updated this revision to Diff 156938.Jul 23 2018, 5:29 PM
rnkovacs marked 11 inline comments as done.

Addressed comments & added two test cases for function pointers.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
211

Um, yes, I'd already seen a crash here on a project before I saw your comment. Thanks!

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2934

You're right. Also, it needed a bit more effort to dig up the function pointer's name. Or should I go further and somehow find out the name of the function it points to?

NoQ added inline comments.Jul 23 2018, 5:31 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2934

Also, it needed a bit more effort to dig up the function pointer's name.

I think it should work out of the box; Call.getDecl() is smart enough to show the right decl when a pointer to a concrete function is being called on the current path.

NoQ added inline comments.Jul 23 2018, 5:58 PM
test/Analysis/inner-pointer.cpp
41–42

Without a definition for this thing, we won't be able to test much. I suggest you to take a pointer to a function directly, i.e. define a std::swap<T>(x, y) somewhere and take a &std::swap<std::string> directly and call it (hope it actually works this way).

NoQ added inline comments.Jul 23 2018, 6:11 PM
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
207–208

We need tests for operators here, due to the problem that i recently encountered in D49627: there's different numbering for arguments and parameters within in-class operators. Namely, this-argument is counted as an argument (shifting other argument indices by 1) but not as a parameter.

207–208

(i.e., tests and most likely some actual code branch)

rnkovacs updated this revision to Diff 157278.Jul 25 2018, 8:22 AM
rnkovacs marked 2 inline comments as done.

Fix note for function pointers & handle argument counting in member operator calls.
I also refactored the code a little, because after moving things from checkPreCall to checkPostCall, the structure was a bit confusing.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
207–208

I did the coding part, but for the tests, I'm struggling to find an example of a member operator in the STL that takes a non-const string reference as an argument. Could you perhaps help me out here?

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2934

That worked, thanks!

test/Analysis/inner-pointer.cpp
41–42

I think I am missing something. This is meant to test that the checker recognizes standard library function calls taking a non-const reference to a string as an argument. At L314, func_ref is called with a string argument, and the checker seems to do all the things it should after the current patch, emitting the new kind of note and such.

I can surely add the std::swap<T>(x, y) definition too, but then the analyzer will see that the pointer was reallocated at the assignment inside the function, and place the note there. I think that would test the previous string-member-function patch more than this one.

Small comments inline.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
149–161

Nit: maybe using FunctionDecl::parameters and range based for loop is sligtly cleaner.

167

Nit: maybe using isa + bool is cleaner here?

167–193

While I cannot recall examples in the STL the number of arguments and parameters might differ. We might have ellipsis in the parameters and this way we may pass more arguments. Since we do not have the parameter type for this case, I think it is ok to not handle it. But it might be worth to have a comment. The other case, when we have default arguments. I do not really know how the analyzer behaves with default arguments but maybe it would be worth to add a test case to ensure we do not crash on that?

rnkovacs updated this revision to Diff 157286.Jul 25 2018, 9:13 AM
rnkovacs marked an inline comment as done.

Tiny bit more re-structuring.

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
149–161

Given that I am manipulating indices below, I feel that actually the plain for loop is a bit simpler here, what do you think?

xazax.hun added inline comments.Jul 25 2018, 9:17 AM
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
149–161

Indeed, I am fine with the old style loop too.

NoQ accepted this revision.Jul 25 2018, 12:56 PM

Looks great!

lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
167–193

The analyzer behaves pretty badly with default arguments; we don't really model them unless they are like plain integer literals. But that's not an issue here because a default parameter/argument is still both a parameter and an argument (where the argument expression takes the form of CXXDefaultArgExpr).

207–208

Mmm mmmmm right, dunno. I thought operator>> would be our main example, but it's apparently non-member for a string, even though it is defined as a member for primitive types.

I guess let's skip the test until we find an example. We did our best to work around potential future problems, that's good.

test/Analysis/inner-pointer.cpp
41–42

Mm, aha, ok, i misunderstood this test. I thought you're trying to define something like std::function and try to see if calling a function pointer wrapped into that thing works. No idea why i was thinking that, sry.

rnkovacs updated this revision to Diff 157809.Jul 27 2018, 5:04 PM
rnkovacs marked an inline comment as done.
rnkovacs added inline comments.
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
167–193

Hm, it seems that passing objects of non-trivial types like std::string to a variadic function is implementation defined, and for clang 6.0, this means that it does not compile (does compile with gcc 8.1 though, example).

Added a test for the default parameter case.

NoQ added inline comments.Jul 27 2018, 5:09 PM
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
167–193

Yup, that's correct; i noticed it a few days ago too in https://reviews.llvm.org/D49443#1170831.

You can disable the warning/error with -Wno-non-pod-varargs for the purposes of testing, but you anyway won't be able to obtain the AST you'll want to test, so i don't think this requires a test.

This revision is now accepted and ready to land.Jul 27 2018, 6:52 PM
This revision was automatically updated to reflect the committed changes.