[PATCH] Teach CastSizeChecker about flexible arrays

Jordan Rose jordan_rose at apple.com
Thu Feb 13 09:18:27 PST 2014


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.)


+  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?


+  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.


+  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.


+      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.

Thanks for working on this!
Jordan


On Feb 10, 2014, at 11:45 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi,
> 
> I got a false positive at work and this patch fixes it. CastSizeChecker
> will now handle structs ending with a flexible array. It will no longer
> warn when you allocate sizeof the struct + a multiple of the array's
> element size.
> 
> Best regards,
> Daniel Fahlgren
> _______________________________________________
> 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/20140213/7b8353d8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CastSizeChecker.patch
Type: text/x-patch
Size: 9469 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140213/7b8353d8/attachment.bin>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140213/7b8353d8/attachment-0001.html>


More information about the cfe-commits mailing list