This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] do not crash on subscripts into ObjC properties
ClosedPublic

Authored by george.karpenkov on Nov 28 2017, 3:20 PM.

Details

Summary

Subscripts into ObjC properties can be achieved by using extension CUDA types.
In that case, subscripts are not allowed to be lvalues, so we relax the assumption that array subscript expression is always an lvalue in that particular case.

Fixes (or at least prevents crashing) in rdar://34829842

Diff Detail

Event Timeline

@dcoughlin @NoQ thoughts? Option B is to relax the assertion when the value is unknown.

Update: change the logic, do nothing if the index result is an rvalue.

dcoughlin added inline comments.Nov 30 2017, 9:08 PM
lib/StaticAnalyzer/Core/ExprEngine.cpp
2136

This drops the flow when A is an rvalue. No code after the subscript expression will be reached. You need to make sure that Pred (or its successors) make it into Dst. You should add a test for this.

This also doesn't perform the pre- and post- statement checker callbacks, which we still need to do for the rvalue case. One way to do this is to move the check for the lvalue into VisitLvalArraySubscriptExpr() (renaming it, of course) and only construct the lvalue SVal when A is an lvalue.

george.karpenkov updated this revision to Diff 125374.
NoQ added inline comments.Dec 4 2017, 11:05 AM
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
335–337

Indent :)

lib/StaticAnalyzer/Core/ExprEngine.cpp
2130

Does this deserve a FIXME?

2151

So we screw all vectors, not just the ObjC rvalue thingies. Not sure if they worked before in any cases, but i guess we could be more careful here.

test/Analysis/vector.m
41

Just curious, did V not work?

49

Devin wanted a coverage test, i.e. show how we continue our analysis after encountering i2.v[0]. We can divide by zero after obtaining the value, or just use ExprInspection's clang_analyzer_warnIfReached().

george.karpenkov marked 2 inline comments as done.
george.karpenkov marked an inline comment as done.
george.karpenkov added inline comments.
lib/StaticAnalyzer/Core/ExprEngine.cpp
2130

actually, the comment was misplaced, and probably yes.

2151

yeah that's a mistake. It is meant to be non-glvalue vectors.

dcoughlin accepted this revision.Dec 4 2017, 6:16 PM

Minor nits inline. Other than that, looks great to me.

lib/StaticAnalyzer/Core/ExprEngine.cpp
2144

I think it would great to add a comment here describing when the "like" kicks in (for example, taking the address of a subscript expression on a void * base).

test/Analysis/vector.m
54

I don't think there is anything special about Objective-C properties here -- the problem manifests on all function calls. Do you think we need to move the test into a '.m' and test both the property and function return case? Or is the function return case sufficient?

58

clang_analyzer_warnIfReached() is more idiomatic here.

This revision is now accepted and ready to land.Dec 4 2017, 6:16 PM
george.karpenkov marked 2 inline comments as done.Dec 5 2017, 1:05 PM
george.karpenkov added inline comments.
test/Analysis/vector.m
54

I would prefer to leave it here.
Obj-C properties are indeed covered by the same if-branch as the return case, but I think it does not hurt to have an extra test case, as such situations are relatively rare.

This revision was automatically updated to reflect the committed changes.