[PATCH] Teach CastSizeChecker about flexible arrays

[Daniel Fahlgren]

Hi Jordan,

On Thu, 2014-02-13 at 09:18 -0800, Jordan Rose wrote:
> Nice to see this improvement! Some comments, but mostly just style
> things:

> +/// Check if we are casting to a struct with a flexible array at the
> end.
> +///
> +/// struct foo {
> +///   size_t len;
> +///   struct bar data[];
> +/// };
> +///
> +/// or
> +///
> +/// struct foo {
> +///   size_t len;
> +///   struct bar data[0];
> +/// }
> +///
> +/// In these cases it is also valid to allocate size of struct foo +
> a multiple
> +/// of struct bar.
> 
> Since these are Doxygen comments, please surround code blocks with
> \code...\endcode.


> +static bool evenFlexibleArraySize(ASTContext &Ctx, CharUnits
> regionSize,
> +                                  CharUnits typeSize, QualType
> ToPointeeTy) {

> LLVM naming conventions require all variables and parameters to start
> with a capital letter. (This occurs throughout the function.)

Should I fix the rest of the file as well or should that be done in a
separate commit (if it should be done at all)?


> +  const RecordType *RT =
> ToPointeeTy.getTypePtr()->getAs<RecordType>();

> QualType has an overloaded operator->, so you can just say
> "ToPointeeTy->getAs<RecordType>()".


> +  FieldDecl *last = 0;

> Within the analyzer, we try to make all AST class pointers const. (I
> actually try to do that everywhere I can. Const-correctness is a nice
> thing.)

> 
> +  for (; iter != end; ++iter)
> +    last = *iter;

> What if the struct is empty?

Then the size of the struct will be 0 or 1, depending on C or C++. Types
with these sizes will be handled in checkPreStmt and never reach
evenFlexibleArraySize. But I agree it is far from obvious and have added
a check to make sure last has been set.
> 
> 
> +  const Type *elemType =
> last->getType()->getArrayElementTypeNoTypeQual();

> It looks like you could move this (and the flexSize calculation) to
> after you've already checked that this record has a flexible or
> zero-length array.

With the added support for last element of size 1 it needs to stay where
it is.
> 
> 
> +  if (const ConstantArrayType *ArrayTy =
> +      Ctx.getAsConstantArrayType(last->getType())) {

> I don't think there's a rule about this, but I think a wrapped
> initialization or assignment should have the next line indented.
> "Whatever clang-format does" is also almost always an acceptable
> answer.
> 
> 

> +    if (ArrayTy->getSize() != 0)

> People sometimes write this pattern with a last element of 1 instead
> of 0. Since the analyzer tries to avoid false positives, we should
> probably detect that as a similar pattern and perform the same check.
> 

I have added support for that as well. However, that forced me to keep
the flexSize calculation at its original place.

> 
> +      BT.reset(new BuiltinBug("Cast region with wrong size.",
> +                          "Cast a region whose size is not a multiple
> of the"
> +                          " destination type size."));
> 
> 
> While you're here, please re-align the string literals so all the
> arguments to BuiltinBug() line up. (Also, after Alex's recent patch
> you'll need to pass the current checker to BuiltinBug as well.
> 
> 
> 
> 
> Index: test/Analysis/malloc-annotations.c
> ===================================================================
> --- test/Analysis/malloc-annotations.c (revision 201043)
> +++ test/Analysis/malloc-annotations.c (working copy)
> 
> 
> Don't worry about adding test cases to malloc-annotations.c. That's
> only supposed to test custom malloc and free wrappers. On the other
> hand, please include some test cases with flexible-array structs where
> the allocation size is too small to begin with, and some where the
> allocation size has zero additional capacity over the sizeof of the
> struct.

Ok, added.
> 
> Thanks for working on this!
> Jordan
> 
Thanks for your feedback, attached is a new version of the patch.

Cheers,
Daniel Fahlgren
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CastSizeChecker.patch
Type: text/x-patch
Size: 8921 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140214/612038fd/attachment.bin>