[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

[Ted Kremenek]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090129/799756d8/attachment.html>