From: Vedant Kumar <vsk@apple.com>
Date: Fri, 23 Jun 2017 21:32:38 +0000 (+0000)
Subject: [ubsan] Improve diagnostics for return value checks (clang)
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9766b307603b0617281b4ccc579112efb6bf56b9;p=clang
[ubsan] Improve diagnostics for return value checks (clang)
This patch makes ubsan's nonnull return value diagnostics more precise,
which makes the diagnostics more useful when there are multiple return
statements in a function. Example:
1 |__attribute__((returns_nonnull)) char *foo() {
2 | if (...) {
3 | return expr_which_might_evaluate_to_null();
4 | } else {
5 | return another_expr_which_might_evaluate_to_null();
6 | }
7 |} // <- The current diagnostic always points here!
runtime error: Null returned from Line 7, Column 2!
With this patch, the diagnostic would point to either Line 3, Column 5
or Line 5, Column 5.
This is done by emitting source location metadata for each return
statement in a sanitized function. The runtime is passed a pointer to
the appropriate metadata so that it can prepare and deduplicate reports.
Compiler-rt patch (with more tests): https://reviews.llvm.org/D34298
Differential Revision: https://reviews.llvm.org/D34299
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@306163 91177308-0d34-0410-b5e6-96231b3b80d8
---
diff --git a/lib/CodeGen/CGCall.cpp b/lib/CodeGen/CGCall.cpp
index 0e5ccc0d9f..38d520a2be 100644
--- a/lib/CodeGen/CGCall.cpp
+++ b/lib/CodeGen/CGCall.cpp
@@ -2906,7 +2906,7 @@ void CodeGenFunction::EmitFunctionEpilog(const CGFunctionInfo &FI,
llvm::Instruction *Ret;
if (RV) {
- EmitReturnValueCheck(RV, EndLoc);
+ EmitReturnValueCheck(RV);
Ret = Builder.CreateRet(RV);
} else {
Ret = Builder.CreateRetVoid();
@@ -2916,8 +2916,7 @@ void CodeGenFunction::EmitFunctionEpilog(const CGFunctionInfo &FI,
Ret->setDebugLoc(std::move(RetDbgLoc));
}
-void CodeGenFunction::EmitReturnValueCheck(llvm::Value *RV,
- SourceLocation EndLoc) {
+void CodeGenFunction::EmitReturnValueCheck(llvm::Value *RV) {
// A current decl may not be available when emitting vtable thunks.
if (!CurCodeDecl)
return;
@@ -2950,27 +2949,30 @@ void CodeGenFunction::EmitReturnValueCheck(llvm::Value *RV,
SanitizerScope SanScope(this);
- llvm::BasicBlock *Check = nullptr;
- llvm::BasicBlock *NoCheck = nullptr;
- if (requiresReturnValueNullabilityCheck()) {
- // Before doing the nullability check, make sure that the preconditions for
- // the check are met.
- Check = createBasicBlock("nullcheck");
- NoCheck = createBasicBlock("no.nullcheck");
- Builder.CreateCondBr(RetValNullabilityPrecondition, Check, NoCheck);
- EmitBlock(Check);
- }
+ // Make sure the "return" source location is valid. If we're checking a
+ // nullability annotation, make sure the preconditions for the check are met.
+ llvm::BasicBlock *Check = createBasicBlock("nullcheck");
+ llvm::BasicBlock *NoCheck = createBasicBlock("no.nullcheck");
+ llvm::Value *SLocPtr = Builder.CreateLoad(ReturnLocation, "return.sloc.load");
+ llvm::Value *CanNullCheck = Builder.CreateIsNotNull(SLocPtr);
+ if (requiresReturnValueNullabilityCheck())
+ CanNullCheck =
+ Builder.CreateAnd(CanNullCheck, RetValNullabilityPrecondition);
+ Builder.CreateCondBr(CanNullCheck, Check, NoCheck);
+ EmitBlock(Check);
- // Now do the null check. If the returns_nonnull attribute is present, this
- // is done unconditionally.
+ // Now do the null check.
llvm::Value *Cond = Builder.CreateIsNotNull(RV);
- llvm::Constant *StaticData[] = {
- EmitCheckSourceLocation(EndLoc), EmitCheckSourceLocation(AttrLoc),
- };
- EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, None);
+ llvm::Constant *StaticData[] = {EmitCheckSourceLocation(AttrLoc)};
+ llvm::Value *DynamicData[] = {SLocPtr};
+ EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, DynamicData);
- if (requiresReturnValueNullabilityCheck())
- EmitBlock(NoCheck);
+ EmitBlock(NoCheck);
+
+#ifndef NDEBUG
+ // The return location should not be used after the check has been emitted.
+ ReturnLocation = Address::invalid();
+#endif
}
static bool isInAllocaArgument(CGCXXABI &ABI, QualType type) {
diff --git a/lib/CodeGen/CGStmt.cpp b/lib/CodeGen/CGStmt.cpp
index 683f366ebe..a13c386461 100644
--- a/lib/CodeGen/CGStmt.cpp
+++ b/lib/CodeGen/CGStmt.cpp
@@ -1024,6 +1024,18 @@ void CodeGenFunction::EmitReturnOfRValue(RValue RV, QualType Ty) {
/// if the function returns void, or may be missing one if the function returns
/// non-void. Fun stuff :).
void CodeGenFunction::EmitReturnStmt(const ReturnStmt &S) {
+ if (requiresReturnValueCheck()) {
+ llvm::Constant *SLoc = EmitCheckSourceLocation(S.getLocStart());
+ auto *SLocPtr =
+ new llvm::GlobalVariable(CGM.getModule(), SLoc->getType(), false,
+ llvm::GlobalVariable::PrivateLinkage, SLoc);
+ SLocPtr->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+ CGM.getSanitizerMetadata()->disableSanitizerForGlobal(SLocPtr);
+ assert(ReturnLocation.isValid() && "No valid return location");
+ Builder.CreateStore(Builder.CreateBitCast(SLocPtr, Int8PtrTy),
+ ReturnLocation);
+ }
+
// Returning from an outlined SEH helper is UB, and we already warn on it.
if (IsOutlinedSEHHelper) {
Builder.CreateUnreachable();
diff --git a/lib/CodeGen/CodeGenFunction.cpp b/lib/CodeGen/CodeGenFunction.cpp
index 1a7797b37e..93a4a38661 100644
--- a/lib/CodeGen/CodeGenFunction.cpp
+++ b/lib/CodeGen/CodeGenFunction.cpp
@@ -860,6 +860,13 @@ void CodeGenFunction::StartFunction(GlobalDecl GD,
Builder.SetInsertPoint(EntryBB);
+ // If we're checking the return value, allocate space for a pointer to a
+ // precise source location of the checked return statement.
+ if (requiresReturnValueCheck()) {
+ ReturnLocation = CreateDefaultAlignTempAlloca(Int8PtrTy, "return.sloc.ptr");
+ InitTempAlloca(ReturnLocation, llvm::ConstantPointerNull::get(Int8PtrTy));
+ }
+
// Emit subprogram debug descriptor.
if (CGDebugInfo *DI = getDebugInfo()) {
// Reconstruct the type from the argument list so that implicit parameters,
diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h
index 11dce073c9..6785111bd0 100644
--- a/lib/CodeGen/CodeGenFunction.h
+++ b/lib/CodeGen/CodeGenFunction.h
@@ -116,9 +116,9 @@ enum TypeEvaluationKind {
SANITIZER_CHECK(MulOverflow, mul_overflow, 0) \
SANITIZER_CHECK(NegateOverflow, negate_overflow, 0) \
SANITIZER_CHECK(NullabilityArg, nullability_arg, 0) \
- SANITIZER_CHECK(NullabilityReturn, nullability_return, 0) \
+ SANITIZER_CHECK(NullabilityReturn, nullability_return, 1) \
SANITIZER_CHECK(NonnullArg, nonnull_arg, 0) \
- SANITIZER_CHECK(NonnullReturn, nonnull_return, 0) \
+ SANITIZER_CHECK(NonnullReturn, nonnull_return, 1) \
SANITIZER_CHECK(OutOfBounds, out_of_bounds, 0) \
SANITIZER_CHECK(PointerOverflow, pointer_overflow, 0) \
SANITIZER_CHECK(ShiftOutOfBounds, shift_out_of_bounds, 0) \
@@ -1407,6 +1407,17 @@ private:
return RetValNullabilityPrecondition;
}
+ /// Used to store precise source locations for return statements by the
+ /// runtime return value checks.
+ Address ReturnLocation = Address::invalid();
+
+ /// Check if the return value of this function requires sanitization.
+ bool requiresReturnValueCheck() const {
+ return requiresReturnValueNullabilityCheck() ||
+ (SanOpts.has(SanitizerKind::ReturnsNonnullAttribute) &&
+ CurCodeDecl && CurCodeDecl->getAttr<ReturnsNonNullAttr>());
+ }
+
llvm::BasicBlock *TerminateLandingPad;
llvm::BasicBlock *TerminateHandler;
llvm::BasicBlock *TrapBB;
@@ -1778,7 +1789,7 @@ public:
SourceLocation EndLoc);
/// Emit a test that checks if the return value \p RV is nonnull.
- void EmitReturnValueCheck(llvm::Value *RV, SourceLocation EndLoc);
+ void EmitReturnValueCheck(llvm::Value *RV);
/// EmitStartEHSpec - Emit the start of the exception spec.
void EmitStartEHSpec(const Decl *D);
diff --git a/test/CodeGenObjC/ubsan-nonnull-and-nullability.m b/test/CodeGenObjC/ubsan-nonnull-and-nullability.m
index b927f55cd4..db6588dac3 100644
--- a/test/CodeGenObjC/ubsan-nonnull-and-nullability.m
+++ b/test/CodeGenObjC/ubsan-nonnull-and-nullability.m
@@ -7,16 +7,26 @@
// CHECK-LABEL: define nonnull i32* @f1
__attribute__((returns_nonnull)) int *_Nonnull f1(int *_Nonnull p) {
// CHECK: entry:
+ // CHECK-NEXT: [[SLOC_PTR:%.*]] = alloca i8*
// CHECK-NEXT: [[ADDR:%.*]] = alloca i32*
+ // CHECK-NEXT: store i8* null, i8** [[SLOC_PTR]]
// CHECK-NEXT: store i32* [[P:%.*]], i32** [[ADDR]]
+ // CHECK-NEXT: store {{.*}} [[SLOC_PTR]]
// CHECK-NEXT: [[ARG:%.*]] = load i32*, i32** [[ADDR]]
+ // CHECK-NEXT: [[SLOC:%.*]] = load {{.*}} [[SLOC_PTR]]
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC]], null
+ // CHECK-NEXT: br i1 [[SLOC_NONNULL]], label %nullcheck
+ //
+ // CHECK: nullcheck:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* [[ARG]], null, !nosanitize
// CHECK-NEXT: br i1 [[ICMP]], label %[[CONT:.+]], label %[[HANDLE:[^,]+]]
// CHECK: [[HANDLE]]:
- // CHECK-NEXT: call void @__ubsan_handle_nonnull_return_abort
+ // CHECK: call void @__ubsan_handle_nonnull_return
// CHECK-NEXT: unreachable, !nosanitize
// CHECK: [[CONT]]:
- // CHECK-NEXT: ret i32*
+ // CHECK-NEXT: br label %no.nullcheck
+ // CHECK: no.nullcheck:
+ // CHECK-NEXT: ret i32* [[ARG]]
return p;
}
@@ -29,3 +39,23 @@ void call_f2() {
// CHECK-NOT: call void @__ubsan_handle_nonnull_arg_abort
f2((void *)0);
}
+
+// If the return value isn't meant to be checked, make sure we don't check it.
+// CHECK-LABEL: define i32* @f3
+int *f3(int *p) {
+ // CHECK-NOT: return.sloc
+ // CHECK-NOT: call{{.*}}ubsan
+ return p;
+}
+
+// Check for a valid "return" source location, even when there is no return
+// statement, to avoid accidentally calling the runtime.
+
+// CHECK-LABEL: define nonnull i32* @f4
+__attribute__((returns_nonnull)) int *f4() {
+ // CHECK: store i8* null, i8** [[SLOC_PTR:%.*]]
+ // CHECK: [[SLOC:%.*]] = load {{.*}} [[SLOC_PTR]]
+ // CHECK: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC]], null
+ // CHECK: br i1 [[SLOC_NONNULL]], label %nullcheck
+ // CHECK: nullcheck:
+}
diff --git a/test/CodeGenObjC/ubsan-nullability.m b/test/CodeGenObjC/ubsan-nullability.m
index 7f53ea6292..eeb24b03c8 100644
--- a/test/CodeGenObjC/ubsan-nullability.m
+++ b/test/CodeGenObjC/ubsan-nullability.m
@@ -2,7 +2,7 @@
// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
-// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 109, i32 1 {{.*}} i32 100, i32 6
+// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 100, i32 6
// CHECK: [[NONNULL_ARG_LOC:@.*]] = private unnamed_addr global {{.*}} i32 204, i32 15 {{.*}} i32 190, i32 23
// CHECK: [[NONNULL_ASSIGN1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 305, i32 9
// CHECK: [[NONNULL_ASSIGN2_LOC:@.*]] = private unnamed_addr global {{.*}} i32 405, i32 10
@@ -10,7 +10,7 @@
// CHECK: [[NONNULL_INIT1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 604, i32 25
// CHECK: [[NONNULL_INIT2_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 26
// CHECK: [[NONNULL_INIT2_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 29
-// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 817, i32 1 {{.*}} i32 800, i32 6
+// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 800, i32 6
#define NULL ((void *)0)
#define INULL ((int *)NULL)
@@ -19,14 +19,11 @@
// CHECK-LABEL: define i32* @{{.*}}nonnull_retval1
#line 100
int *_Nonnull nonnull_retval1(int *p) {
- // CHECK: br i1 true, label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
- // CHECK: [[NULL]]:
// CHECK: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC1]]
return p;
- // CHECK: [[NONULL]]:
- // CHECK-NEXT: ret i32*
+ // CHECK: ret i32*
}
#line 190
@@ -108,10 +105,13 @@ int *_Nonnull nonnull_retval2(int *_Nonnull arg1, //< Test this.
// CHECK-NEXT: [[DO_RV_CHECK_1:%.*]] = and i1 true, [[ARG1CMP]], !nosanitize
// CHECK: [[ARG2CMP:%.*]] = icmp ne i32* %arg2, null, !nosanitize
// CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[DO_RV_CHECK_1]], [[ARG2CMP]]
- // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
+ // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null
+ // CHECK-NEXT: [[DO_RV_CHECK_3:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK_2]]
+ // CHECK: br i1 [[DO_RV_CHECK_3]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC2]]
return arg1;
// CHECK: [[NONULL]]:
@@ -129,10 +129,13 @@ int *_Nonnull nonnull_retval2(int *_Nonnull arg1, //< Test this.
+(int *_Nonnull) objc_clsmethod: (int *_Nonnull) arg1 {
// CHECK: [[ARG1CMP:%.*]] = icmp ne i32* %arg1, null, !nosanitize
// CHECK-NEXT: [[DO_RV_CHECK:%.*]] = and i1 true, [[ARG1CMP]]
- // CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
+ // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null
+ // CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK]]
+ // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]:
@@ -143,10 +146,13 @@ int *_Nonnull nonnull_retval2(int *_Nonnull arg1, //< Test this.
-(int *_Nonnull) objc_method: (int *_Nonnull) arg1 {
// CHECK: [[ARG1CMP:%.*]] = icmp ne i32* %arg1, null, !nosanitize
// CHECK-NEXT: [[DO_RV_CHECK:%.*]] = and i1 true, [[ARG1CMP]]
- // CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
+ // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null
+ // CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK]]
+ // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]: