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
Index: lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp (revision 201356)
+++ lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp (working copy)
@@ -29,6 +29,66 @@
};
}
+/// Check if we are casting to a struct with a flexible array at the end.
+/// \code
+/// struct foo {
+/// size_t len;
+/// struct bar data[];
+/// };
+/// \endcode
+/// or
+/// \code
+/// struct foo {
+/// size_t len;
+/// struct bar data[0];
+/// }
+/// \endcode
+/// In these cases it is also valid to allocate size of struct foo + a multiple
+/// of struct bar.
+static bool evenFlexibleArraySize(ASTContext &Ctx, CharUnits RegionSize,
+ CharUnits TypeSize, QualType ToPointeeTy) {
+ const RecordType *RT = ToPointeeTy->getAs<RecordType>();
+ if (!RT)
+ return false;
+
+ const RecordDecl *RD = RT->getDecl();
+ RecordDecl::field_iterator Iter(RD->field_begin());
+ RecordDecl::field_iterator End(RD->field_end());
+ const FieldDecl *Last = 0;
+ for (; Iter != End; ++Iter)
+ Last = *Iter;
+
+ if (!Last)
+ return false;
+
+ const Type *ElemType = Last->getType()->getArrayElementTypeNoTypeQual();
+ CharUnits FlexSize;
+ if (const ConstantArrayType *ArrayTy =
+ Ctx.getAsConstantArrayType(Last->getType())) {
+ FlexSize = Ctx.getTypeSizeInChars(ElemType);
+ if (ArrayTy->getSize() == 1 && TypeSize > FlexSize)
+ TypeSize -= FlexSize;
+ else if (ArrayTy->getSize() != 0)
+ return false;
+ } else if (RD->hasFlexibleArrayMember()) {
+ FlexSize = Ctx.getTypeSizeInChars(ElemType);
+ } else {
+ return false;
+ }
+
+ if (FlexSize.isZero())
+ return false;
+
+ CharUnits Left = RegionSize - TypeSize;
+ if (Left.isNegative())
+ return false;
+
+ if (Left % FlexSize == 0)
+ return true;
+
+ return false;
+}
+
void CastSizeChecker::checkPreStmt(const CastExpr *CE,CheckerContext &C) const {
const Expr *E = CE->getSubExpr();
ASTContext &Ctx = C.getASTContext();
@@ -66,18 +126,21 @@
if (typeSize.isZero())
return;
- if (regionSize % typeSize != 0) {
- if (ExplodedNode *errorNode = C.generateSink()) {
- if (!BT)
- BT.reset(
- new BuiltinBug(this, "Cast region with wrong size.",
- "Cast a region whose size is not a multiple of the"
- " destination type size."));
- BugReport *R = new BugReport(*BT, BT->getDescription(),
- errorNode);
- R->addRange(CE->getSourceRange());
- C.emitReport(R);
- }
+ if (regionSize % typeSize == 0)
+ return;
+
+ if (evenFlexibleArraySize(Ctx, regionSize, typeSize, ToPointeeTy))
+ return;
+
+ if (ExplodedNode *errorNode = C.generateSink()) {
+ if (!BT)
+ BT.reset(new BuiltinBug(this, "Cast region with wrong size.",
+ "Cast a region whose size is not a multiple"
+ " of the destination type size."));
+ BugReport *R = new BugReport(*BT, BT->getDescription(),
+ errorNode);
+ R->addRange(CE->getSourceRange());
+ C.emitReport(R);
}
}
Index: test/Analysis/no-outofbounds.c
===================================================================
--- test/Analysis/no-outofbounds.c (revision 201356)
+++ test/Analysis/no-outofbounds.c (working copy)
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core,alpha.unix,alpha.security.ArrayBound -analyzer-store=region -verify %s
+// expected-no-diagnostics
//===----------------------------------------------------------------------===//
// This file tests cases where we should not flag out-of-bounds warnings.
@@ -24,8 +25,7 @@
void field() {
struct vec { size_t len; int data[0]; };
- // FIXME: Not warn for this.
- struct vec *a = malloc(sizeof(struct vec) + 10); // expected-warning {{Cast a region whose size is not a multiple of the destination type size}}
+ struct vec *a = malloc(sizeof(struct vec) + 10*sizeof(int));
a->len = 10;
a->data[1] = 5; // no-warning
free(a);
Index: test/Analysis/malloc.c
===================================================================
--- test/Analysis/malloc.c (revision 201356)
+++ test/Analysis/malloc.c (working copy)
@@ -270,6 +270,222 @@
buf[1] = 'c'; // not crash
}
+void cast_emtpy_struct() {
+ struct st {
+ };
+
+ struct st *s = malloc(sizeof(struct st)); // no-warning
+ free(s);
+}
+
+void cast_struct_1() {
+ struct st {
+ int i[100];
+ char j[];
+ };
+
+ struct st *s = malloc(sizeof(struct st)); // no-warning
+ free(s);
+}
+
+void cast_struct_2() {
+ struct st {
+ int i[100];
+ char j[0];
+ };
+
+ struct st *s = malloc(sizeof(struct st)); // no-warning
+ free(s);
+}
+
+void cast_struct_3() {
+ struct st {
+ int i[100];
+ char j[1];
+ };
+
+ struct st *s = malloc(sizeof(struct st)); // no-warning
+ free(s);
+}
+
+void cast_struct_4() {
+ struct st {
+ int i[100];
+ char j[2];
+ };
+
+ struct st *s = malloc(sizeof(struct st)); // no-warning
+ free(s);
+}
+
+void cast_struct_5() {
+ struct st {
+ char i[200];
+ char j[1];
+ };
+
+ struct st *s = malloc(sizeof(struct st) - sizeof(char)); // no-warning
+ free(s);
+}
+
+void cast_struct_warn_1() {
+ struct st {
+ int i[100];
+ char j[2];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 2); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
+void cast_struct_warn_2() {
+ struct st {
+ int i[100];
+ char j[2];
+ };
+
+ struct st *s = malloc(2); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
+void cast_struct_flex_array_1() {
+ struct st {
+ int i[100];
+ char j[];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3); // no-warning
+ free(s);
+}
+
+void cast_struct_flex_array_2() {
+ struct st {
+ int i[100];
+ char j[0];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3); // no-warning
+ free(s);
+}
+
+void cast_struct_flex_array_3() {
+ struct st {
+ int i[100];
+ char j[1];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3); // no-warning
+ free(s);
+}
+
+void cast_struct_flex_array_4() {
+ struct foo {
+ char f[32];
+ };
+ struct st {
+ char i[100];
+ struct foo data[];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3 * sizeof(struct foo)); // no-warning
+ free(s);
+}
+
+void cast_struct_flex_array_5() {
+ struct foo {
+ char f[32];
+ };
+ struct st {
+ char i[100];
+ struct foo data[0];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3 * sizeof(struct foo)); // no-warning
+ free(s);
+}
+
+void cast_struct_flex_array_6() {
+ struct foo {
+ char f[32];
+ };
+ struct st {
+ char i[100];
+ struct foo data[1];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3 * sizeof(struct foo)); // no-warning
+ free(s);
+}
+
+void cast_struct_flex_array_warn_1() {
+ struct foo {
+ char f[32];
+ };
+ struct st {
+ char i[100];
+ struct foo data[];
+ };
+
+ struct st *s = malloc(3 * sizeof(struct st) + 3 * sizeof(struct foo)); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
+void cast_struct_flex_array_warn_2() {
+ struct foo {
+ char f[32];
+ };
+ struct st {
+ char i[100];
+ struct foo data[0];
+ };
+
+ struct st *s = malloc(3 * sizeof(struct st) + 3 * sizeof(struct foo)); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
+void cast_struct_flex_array_warn_3() {
+ struct foo {
+ char f[32];
+ };
+ struct st {
+ char i[100];
+ struct foo data[1];
+ };
+
+ struct st *s = malloc(3 * sizeof(struct st) + 3 * sizeof(struct foo)); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
+void cast_struct_flex_array_warn_4() {
+ struct st {
+ int i[100];
+ int j[];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
+void cast_struct_flex_array_warn_5() {
+ struct st {
+ int i[100];
+ int j[0];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
+void cast_struct_flex_array_warn_6() {
+ struct st {
+ int i[100];
+ int j[1];
+ };
+
+ struct st *s = malloc(sizeof(struct st) + 3); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}}
+ free(s);
+}
+
void mallocCastToVoid() {
void *p = malloc(2);
const void *cp = p; // not crash
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
