[cfe-commits] r63347 - in /cfe/trunk: include/clang/Analysis/PathSensitive/BasicValueFactory.h include/clang/Analysis/PathSensitive/MemRegion.h include/clang/Analysis/PathSensitive/SVals.h lib/Analysis/MemRegion.cpp lib/Analysis/RegionStore.cpp l

[Zhongxing Xu]

On Fri, Jan 30, 2009 at 3:51 PM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Jan 29, 2009, at 9:15 PM, Zhongxing Xu wrote:
>
>
>> Added: cfe/trunk/test/Analysis/rdar-6541136.c
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/rdar-6541136.c?rev=63347&view=auto
>>
>>
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/rdar-6541136.c (added)
>> +++ cfe/trunk/test/Analysis/rdar-6541136.c Thu Jan 29 18:08:43 2009
>> @@ -0,0 +1,20 @@
>> +// clang -verify -analyze -checker-cfref -analyzer-store-basic %s
>> +
>> +struct tea_cheese { unsigned magic; };
>> +typedef struct tea_cheese kernel_tea_cheese_t;
>> +extern kernel_tea_cheese_t _wonky_gesticulate_cheese;
>> +
>> +// This test case exercises the ElementRegion::getRValueType() logic.
>> +// All it tests is that it does not crash or do anything weird.
>> +// The out-of-bounds-access on line 19 is caught using the region store
>> variant.
>> +
>> +void foo( void )
>> +{
>> +  kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese;
>> +  struct load_wine *cmd = (void*) &wonky[1];
>
>
> This is like an ill use of the type system. What do you think we emit a
> warning instead of following it.
> That is, we emit a warning when the array region does not have array type
> or pointer type.
>
>
> Yes this is really gross code.    It actually contains more stuff than the
> original test case I looked at just to see if the analyzer would crash.  The
> original test case was basically:
>
>   kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese;
>   struct load_wine *cmd = (void*) &wonky[1];
>
> Let's just consider this for a moment.  While the out-of-bounds access
> looks wrong in all cases, aside from the out-of-bounds access it may be
> "okay."  Consider the following possible definition of struct load_wine:
>
> struct load_wine {
>   struct tea_cheese x;
>   ...
> };
>
> In this case, it is okay to cast a 'struct tea_cheese*' to a 'struct
> load_wine*'.  This kind of pattern appears a lot in the Gtk+ code (and other
> C libraries) to simulate object-oriented programming and inheritance.
>
> The problem here is that we don't have the definition of 'struct
> load_wine'.  If we did, we could do some extra type checking (as you
> suggest).  In this case, we don't, so what do we do?
>
> As for:
>
>   char *p = (void*) &wonky[1];
>   *p = 1;
>
> Yes that is really gross.  What should we do here?  This is technically an
> out-of-bounds memory write (which we don't flag because of the abuse of the
> type system), but as you said it is an abuse of the type system.  With our
> path-sensitive type information in the RegionStore objects perhaps we can
> flag a more lucid warning?  On the other hand, assuming that &wonky[1] is
> valid (say wonky was an array), we're just writing '1' to the first byte of
> the 'magic' field.  The rabbit hole goes arbitrary deep.
>

As the rabbit hole goes arbitrary deep here, I suggest we abandon early when
we getLValueElement for wonky[1]. For this example, I think the region of
_wonky_gesticulate_cheese should be the brother but not the parent of the
region for wonky[1]. But when we getLValueElement for wonky[1], we actually
use the region of _wonky_gesticulate_cheese as the super region for the
element region.

Then it goes to my original argument for guaranteeing all base regions
passed to getLValueElement() to be ElementRegions, since we do not have
enough information in getLValueElement() for us to make the right decision.
For code

char * p = (char*) alloc(4);
char c = p[1];

we make p points to an element region when we do the cast. It saves much
trouble later when p is used as array base.

If we do not make that ElementRegion when doing cast, it is hard to
distinguish the case for code:

struct s* p = (struct s*) alloc(sizeof struct s);
struct s v = p[1];

For this case the region for p should not be used as the base region for
element region.

My point is that we make an ElementRegion in the first case and an
AnonTypedRegion for the second case, and we can only do the right thing when
doing cast, but not later when we getLValueElement for p[1].

Then when we getLValueElement, we abandon (emit warning or retun unknown)
when the base region is not an ElementRegion.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090130/a338c5a8/attachment.html>