]> granicus.if.org Git - clang/commitdiff
Revert "[analyzer] Toning down invalidation a bit".
authorArtem Dergachev <artem.dergachev@gmail.com>
Wed, 3 Apr 2019 18:21:16 +0000 (18:21 +0000)
committerArtem Dergachev <artem.dergachev@gmail.com>
Wed, 3 Apr 2019 18:21:16 +0000 (18:21 +0000)
This reverts commit r352473.

The overall idea is great, but it seems to cause unintented consequences
when not only Region Store invalidation but also pointer escape mechanism
was accidentally affected.

Based on discussions in https://reviews.llvm.org/D58121#1452483
and https://reviews.llvm.org/D57230#1434161

Differential Revision: https://reviews.llvm.org/D57230

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@357620 91177308-0d34-0410-b5e6-96231b3b80d8

lib/StaticAnalyzer/Core/CallEvent.cpp
test/Analysis/call-invalidation.cpp
test/Analysis/cxx-uninitialized-object.cpp
test/Analysis/malloc.c
test/Analysis/taint-generic.c
test/Analysis/taint-tester.c

index 8dc6646e0eb27f9b273f1ab5fe2fc2d107a97f22..11dda7c3acb7cf5b06316aa01fb838ef897795fa 100644 (file)
@@ -303,23 +303,11 @@ ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount,
   for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) {
     // Mark this region for invalidation.  We batch invalidate regions
     // below for efficiency.
-    if (const MemRegion *MR = getArgSVal(Idx).getAsRegion()) {
-      bool UseBaseRegion = true;
-      if (const auto *FR = MR->getAs<FieldRegion>()) {
-        if (const auto *TVR = FR->getSuperRegion()->getAs<TypedValueRegion>()) {
-          if (!TVR->getValueType()->isUnionType()) {
-            ETraits.setTrait(MR, RegionAndSymbolInvalidationTraits::
-                                     TK_DoNotInvalidateSuperRegion);
-            UseBaseRegion = false;
-          }
-        }
-      }
-      // todo: factor this out + handle the lower level const pointers.
-      if (PreserveArgs.count(Idx))
-        ETraits.setTrait(
-            UseBaseRegion ? MR->getBaseRegion() : MR,
-            RegionAndSymbolInvalidationTraits::TK_PreserveContents);
-    }
+    if (PreserveArgs.count(Idx))
+      if (const MemRegion *MR = getArgSVal(Idx).getAsRegion())
+        ETraits.setTrait(MR->getBaseRegion(),
+                        RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+        // TODO: Factor this out + handle the lower level const pointers.
 
     ValuesToInvalidate.push_back(getArgSVal(Idx));
 
index dade8db9ac2bc5614c568133831779aba19fa9cb..c107e107054490fd339ec68c302833030ecef12b 100644 (file)
@@ -132,21 +132,18 @@ void testInvalidationThroughBaseRegionPointer() {
   PlainStruct s1;
   s1.x = 1;
   s1.z = 1;
-  s1.y = 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.y == 1); // expected-warning{{TRUE}}
   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{{TRUE}}
-  clang_analyzer_eval(s1.y == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(s1.x == 1); // expected-warning{{UNKNOWN}}
 }
 
 
index 93a02a48382a7bb8fe5eaa10cb1c1999ac033dfc..07006bea478142ccc50218c4d532fb41a29c347c 100644 (file)
@@ -358,7 +358,7 @@ template <class T>
 void wontInitialize(const T &);
 
 class PassingToUnknownFunctionTest1 {
-  int a, b; // expected-note{{uninitialized field 'this->b'}}
+  int a, b;
 
 public:
   PassingToUnknownFunctionTest1() {
@@ -368,7 +368,8 @@ public:
   }
 
   PassingToUnknownFunctionTest1(int) {
-    mayInitialize(a); // expected-warning{{1 uninitialized field at the end of the constructor call}}
+    mayInitialize(a);
+    // All good!
   }
 
   PassingToUnknownFunctionTest1(int, int) {
index cbc21b492e5902774539a2e61f044d4a321e8052..5288e21a282188a4f4b3af4023eaad913d843a60 100644 (file)
@@ -1758,8 +1758,8 @@ void constEscape(const void *ptr);
 void testConstEscapeThroughAnotherField() {
   struct IntAndPtr s;
   s.p = malloc(sizeof(int));
-  constEscape(&(s.x));
-} // expected-warning {{Potential leak of memory pointed to by 's.p'}}
+  constEscape(&(s.x)); // could free s->p!
+} // no-warning
 
 // PR15623
 int testNoCheckerDataPropogationFromLogicalOpOperandToOpResult(void) {
@@ -1812,6 +1812,22 @@ void testLivenessBug(struct B *in_b) {
   livenessBugRealloc(b->a);
 }
 
+struct ListInfo {
+  struct ListInfo *next;
+};
+
+struct ConcreteListItem {
+  struct ListInfo li;
+  int i;
+};
+
+void list_add(struct ListInfo *list, struct ListInfo *item);
+
+void testCStyleListItems(struct ListInfo *list) {
+  struct ConcreteListItem *x = malloc(sizeof(struct ConcreteListItem));
+  list_add(list, &x->li); // will free 'x'.
+}
+
 // ----------------------------------------------------------------------------
 // False negatives.
 
index 42e390dddef2f5b73a9b9986e7dd14a8deae798b..cdac02bf9e37b5ab37312ddca69e5d34a4308fc1 100644 (file)
@@ -238,7 +238,6 @@ void testUnion() {
 
   int sock = socket(AF_INET, SOCK_STREAM, 0);
   read(sock, &tainted.y, sizeof(tainted.y));
-  tainted.x = 0;
   // FIXME: overlapping regions aren't detected by isTainted yet
   __builtin_memcpy(buffer, tainted.y, tainted.x);
 }
index b072eb84d43bd6c0836f697c4d59471254f2334f..3a8cc1825a02928781a62dcfa470328518cfb24e 100644 (file)
@@ -51,7 +51,7 @@ void taintTracking(int x) {
   scanf("%d", &xy.y);
   scanf("%d", &xy.x);
   int tx = xy.x; // expected-warning + {{tainted}}
-  int ty = xy.y; // expected-warning + {{tainted}}
+  int ty = xy.y; // FIXME: This should be tainted as well.
   char ntz = xy.z;// no warning
   // Now, scanf scans both.
   scanf("%d %d", &xy.y, &xy.x);