Page MenuHomePhabricator

[analyzer] Fix crash on getSVal: handle case of CompoundVal
ClosedPublic

Authored by ilya-palachev on Nov 9 2016, 1:59 AM.

Details

Summary

If the pointer to the uninitialized union is casted to the structure of another type, this may lead to the crash in the RegionStore. This patch tries to handle this bug.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-palachev retitled this revision from to [analyzer] Fix crash on getSVal: handle case of CompoundVal.
ilya-palachev updated this object.
ilya-palachev added reviewers: dcoughlin, zaks.anna, NoQ.
ilya-palachev set the repository for this revision to rL LLVM.
ilya-palachev added subscribers: cfe-commits, a.sidorin.
NoQ edited edge metadata.Nov 10 2016, 11:40 AM

Wow, this crash must have been hard to notice!

I think we shouldn't be default-binding non-lazy compound values. Normally we unpack them into field bindings right away, but it seems that nobody cared to implement this for unions.

The current crash goes through RegionStoreManager::bind(),

   2000	    if (Ty->isVectorType())
   2001	      return bindVector(B, TR, V);
   2002	    if (Ty->isUnionType())
-> 2003	      return bindAggregate(B, TR, V);

I think that instead of doing bindAggregate(), we should do something similar to what RegionStoreManager::bindStruct() does (the code that handles compound values should probably be factored out). It should be even easier because unions hold only one value.

I'm not sure if my suggestion has any immediate benefits to overweight its complexity, but it should make improving support for unions easier in the future.

Also, the code for the lazy compound value is really out of place here. All it does is works around the huge and famous FIXME in getBindingForFieldOrElementCommon(). But i don't think we should grow this FIXME into unions.

NoQ accepted this revision.Nov 11 2016, 10:14 AM
NoQ edited edge metadata.

I think this patch is good to land, but if you have time i'd suggest to investigate this a little bit deeper in order to squash even more bugs.

My concern is that we've never implemented reading from a default-bound compound value, because we thought it never happens because we always bind it directly field-by-field instead.

So, since you've found a place where we default-bind a compound value as a whole, it might be great to investigate which consequences does this behavior have. For instance, can we load a field value from a default-bound compound value? If not, it'd be a direct benefit to unpack the value properly upon binding. Eg., an ExprInspection-based test of the form "C c = { 42 }; clang_analyzer_eval(c.x == 42);", where C is a type that triggers default-binding the initializer list (a union?), might expose the problem.

This revision is now accepted and ready to land.Nov 11 2016, 10:14 AM

@NoQ , @ilya-palachev - what are the plans for this ?

This revision was automatically updated to reflect the committed changes.