[cfe-commits] r75356 - in /cfe/trunk: lib/Analysis/GRExprEngine.cpp test/Analysis/misc-ps.m

[Zhongxing Xu]

On Tue, Jul 14, 2009 at 2:32 PM, Ted Kremenek<kremenek at apple.com> wrote:
>
> On Jul 13, 2009, at 6:28 PM, Zhongxing Xu wrote:
>
> 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.
>
> What should the semantics be if there already is a cast type associated with
>
> a region?
>
> I think it's seldom that programmer would cast it back to generic
> pointer intentionally. Often it is cast back to generic
> pointer passively. E.g.:
>
> void invalidate(void* p);
> int *p = (int*)alloca();
> void *q = (void*) p; // people don't do this.
> invalidate(p); // people often do this.
>
> Agreed.  Should we also consider 'char *' as a case in 'isGenericPtr'?

If this happens in real code, we can add it.

>
> 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.
>
> The check is done (or should be done) in NewCastRegion(). If the cast
> is legal, then type 'struct s' is used to
> invalidate the region. I actually am not very clear about what you are
> worrying about here.
>
> I think you're right.  I think I may have thought too hard about it and
> confused myself.
>
>
>
> Index: lib/Analysis/GRExprEngine.cpp
>
> ===================================================================
>
> --- lib/Analysis/GRExprEngine.cpp (revision 75492)
>
> +++ lib/Analysis/GRExprEngine.cpp (working copy)
>
> @@ -1119,9 +1119,9 @@
>
>      //  invalidate(y);  // 'x' now binds to a symbolic region
>
>      //  int z = *y;
>
>      //
>
> -    if (isa<Loc>(V) && !Loc::IsLocType(Ex->getType())) {
>
> -      V = EvalCast(V, Ex->getType());
>
> -    }
>
> +    //if (isa<Loc>(V) && !Loc::IsLocType(Ex->getType())) {
>
> +    //  V = EvalCast(V, Ex->getType());
>
> +    // }
>
> 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:
>
>     int *__gruep__ = ((int *)&((b)->grue));
>
>     int __gruev__ = *__gruep__;
>
> that all we end up doing is conjuring a new symbol for the second
>
> declaration of '__gruev__' (even when using RegionStoreManager).
>
> This is expected. We invalidate *__gruep__ with a conjured symbol.
>
> Right!  Once again I confused myself; there is a difference between the
> conjured symbols used by RegionStore and the ones used to recover
> path-sensitivity in GRExprEngine.  It would be nice if if we had a way to
> distinguish them.

Agreed. It's worth to distinguish symbols in different usages.

> Just as a test, I augmented my copy of 'testB' to contain two infeasible
> null dereferences.  With RegionStore we get no null dereference errors (as
> it should be).  BasicStore flags two:
> typedef struct _BStruct { void *grue; } BStruct;
> void testB_aux(void *ptr);
> void testB(BStruct *b) {
>   {
>     int *__gruep__ = ((int *)&((b)->grue));
>     int __gruev__ = *__gruep__;
>     int __gruev2__ = *__gruep__;
>     if (__gruev__ != __gruev2__) {
>       int *p = 0;
>       *p = 0xDEADBEEF;
>     }
>
>     testB_aux(__gruep__);
>   }
>   {
>     int *__gruep__ = ((int *)&((b)->grue));
>     int __gruev__ = *__gruep__;
>     int __gruev2__ = *__gruep__;
>     if (__gruev__ != __gruev2__) {
>       int *p = 0;
>       *p = 0xDEADBEEF;
>     }
>
>     if (~0 != __gruev__) {}
>   }
> }