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

[ilya-palachev]

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.

[NoQ]

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]

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.

[alexander-shaposhnikov]

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

lib/StaticAnalyzer/Core/RegionStore.cpp

if (const Optional<SVal> &D = B.getDefaultBinding(superR)) {
     if (val.isZeroConstant())
       return svalBuilder.makeZeroVal(Ty);
 
     if (val.isUnknownOrUndef())
       return val;
 
     // Lazy bindings are usually handled through getExistingLazyBinding().
     // We should unify these two code paths at some point.
-    if (val.getAs<nonloc::LazyCompoundVal>())
+    if (val.getAs<nonloc::LazyCompoundVal>() ||
+        val.getAs<nonloc::CompoundVal>())
       return val;
 
     llvm_unreachable("Unknown default value");
   }
 
   return None;
 }
 

test/Analysis/uninit-vals-union.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core.builtin -analyzer-store=region -verify -Wno-unused %s
+
+typedef union {
+  int y;
+} U;
+
+typedef struct { int x; } A;
+
+void foo() {
+  U u = {};
+  A *a = &u; // expected-warning{{incompatible pointer types}}
+  a->x;      // no-crash
+}