[cfe-commits] r120177 - /cfe/trunk/lib/Checker/ReturnPointerRangeChecker.cpp

[Ted Kremenek]

Hmm.  I think I see what you mean.  I agree that removing the use of StripCasts() is appropriate.

That said, do you think both ReturnPointerRangeChecker and ArrayBoundChecker are doing the bounds checks in the most appropriate way?  I think the motivation for using StripCasts() (even though it was wrong) was to try and reason about out-of-bounds accesses using the extent of the raw memory region.

For example, this is what is going on in RegionStore::getSizeInElements():

  DefinedOrUnknownSVal RegionStoreManager::getSizeInElements(const GRState *state,
                                                             const MemRegion *R,
                                                             QualType EleTy) {
    SVal Size = cast<SubRegion>(R)->getExtent(ValMgr);
    SValuator &SVator = ValMgr.getSValuator();
    ...

I think the motivation for using StripCasts() what so that we were doing the bounds check relative to the base memory region (for which we have an extent).  By removing the use of StripCasts(), any time (I believe) we introduce an ElementRegion due to a cast this bounds check won't work.

Should we instead be doing the bounds check in terms of raw offsets (relative to the underlying base region)?


On Nov 27, 2010, at 11:49 PM, Xu Zhongxing wrote:

> No. I want to reduce the uses of StripCasts. It shouldn't be used everywhere. Here we should check the zero index explicitly.
> 
> On Sun, Nov 28, 2010 at 3:45 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Hi Zhongxing,
> 
> What is the motivation for this change?  Did the previous logic cause a test case to fail?
> 
> On Nov 26, 2010, at 1:07 AM, Zhongxing Xu <xuzhongxing at gmail.com> wrote:
> 
> > Author: zhongxingxu
> > Date: Fri Nov 26 03:07:38 2010
> > New Revision: 120177
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=120177&view=rev
> > Log:
> > Should not use StripCasts() in this context.
> >
> > Modified:
> >    cfe/trunk/lib/Checker/ReturnPointerRangeChecker.cpp
> >
> > Modified: cfe/trunk/lib/Checker/ReturnPointerRangeChecker.cpp
> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Checker/ReturnPointerRangeChecker.cpp?rev=120177&r1=120176&r2=120177&view=diff
> > ==============================================================================
> > --- cfe/trunk/lib/Checker/ReturnPointerRangeChecker.cpp (original)
> > +++ cfe/trunk/lib/Checker/ReturnPointerRangeChecker.cpp Fri Nov 26 03:07:38 2010
> > @@ -48,19 +48,16 @@
> >
> >   SVal V = state->getSVal(RetE);
> >   const MemRegion *R = V.getAsRegion();
> > -  if (!R)
> > -    return;
> > -
> > -  R = R->StripCasts();
> > -  if (!R)
> > -    return;
> >
> >   const ElementRegion *ER = dyn_cast_or_null<ElementRegion>(R);
> >   if (!ER)
> >     return;
> >
> >   DefinedOrUnknownSVal Idx = cast<DefinedOrUnknownSVal>(ER->getIndex());
> > -
> > +  // Zero index is always in bound, this also passes ElementRegions created for
> > +  // pointer casts.
> > +  if (Idx.isZeroConstant())
> > +    return;
> >   // FIXME: All of this out-of-bounds checking should eventually be refactored
> >   // into a common place.
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20101128/31df92b9/attachment.html>