StaticAnalyzer CastToStruct : No memory corruption when casting array to struct

[danielmarjamaki]

Summary

There is no memory corruption when array is casted to struct (if array is large enough):

int Buf[100];
struct S * P = (struct S *)Buf;

This patch will fix false positives for that. However it does not check if the buffer is large enough. So this patch could cause false negatives.

[NoQ]

Adding cfe-commits as per developer policy.

Yeah, it doesn't probably cause the same kind of memory corruption, however i wouldn't call this code safe: it still violates the strict aliasing rule, unless the array is of chars. I think this warning is worth keeping for non-char arrays. Even with -fno-strict-aliasing of some sort, the programmer might run into endianness or alignment issues.

[danielmarjamaki]

If the -fno-strict-aliasing would fix this warning then it would be OK.

If you are telling me that this CastToStruct check is about alignment or endianness then I think the message is highly misleading. We should rewrite the message.

In general, using char instead of short/int does not prevent alignment/endianness problems as far as I see. You can still have just as many such problems even though array is char.

I am not sure why they did not use char here. But on their platform, sizeof(char)==sizeof(short)==sizeof(int)==1. It does not matter much if a typedef uses char or int.

[danielmarjamaki]

hmm.. I don't actually care much about this specific check at the moment.

I saw other false positives (unreachable code) and thought that this check made the analyzer think there was corrupted memory.

Now I can't reproduce my other false positives.

I'll close this for now. Unless I get those other FP again I don't care about this.

[NoQ]

Random thoughts:

• This checker doesn't alter the exploded graph, so it cannot be causing or suppressing positives in other checkers.

• We should not be adding platform-specific behavior (eg. working as if sizeof(int) == 1) without actually ensuring that it is so on that platform.

• As far as i understand, even if sizeof(int) == 1, casting an int * into a structure pointer violates the strict aliasing rule. Simply because int and char are different types, even if they have same size and signedness (note that also char, signed char, unsigned char are three different types, despite two of them refer to the same thing).

In general, using char instead of short/int does not prevent alignment/endianness problems as far as I see.

You're right on this one, it'd only prevent endianness of the buffer ints from causing problems.

lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp

void CastToStructChecker::checkPreStmt(const CastExpr *CE,
   QualType ToTy = Ctx.getCanonicalType(CE->getType());
 
   const PointerType *OrigPTy = dyn_cast<PointerType>(OrigTy.getTypePtr());
   const PointerType *ToPTy = dyn_cast<PointerType>(ToTy.getTypePtr());
 
   if (!ToPTy || !OrigPTy)
     return;
 
+  if (E->IgnoreParenCasts()->getType()->isArrayType())
+    return;
+
   QualType OrigPointeeTy = OrigPTy->getPointeeType();
   QualType ToPointeeTy = ToPTy->getPointeeType();
 
   if (!ToPointeeTy->isStructureOrClassType())
     return;
 
   // We allow cast from void*.
   if (OrigPointeeTy->isVoidType())

test/Analysis/array-struct.c

void f18() {
   char *p = (char *) __builtin_alloca(10);
   read(p);
   q = p;
   q++;
   if (*q) { // no-warning
   }
 }
 
+void f19() {
+  int data[10];
+  struct s3 *a = (struct s3 *)data;
+}
 
 // [PR13927] offsetof replacement macro flagged as "dereference of a null pointer"
 int offset_of_data_array(void)
 {
   return ((char *)&(((struct s*)0)->data_array)) - ((char *)0); // no-warning
 }
 
 int testPointerArithmeticOnVoid(void *bytes) {

test/Analysis/misc-ps-region-store.m

 struct ArrayWrapper { unsigned char y[16]; };
 struct WrappedStruct { unsigned z; };
 
 void test_handle_array_wrapper_helper();
 
 int test_handle_array_wrapper() {
   struct ArrayWrapper x;
   test_handle_array_wrapper_helper(&x);
-  struct WrappedStruct *p = (struct WrappedStruct*) x.y; // expected-warning{{Casting a non-structure type to a structure type and accessing a field can lead to memory access errors or data corruption}}
+  struct WrappedStruct *p = (struct WrappedStruct*) x.y;
   return p->z;  // no-warning
 }
 
 //===----------------------------------------------------------------------===//
 // <rdar://problem/7261075> [RegionStore] crash when 
 //   handling load: '*((unsigned int *)"????")'
 //===----------------------------------------------------------------------===//