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

[Adam Magier via Phabricator via cfe-commits]

AdamMagierFOSS added a comment.

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




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2247
   // 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,

rjmccall wrote:
> 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`.
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.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2259
   //   greater than zero.
-  if (SanOpts.has(SanitizerKind::VLABound) &&
-  size->getType()->isSignedIntegerType()) {
+  if (SanOpts.has(SanitizerKind::VLABound) && SEType->isIntegerType()) 
{
 SanitizerScope SanScope(this);

rjmccall wrote:
> Sema requires the bound expression to have integral type, so you don't need 
> to do this.
I suspected this would be unnecessary - thank you for confirming, I'll remove 
this.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2275
   // undefined behavior to have a negative bound.
-  entry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
+  MapEntry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
 }

rjmccall wrote:
> This would be a different bug, but should UBSan also be doing a bounds check 
> if the type is larger than `size_t`?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116048/new/

https://reviews.llvm.org/D116048

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adam Magier via Phabricator via cfe-commits]

AdamMagierFOSS updated this revision to Diff 399057.
AdamMagierFOSS added a comment.

Updating based on feedback from rjmccall.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116048/new/

https://reviews.llvm.org/D116048

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/catch-undef-behavior.c


Index: clang/test/CodeGen/catch-undef-behavior.c
===
--- clang/test/CodeGen/catch-undef-behavior.c
+++ clang/test/CodeGen/catch-undef-behavior.c
@@ -17,6 +17,7 @@
 // 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 {{.*}} @{{.*}} }
@@ -186,6 +187,16 @@
   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
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2247,32 +2247,35 @@
 
   // 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];
+llvm::Value *&entry = VLASizeMap[sizeExpr];
 if (!entry) {
-  llvm::Value *Size = EmitScalarExpr(size);
+  llvm::Value *size = EmitScalarExpr(sizeExpr);
+  clang::QualType sizeExprType = 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)) {
 SanitizerScope SanScope(this);
-llvm::Value *Zero = llvm::Constant::getNullValue(Size->getType());
+llvm::Value *Zero = llvm::Constant::getNullValue(size->getType());
+llvm::Value *CheckCondition =
+sizeExprType->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),
-  SanitizerHandler::VLABoundNotPositive, StaticArgs, Size);
+EmitCheckSourceLocation(sizeExpr->getBeginLoc()),
+EmitCheckTypeDescriptor(sizeExprType)};
+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);
+  entry = Builder.CreateIntCast(size, SizeTy, /*signed*/ false);
 }
   }
   type = vat->getElementType();


Index: clang/test/CodeGen/catch-undef-behavior.c
===
--- clang/test/CodeGen/catch-undef-behavior.c
+++ clang/test/CodeGen/catch-undef-behavior.c
@@ -17,6 +17,7 @@
 // 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:.*]] = pri

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

[Adam Magier via Phabricator via cfe-commits]

AdamMagierFOSS added a comment.

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




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2256
+  llvm::Value *size = EmitScalarExpr(sizeExpr);
+  clang::QualType sizeExprType = sizeExpr->getType();
 

rjmccall wrote:
> You can sink this into the `if` block.
Sure thing.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2275
   // undefined behavior to have a negative bound.
-  entry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
+  MapEntry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
 }

rjmccall wrote:
> AdamMagierFOSS wrote:
> > rjmccall wrote:
> > > This would be a different bug, but should UBSan also be doing a bounds 
> > > check if the type is larger than `size_t`?
> > 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.
> I'm pretty sure you should, but it's fine to do it in a different patch.  
> Please leave a FIXME about it, though.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116048/new/

https://reviews.llvm.org/D116048

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adam Magier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb2715660ed0f: [clang][CodeGen][UBSan] VLA size checking for 
unsigned integer parameter (authored by AdamMagierFOSS).

Changed prior to commit:
  https://reviews.llvm.org/D116048?vs=399057&id=399114#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116048/new/

https://reviews.llvm.org/D116048

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/catch-undef-behavior.c


Index: clang/test/CodeGen/catch-undef-behavior.c
===
--- clang/test/CodeGen/catch-undef-behavior.c
+++ clang/test/CodeGen/catch-undef-behavior.c
@@ -17,6 +17,7 @@
 // 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 {{.*}} @{{.*}} }
@@ -186,6 +187,16 @@
   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
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2247,32 +2247,36 @@
 
   // 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];
+llvm::Value *&entry = VLASizeMap[sizeExpr];
 if (!entry) {
-  llvm::Value *Size = EmitScalarExpr(size);
+  llvm::Value *size = EmitScalarExpr(sizeExpr);
 
   // 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)) {
 SanitizerScope SanScope(this);
-llvm::Value *Zero = llvm::Constant::getNullValue(Size->getType());
+llvm::Value *Zero = llvm::Constant::getNullValue(size->getType());
+clang::QualType SEType = sizeExpr->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),
-  SanitizerHandler::VLABoundNotPositive, StaticArgs, Size);
+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);
+  // FIXME: What about when size's type is larger than size_t?
+  entry = Builder.CreateIntCast(size, SizeTy, /*signed*/ false);
 }
   }
   type = vat->getElementType();


Index: clang/test/CodeGen/catch-undef-behavior.c
===
--- clang/test/CodeGen/catch-undef-behavior.c
+++ clang/test/CodeGen/catch-undef-behavior.c
@@ -17,6 +17,7 @@
 // CHECK-UBSAN: @[[LINE_700:.*]] = {{.*}}, i32 700, i32 14 {{.*}} @[[STRUCT_S]], i8 2, i8 3 }
 // CH

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

[Adam Magier via Phabricator via cfe-commits]

AdamMagierFOSS created this revision.
AdamMagierFOSS added reviewers: rjmccall, rsmith.
AdamMagierFOSS requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116048

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/catch-undef-behavior.c


Index: clang/test/CodeGen/catch-undef-behavior.c
===
--- clang/test/CodeGen/catch-undef-behavior.c
+++ clang/test/CodeGen/catch-undef-behavior.c
@@ -17,6 +17,7 @@
 // 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 {{.*}} @{{.*}} }
@@ -186,6 +187,16 @@
   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
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2244,32 +2244,35 @@
 
   // 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();


Index: clang/test/CodeGen/catch-undef-behavior.c
===
--- clang/test/CodeGen/catch-undef-behavior.c
+++ clang/test/CodeGen/catch-undef-behavior.c
@@ -17,6 +17,7 @@
 // CHECK-UBSAN: @[[LINE_700:.*]] = {{.*}}, i32 700, i32 14 {{.*}} @[[STRUCT_S]], i8 2, i8 3 }
 // CHECK-UBSAN: @