<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Jul 11, 2009, at 3:42 AM, Zhongxing Xu wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; ">Hi Ted,<br><br>Here is another fix for this bug. Instead of recovering from a wrong<br>invalidation, this patch aims to invalidate the region correctly. It<br>uses the cast-to type to invalidate the region when available. To<br>avoid invalid cast-to type like 'void*' or 'id', region store now only<br>records non-generic casts of regions.<br><br>On Sat, Jul 11, 2009 at 12:38 PM, Ted Kremenek<<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br><blockquote type="cite">Author: kremenek<br></blockquote><blockquote type="cite">Date: Fri Jul 10 23:38:49 2009<br></blockquote><blockquote type="cite">New Revision: 75356<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=75356&view=rev">http://llvm.org/viewvc/llvm-project?rev=75356&view=rev</a><br></blockquote><blockquote type="cite">Log:<br></blockquote><blockquote type="cite">Handle insidious corner case exposed by RegionStoreManager when handling void* values that are bound<br></blockquote><blockquote type="cite">to symbolic regions and then treated like integers.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Modified:<br></blockquote><blockquote type="cite">   cfe/trunk/lib/Analysis/GRExprEngine.cpp<br></blockquote><blockquote type="cite">   cfe/trunk/test/Analysis/misc-ps.m<br></blockquote></span><br class="Apple-interchange-newline"></blockquote></div><div><br></div><div>Hi Zhongxing,</div><div><br></div><div>I spent some time looking over this patch and testing it's behavior.  I think we're on the right track, but the patch itself isn't where we need it yet.  First I'll make some comments inline to mention some specific points, and then mention some other issues I observed.</div><div><br></div><div><div>Index: include/clang/Analysis/PathSensitive/Store.h</div><div>===================================================================</div><div>--- include/clang/Analysis/PathSensitive/Store.h<span class="Apple-tab-span" style="white-space:pre">       </span>(revision 75492)</div><div>+++ include/clang/Analysis/PathSensitive/Store.h<span class="Apple-tab-span" style="white-space:pre">     </span>(working copy)</div><div>@@ -145,6 +145,10 @@</div><div>     return state;</div><div>   }</div><div> </div><div>+  virtual const QualType *getCastType(const GRState *state, const MemRegion *R){</div><div>+    return 0;</div><div>+  }</div><div>+</div><div><br></div><div>Seems fine.  This is our notion of type information on the side.  It isn't as rich as it needs to be in the long term, but it's a good start.</div><div><br></div><div><br></div><div>   /// EvalBinOp - Perform pointer arithmetic.</div><div>   virtual SVal EvalBinOp(const GRState *state, BinaryOperator::Opcode Op,</div><div>                          Loc lhs, NonLoc rhs, QualType resultTy) {</div><div>Index: lib/Analysis/RegionStore.cpp</div><div>===================================================================</div><div>--- lib/Analysis/RegionStore.cpp<span class="Apple-tab-span" style="white-space:pre">       </span>(revision 75492)</div><div>+++ lib/Analysis/RegionStore.cpp<span class="Apple-tab-span" style="white-space:pre">     </span>(working copy)</div><div>@@ -327,6 +327,10 @@</div><div>   const GRState *setCastType(const GRState *state, const MemRegion* R,</div><div>                              QualType T);</div><div> </div><div>+  const QualType *getCastType(const GRState *state, const MemRegion *R) {</div><div>+    return state->get<RegionCasts>(R);</div><div>+  }</div><div>+</div><div><br></div><div>Makes sense.</div><div><br></div><div><br></div><div>   static inline RegionBindingsTy GetRegionBindings(Store store) {</div><div>    return RegionBindingsTy(static_cast<const RegionBindingsTy::TreeTy*>(store));</div><div>   }</div><div>@@ -348,7 +352,26 @@</div><div> };</div><div> </div><div> } // end anonymous namespace</div><div>+static bool isGenericPtr(ASTContext &Ctx, QualType Ty) {</div><div>+  if (Ty->isObjCIdType() || Ty->isObjCQualifiedIdType())</div><div>+    return true;</div><div>+  while (true) {</div><div>+    Ty = Ctx.getCanonicalType(Ty);</div><div> </div><div>+    if (Ty->isVoidType())</div><div>+      return true;</div><div>+</div><div>+    if (const PointerType *PT = Ty->getAsPointerType()) {</div><div>+      Ty = PT->getPointeeType();</div><div>+      continue;</div><div>+    }</div><div>+</div><div>+    break;</div><div>+  }</div><div>+</div><div>+  return false;</div><div>+}</div><div>+</div><div><br></div><div>I have mixed feelings about this.  The problem is that there are wide range of generic pointers, e.g. "char *" is also a generic pointer.  I guess as long as we are using this just to handle transient type erasure then this seems okay.</div><div><br></div><div> //===----------------------------------------------------------------------===//</div><div> // RegionStore creation.</div><div> //===----------------------------------------------------------------------===//</div><div>@@ -1251,6 +1274,8 @@</div><div> </div><div> const GRState *RegionStoreManager::setCastType(const GRState *state, </div><div> <span class="Apple-tab-span" style="white-space:pre">                                   </span>       const MemRegion* R, QualType T) {</div><div>+  if (isGenericPtr(getContext(), T))</div><div>+    return state;</div><div>   return state->set<RegionCasts>(R, T);</div><div> }</div><div><br></div><div>Makes sense.  I think we should add a comment about why the check for 'isGenericPtr' is there.  Also, we should document that if the region already has a cast type, casting it to void* doesn't remove that cast type.</div><div><br></div><div>What should the semantics be if there already is a cast type associated with a region?</div><div><br></div><div> </div><div>Index: lib/Analysis/Store.cpp</div><div>===================================================================</div><div>--- lib/Analysis/Store.cpp<span class="Apple-tab-span" style="white-space:pre">     </span>(revision 75492)</div><div>+++ lib/Analysis/Store.cpp<span class="Apple-tab-span" style="white-space:pre">   </span>(working copy)</div><div>@@ -235,8 +235,16 @@</div><div> </div><div>   const TypedRegion *TR = cast<TypedRegion>(R);</div><div> </div><div>-  QualType T = TR->getValueType(Ctx);</div><div>+  QualType T;</div><div> </div><div>+  // If the region is cast to another type, use that type.</div><div>+  const QualType *CastTy = getCastType(state, R);</div><div>+  if (CastTy) {</div><div>+    T = (*CastTy)->getAsPointerType()->getPointeeType();</div><div>+  }</div><div><br></div><div>As a nitpick, I strongly prefer the style:</div><div>   </div><div>  if (const QualType *CastTy = ...)</div><div><br></div><div>  to</div><div><br></div><div>  const QualType *CastTy;</div><div>  if (CastTy = ...)</div><div><br></div><div>The first case takes less code and restricts the scope of CastTy to the 'if' statement.  I like restricting scope of local variables to just the place where they are logically used, and no bigger.  It allows people reading the code to understand immediately that the value isn't used later.  It also reduces some subtle programming errors from a variable (incorrectly) being re-used.</div><div><br></div><div>Second, this doesn't handle the case anymore where the value is an Objective-C pointer, since Objective-C pointers are now represented using ObjCObjectPointerType.  That actually might not be an issue since those regions shouldn't be boundable, but it's worth putting in an assertion.</div><div><br></div><div>+  else</div><div>+    T = TR->getValueType(Ctx);</div><div>+</div><div>   if (Loc::IsLocType(T) || (T->isIntegerType() && T->isScalarType())) {</div><div>     SVal V = ValMgr.getConjuredSymbolVal(E, T, Count);</div><div>     return Bind(state, ValMgr.makeLoc(TR), V);</div><div><br></div><div>Looking at the rest of the InvalidateRegion code, I'm actually a little dubious.  I know you didn't write it, but I think we should consider what the general solution should be here.  Consider:</div><div><br></div><div>    int x;</div><div>    struct s *p = (struct s*) &x;</div><div>    foo(p);</div><div><br></div><div>This code might actually be valid if 'sizeof(struct s) <= sizeof(int)' and the alignments of both 'int' and 'struct s' are compatible.  Otherwise, we'll get a potential buffer overflow that we won't catch. Alternatively, we might not actually be doing the right thing.  I think this is something worth discussing.</div><div><br></div><div>Index: lib/Analysis/GRExprEngine.cpp</div><div>===================================================================</div><div>--- lib/Analysis/GRExprEngine.cpp<span class="Apple-tab-span" style="white-space:pre">      </span>(revision 75492)</div><div>+++ lib/Analysis/GRExprEngine.cpp<span class="Apple-tab-span" style="white-space:pre">    </span>(working copy)</div><div>@@ -1119,9 +1119,9 @@</div><div>     //  invalidate(y);  // 'x' now binds to a symbolic region</div><div>     //  int z = *y;</div><div>     //    </div><div>-    if (isa<Loc>(V) && !Loc::IsLocType(Ex->getType())) {</div><div>-      V = EvalCast(V, Ex->getType());</div><div>-    }</div><div>+    //if (isa<Loc>(V) && !Loc::IsLocType(Ex->getType())) {</div><div>+    //  V = EvalCast(V, Ex->getType());</div><div>+    // }</div><div><br></div><div>This is the main change, but I cannot actually evaluate it yet.  Using the '-analyzer-viz-egraph-graphviz' option, I saw that for the following code:</div><div><br></div><div><div>    int *__gruep__ = ((int *)&((b)->grue));</div><div>    int __gruev__ = *__gruep__;</div><div><br></div><div>that all we end up doing is conjuring a new symbol for the second declaration of '__gruev__' (even when using RegionStoreManager).  This can be seen readily with some enhancements I made today to the pretty-printing code for regions and symbols.  This issue may be related to the test case I added to 'misc-ps-region-store.m' (which causes that file to now XFAIL).</div></div><div><br></div><div>I think your patch is a step in the right direction, and aside from the few comments I made I'd be happy with applying it and iterating on the design from there.</div><div><br></div><div>Ted</div></div></body></html>