[clang][CodeGen][UBSan] VLA size checking for unsigned integer parameter

[AdamMagierFOSS]

Summary

The code generation for the UBSan VLA size check was qualified by a con-
dition that the parameter must be a signed integer, however the C spec
does not make any distinction that only signed integer parameters can be
used to declare a VLA, only qualifying that it must be greater than zero
if it is not a constant.

[rjmccall]

Thanks, patch basically looks good. Minor requests and an observation for possible follow-up work.

clang/lib/CodeGen/CodeGenFunction.cpp

2247

Please continue to use lowercase names for local variables for consistency with the surrounding code. The rename to sizeExpr is good; you can then rename Size to size or sizeValue.

2259

Sema requires the bound expression to have integral type, so you don't need to do this.

2275

This would be a different bug, but should UBSan also be doing a bounds check if the type is larger than size_t?

[AdamMagierFOSS]

Thank you for the feedback - I've added responses inline and I'll update the change to reflect the feedback.

clang/lib/CodeGen/CodeGenFunction.cpp

2247

Thank you for clarifying. I was unsure which style to stick to since I saw both "new style" code and "old style" code within the same scope, so I went with the "new style" to align with recent submissions to the clang project I looked at. It's no problem at all for me to switch my changes to the "old style" if that's preferred in this instance.

2259

I suspected this would be unnecessary - thank you for confirming, I'll remove this.

2275

Interesting point, I'd have to reread through the spec to give a precise/definitive answer. To keep this review focused I'll table the discussion for a separate forum.

[AdamMagierFOSS]

Updating based on feedback from rjmccall.

[rjmccall]

Okay, very minor requests, but otherwise LGTM; feel free to commit with these changes.

clang/lib/CodeGen/CodeGenFunction.cpp

2251

You can sink this into the if block.

2275

I'm pretty sure you should, but it's fine to do it in a different patch. Please leave a FIXME about it, though.

[AdamMagierFOSS]

Thanks once again for the feedback - I'll make the changes and commit directly.

clang/lib/CodeGen/CodeGenFunction.cpp

2251

Sure thing.

2275

My gut says the check should be performed as you say, but I can't prove it to myself in a satisfactory manner at the moment. I'll leave a FIXME for this as you requested.

[alexfh]

We've started seeing some tests dying with SIGILL (illegal instruction) after this patch. I'm working on a reduced repro, but please take a look, if you can find any issues yourself.

[alexfh]

In D116048#3261573, @alexfh wrote:

We've started seeing some tests dying with SIGILL (illegal instruction) after this patch. I'm working on a reduced repro, but please take a look, if you can find any issues yourself.

False alarm. All of these turned out to be real issues detected by the updated clang.

clang/lib/CodeGen/CodeGenFunction.cpp

case Type::IncompleteArray:
       break;
 
     case Type::VariableArray: {
       // Losing element qualification here is fine.
       const VariableArrayType *vat = cast<VariableArrayType>(ty);
 
       // Unknown size indication requires no size computation.
       // Otherwise, evaluate and record it.
-      if (const Expr *size = vat->getSizeExpr()) {
+      if (const Expr *SizeExpr = vat->getSizeExpr()) {
         // It's possible that we might have emitted this already,
         // e.g. with a typedef and a pointer to it.
-        llvm::Value *&entry = VLASizeMap[size];
-        if (!entry) {
-          llvm::Value *Size = EmitScalarExpr(size);
+        llvm::Value *&MapEntry = VLASizeMap[SizeExpr];
+        if (!MapEntry) {
+          llvm::Value *Size = EmitScalarExpr(SizeExpr);
+          clang::QualType SEType = SizeExpr->getType();
 
           // C11 6.7.6.2p5:
           //   If the size is an expression that is not an integer constant
           //   expression [...] each time it is evaluated it shall have a value
           //   greater than zero.
-          if (SanOpts.has(SanitizerKind::VLABound) &&
-              size->getType()->isSignedIntegerType()) {
+          if (SanOpts.has(SanitizerKind::VLABound) && SEType->isIntegerType()) {
             SanitizerScope SanScope(this);
             llvm::Value *Zero = llvm::Constant::getNullValue(Size->getType());
+            llvm::Value *CheckCondition =
+                SEType->isSignedIntegerType()
+                    ? Builder.CreateICmpSGT(Size, Zero)
+                    : Builder.CreateICmpUGT(Size, Zero);
             llvm::Constant *StaticArgs[] = {
-                EmitCheckSourceLocation(size->getBeginLoc()),
-                EmitCheckTypeDescriptor(size->getType())};
-            EmitCheck(std::make_pair(Builder.CreateICmpSGT(Size, Zero),
-                                     SanitizerKind::VLABound),
+                EmitCheckSourceLocation(SizeExpr->getBeginLoc()),
+                EmitCheckTypeDescriptor(SEType)};
+            EmitCheck(std::make_pair(CheckCondition, SanitizerKind::VLABound),
                       SanitizerHandler::VLABoundNotPositive, StaticArgs, Size);
           }
 
           // Always zexting here would be wrong if it weren't
           // undefined behavior to have a negative bound.
-          entry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
+          MapEntry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
         }
       }
       type = vat->getElementType();
       break;
     }
 
     case Type::FunctionProto:
     case Type::FunctionNoProto:

clang/test/CodeGen/catch-undef-behavior.c

 // CHECK-UBSAN: @[[LINE_500:.*]] = {{.*}}, i32 500, i32 10 {{.*}} @{{.*}}, i8 2, i8 0 }
 // CHECK-UBSAN: @[[LINE_600:.*]] = {{.*}}, i32 600, i32 3 {{.*}} @{{.*}}, i8 2, i8 1 }
 
 // CHECK-UBSAN: @[[STRUCT_S:.*]] = private unnamed_addr constant { i16, i16, [11 x i8] } { i16 -1, i16 0, [11 x i8] c"'struct S'\00" }
 
 // CHECK-UBSAN: @[[LINE_700:.*]] = {{.*}}, i32 700, i32 14 {{.*}} @[[STRUCT_S]], i8 2, i8 3 }
 // CHECK-UBSAN: @[[LINE_800:.*]] = {{.*}}, i32 800, i32 12 {{.*}} @{{.*}} }
 // CHECK-UBSAN: @[[LINE_900:.*]] = {{.*}}, i32 900, i32 11 {{.*}} @{{.*}} }
+// CHECK-UBSAN: @[[LINE_1000:.*]] = {{.*}}, i32 1000, i32 11 {{.*}} @{{.*}} }
 // CHECK-UBSAN: @[[FP16:.*]] = private unnamed_addr constant { i16, i16, [9 x i8] } { i16 1, i16 16, [9 x i8] c"'__fp16'\00" }
 // CHECK-UBSAN: @[[LINE_1200:.*]] = {{.*}}, i32 1200, i32 10 {{.*}} @{{.*}} }
 // CHECK-UBSAN: @[[LINE_1300:.*]] = {{.*}}, i32 1300, i32 10 {{.*}} @{{.*}} }
 // CHECK-UBSAN: @[[LINE_1400:.*]] = {{.*}}, i32 1400, i32 10 {{.*}} @{{.*}} }
 // Make sure we check the fp16 type_mismatch data so we can easily match the signed char float_cast_overflow
 // CHECK-UBSAN: @[[LINE_1500:.*]] = {{.*}}, i32 1500, i32 10 {{.*}} @[[FP16]], {{.*}} }
 // CHECK-UBSAN: @[[SCHAR:.*]] = private unnamed_addr constant { i16, i16, [14 x i8] } { i16 0, i16 7, [14 x i8] c"'signed char'\00" }
 // CHECK-UBSAN: @[[LINE_1500:.*]] = {{.*}}, i32 1500, i32 10 {{.*}} @[[FP16]], {{.*}} }
void vla_bound(int n) {
   // CHECK-UBSAN:      icmp sgt i32 %[[PARAM:.*]], 0
   //
   // CHECK-UBSAN:      %[[ARG:.*]] = zext i32 %[[PARAM]] to i64
   // CHECK-UBSAN-NEXT: call void @__ubsan_handle_vla_bound_not_positive(i8* bitcast ({{.*}} @[[LINE_900]] to i8*), i64 %[[ARG]])
 #line 900
   int arr[n * 3];
 }
 
+// CHECK-UBSAN-LABEL: @vla_bound_unsigned
+void vla_bound_unsigned(unsigned int n) {
+  // CHECK-UBSAN:      icmp ugt i32 %[[PARAM:.*]], 0
+  //
+  // CHECK-UBSAN:      %[[ARG:.*]] = zext i32 %[[PARAM]] to i64
+  // CHECK-UBSAN-NEXT: call void @__ubsan_handle_vla_bound_not_positive(i8* bitcast ({{.*}} @[[LINE_1000]] to i8*), i64 %[[ARG]])
+#line 1000
+  int arr[n * 3];
+}
+
 // CHECK-UBSAN-LABEL: @int_float_no_overflow
 float int_float_no_overflow(__int128 n) {
   // CHECK-UBSAN-NOT: call void @__ubsan_handle
   return n;
 }
 
 // CHECK-COMMON-LABEL: @int_float_overflow
 float int_float_overflow(unsigned __int128 n) {