[PATCH] D11832: [Patch] [Analyzer] false positive: Potential leak connected with memcpy (PR 22954)

[pierre gousseau via cfe-commits]

pgousseau added a comment.

In http://reviews.llvm.org/D11832#224929, @dcoughlin wrote:

> You should consider what should happen when the memcpy may write past the end of the fixed-size array and add tests that specify correct behavior for these cases. An important example is:
>
>   struct Foo {
>     char data[4];
>     int i;
>   };
>   
>   Foo f;
>   f.i = 10;
>  
>   memcpy(f.data, someBuf, 100);
>   
>   clang_analyzer_eval(f.i == 10); // What should this yield?
>   
>
> I think it is also important to add tests for regions at symbolic offsets, for bindings in the super region having keys with symbolic offsets, and for cases where there is potential aliasing and casting between regions with symbolic offsets.


Yes it seems I overlooked symbolic offsets in the test cases, will handle those.

> Also, Jordan wrote up a description of the region store in docs/analyzer/RegionStore.txt that you might find helpful if you haven't already seen it.


Very usefull thanks !


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:746
@@ -739,1 +745,3 @@
+    return static_cast<DERIVED*>(this)->hasTrait(MR, IK);
+  }
 };
----------------
ayartsev wrote:
> Hmm.. Either we completely encapsulate 'RegionAndSymbolInvalidationTraits' in the 'invalidateRegionsWorker' class or we move 'RegionAndSymbolInvalidationTraits' (maybe renamed to the more general name) to the base 'ClusterAnalysis' class. 
> In the suggested solution you on the one hand make processing of 'RegionAndSymbolInvalidationTraits' specific to 'invalidateRegionsWorker' class but on the other hand explicitly refer to 'RegionAndSymbolInvalidationTraits::InvalidationKinds' and  'RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion' in the 'ClusterAnalysis' class.
> It seems to me the proper way to encapsulate 'RegionAndSymbolInvalidationTraits' in the 'invalidateRegionsWorker' is to just override 'AddToWorkList()' in subclasses (as you done with 'hasTrait()').
I agree yes overriding AddToWorkList would fit better, will upload new patch. Thanks !

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1110
@@ +1109,3 @@
+      assert(RO.getOffset() >= 0 && "Offset should not be negative");
+      uint64_t LowerOffset = RO.getOffset();
+      uint64_t UpperOffset = LowerOffset + *NumElements * ElemSize;
----------------
dcoughlin wrote:
> R0.getOffset() will assert if R0 is a symbolic region offset. This can happen if the invalidated array is itself in an array (e.g., someOtherArray[i].array) or is in a union.
Yes that definitely needs fixing. Thanks !

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1118
@@ +1117,3 @@
+               ++I) {
+            uint64_t ROffset = I.getKey().getOffset();
+            if (ROffset >= LowerOffset && ROffset <= UpperOffset)
----------------
dcoughlin wrote:
> getOffset() here will assert also if there is any key with a symbolic offset in SuperR.
Will fix. Thanks !

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1119
@@ +1118,3 @@
+            uint64_t ROffset = I.getKey().getOffset();
+            if (ROffset >= LowerOffset && ROffset <= UpperOffset)
+              B = B.removeBinding(I.getKey());
----------------
dcoughlin wrote:
> Should this be ROffset < UpperOffset?
Yes, will change to (ROffset < UpperOffset || (LowerOffset == UpperOffset && ROffset == LowerOffset)).
To handle arrays of 0 elements and arrays of 0 sized elements.
Thanks !


http://reviews.llvm.org/D11832