[analyzer] Let TK_PreserveContents span across the whole base region.

[NoQ]

Summary

Essentially, if s is a structure, and foo(const void *) is evaluated conservatively, then foo(&s) does not invalidate s, but foo(&(s.x)) invalidates the whole s, because the store only looks at traits of base regions (inside binding keys), and s.x is a field region.

This patch represents the idea that only whole base regions should carry the TK_PreserveContents trait. This also makes a bit of sense, because no matter what pointer arithmetic we do with a const pointer, it's still a const pointer. There's an extra complication with mutable fields in C++ classes, which i neither added nor fixed here.

In CallEvent.cpp below the changed code there's a FIXME comment, but i'm not sure what it means; if anybody thinks it means exactly what this patch is about, then i'd have to update it :)

What i don't like about the approach this patch implements, is that it makes the core rely on an implementation detail of RegionStoreManager ("only base regions are relevant" is such implementation detail). Instead, i also tried to add a few extra virtual methods into the StoreManager to avoid this problem, but it made the patch much heavier. I can post that, unless anybody else thinks that it's a natural thing (rather than implementation detail) to propagate this trait to base regions.

Instead, it should be possible to auto-replace the region with a base region inside setTrait() and hasTrait() methods.

[zaks.anna]

LGTM!

One thing to be aware here is that a const pointer could be deleted, so we should be able to delete a parent object without a warning. (I think that should work with this patch since you don't change the trait, but you might want to add a test.)

What i don't like about the approach this patch implements, is that it makes the core rely on an
implementation detail of RegionStoreManager ("only base regions are relevant" is such implementation
detail). Instead, i also tried to add a few extra virtual methods into the StoreManager to avoid this
problem, but it made the patch much heavier. I can post that, unless anybody else thinks that it's a
natural thing (rather than implementation detail) to propagate this trait to base regions.

I'd commit this patch. If you want, feel free to follow up with another to remove the leaking of the implementation detail. I do not fully understand what the alternate solution is... I do not think it would make sense to automatically apply the TK_PreserveContents trait to the base region when one calls ETraits.setTrait(); not sure if you are suggesting that.

lib/StaticAnalyzer/Core/CallEvent.cpp

182 ↗

(On Diff #53551)

Not sure, but this could mean to deal with cases where you pass a non const pointer to a class that has const fields..

cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

if (Optional<loc::MemRegionVal> MR = L->getAs<loc::MemRegionVal>()) {
     // Invalidate this region.
     const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
 
     bool CausesPointerEscape = false;
     RegionAndSymbolInvalidationTraits ITraits;
     // Invalidate and escape only indirect regions accessible through the source
     // buffer.
     if (IsSourceBuffer) {
-      ITraits.setTrait(R,
+      ITraits.setTrait(R->getBaseRegion(),
                        RegionAndSymbolInvalidationTraits::TK_PreserveContents);
       ITraits.setTrait(R, RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
       CausesPointerEscape = true;
     } else {
       const MemRegion::Kind& K = R->getKind();
       if (K == MemRegion::FieldRegionKind)
         if (Size && IsFirstBufInBound(C, state, E, Size)) {
           // If destination buffer is a field region and access is in bound,

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount,
   if (!argumentsMayEscape())
     findPtrToConstParams(PreserveArgs, *this);
 
   for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) {
     // Mark this region for invalidation.  We batch invalidate regions
     // below for efficiency.
     if (PreserveArgs.count(Idx))
       if (const MemRegion *MR = getArgSVal(Idx).getAsRegion())
-        ETraits.setTrait(MR->StripCasts(),
+        ETraits.setTrait(MR->getBaseRegion(),
                         RegionAndSymbolInvalidationTraits::TK_PreserveContents);
         // TODO: Factor this out + handle the lower level const pointers.
 
     ValuesToInvalidate.push_back(getArgSVal(Idx));
   }
 
   // Invalidate designated regions using the batch invalidation API.
   // NOTE: Even if RegionsToInvalidate is empty, we may still invalidate

cfe/trunk/test/Analysis/call-invalidation.cpp

void testPureConst() {
   clang_analyzer_eval(global == -5); // expected-warning{{TRUE}}
 
   usePointer(&p);
   clang_analyzer_eval(x == 42); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(global == -5); // expected-warning{{UNKNOWN}}
 }
 
 
+struct PlainStruct {
+  int x, y;
+  mutable int z;
+};
+
+PlainStruct glob;
+
+void useAnything(void *);
+void useAnythingConst(const void *);
+
+void testInvalidationThroughBaseRegionPointer() {
+  PlainStruct s1;
+  s1.x = 1;
+  s1.z = 1;
+  clang_analyzer_eval(s1.x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(s1.z == 1); // expected-warning{{TRUE}}
+  // Not only passing a structure pointer through const pointer parameter,
+  // but also passing a field pointer through const pointer parameter
+  // should preserve the contents of the structure.
+  useAnythingConst(&(s1.y));
+  clang_analyzer_eval(s1.x == 1); // expected-warning{{TRUE}}
+  // FIXME: Should say "UNKNOWN", because it is not uncommon to
+  // modify a mutable member variable through const pointer.
+  clang_analyzer_eval(s1.z == 1); // expected-warning{{TRUE}}
+  useAnything(&(s1.y));
+  clang_analyzer_eval(s1.x == 1); // expected-warning{{UNKNOWN}}
+}
+
+
+void useFirstConstSecondNonConst(const void *x, void *y);
+void useFirstNonConstSecondConst(void *x, const void *y);
+
+void testMixedConstNonConstCalls() {
+  PlainStruct s2;
+  s2.x = 1;
+  useFirstConstSecondNonConst(&(s2.x), &(s2.y));
+  clang_analyzer_eval(s2.x == 1); // expected-warning{{UNKNOWN}}
+  s2.x = 1;
+  useFirstNonConstSecondConst(&(s2.x), &(s2.y));
+  clang_analyzer_eval(s2.x == 1); // expected-warning{{UNKNOWN}}
+  s2.y = 1;
+  useFirstConstSecondNonConst(&(s2.x), &(s2.y));
+  clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}}
+  s2.y = 1;
+  useFirstNonConstSecondConst(&(s2.x), &(s2.y));
+  clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}}
+}

cfe/trunk/test/Analysis/malloc.c

 } //expected-warning{{Potential leak}}
 
 void testEscapeThroughSystemCallTakingVoidPointer3(fake_rb_tree_t *rbt) {
   int *data = (int *)malloc(32);
   fake_rb_tree_init(rbt, data);
   fake_rb_tree_insert_node(rbt, data); // no warning
 }
 
+struct IntAndPtr {
+  int x;
+  int *p;
+};
+
+void constEscape(const void *ptr);
+
+void testConstEscapeThroughAnotherField() {
+  struct IntAndPtr s;
+  s.p = malloc(sizeof(int));
+  constEscape(&(s.x)); // could free s->p!
+} // no-warning
+
 // ----------------------------------------------------------------------------
 // False negatives.
 
 void testMallocWithParam(int **p) {
   *p = (int*) malloc(sizeof(int));
   *p = 0; // FIXME: should warn here
 }
 
 void testMallocWithParam_2(int **p) {
   *p = (int*) malloc(sizeof(int)); // no-warning
 }
 
 void testPassToSystemHeaderFunctionIndirectly() {
   int *p = malloc(4);
   p++;
   fakeSystemHeaderCallInt(p);
   // FIXME: This is a leak: if we think a system function won't free p, it
   // won't free (p-1) either.
 }
+
+void testMallocIntoMalloc() {
+  StructWithPtr *s = malloc(sizeof(StructWithPtr));
+  s->memP = malloc(sizeof(int));
+  free(s);
+} // FIXME: should warn here