[ubsan] Null-check pointers in -fsanitize=vptr (PR33881)
The instrumentation generated by -fsanitize=vptr does not null check a
user pointer before loading from it. This causes crashes in the face of
UB member calls (this=nullptr), i.e it's causing user programs to crash
only after UBSan is turned on.
The fix is to make run-time null checking a prerequisite for enabling
-fsanitize=vptr, and to then teach UBSan to reuse these run-time null
checks to make -fsanitize=vptr safe.
Testing: check-clang, check-ubsan, a stage2 ubsan-enabled build
Differential Revision: https://reviews.llvm.org/D35735
https://bugs.llvm.org/show_bug.cgi?id=33881
llvm-svn: 309007
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 9572bd3..e9ad05c 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -604,20 +604,23 @@
auto PtrToAlloca =
dyn_cast<llvm::AllocaInst>(Ptr->stripPointerCastsNoFollowAliases());
+ llvm::Value *IsNonNull = nullptr;
+ bool IsGuaranteedNonNull =
+ SkippedChecks.has(SanitizerKind::Null) || PtrToAlloca;
bool AllowNullPointers = TCK == TCK_DowncastPointer || TCK == TCK_Upcast ||
TCK == TCK_UpcastToVirtualBase;
if ((SanOpts.has(SanitizerKind::Null) || AllowNullPointers) &&
- !SkippedChecks.has(SanitizerKind::Null) && !PtrToAlloca) {
+ !IsGuaranteedNonNull) {
// The glvalue must not be an empty glvalue.
- llvm::Value *IsNonNull = Builder.CreateIsNotNull(Ptr);
+ IsNonNull = Builder.CreateIsNotNull(Ptr);
// The IR builder can constant-fold the null check if the pointer points to
// a constant.
- bool PtrIsNonNull =
+ IsGuaranteedNonNull =
IsNonNull == llvm::ConstantInt::getTrue(getLLVMContext());
// Skip the null check if the pointer is known to be non-null.
- if (!PtrIsNonNull) {
+ if (!IsGuaranteedNonNull) {
if (AllowNullPointers) {
// When performing pointer casts, it's OK if the value is null.
// Skip the remaining checks in that case.
@@ -691,12 +694,24 @@
// -- the [pointer or glvalue] is used to access a non-static data member
// or call a non-static member function
CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
+ bool HasNullCheck = IsGuaranteedNonNull || IsNonNull;
if (SanOpts.has(SanitizerKind::Vptr) &&
- !SkippedChecks.has(SanitizerKind::Vptr) &&
+ !SkippedChecks.has(SanitizerKind::Vptr) && HasNullCheck &&
(TCK == TCK_MemberAccess || TCK == TCK_MemberCall ||
TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference ||
TCK == TCK_UpcastToVirtualBase) &&
RD && RD->hasDefinition() && RD->isDynamicClass()) {
+ // Ensure that the pointer is non-null before loading it. If there is no
+ // compile-time guarantee, reuse the run-time null check.
+ if (!IsGuaranteedNonNull) {
+ assert(IsNonNull && "Missing run-time null check");
+ if (!Done)
+ Done = createBasicBlock("vptr.null");
+ llvm::BasicBlock *VptrNotNull = createBasicBlock("vptr.not.null");
+ Builder.CreateCondBr(IsNonNull, VptrNotNull, Done);
+ EmitBlock(VptrNotNull);
+ }
+
// Compute a hash of the mangled name of the type.
//
// FIXME: This is not guaranteed to be deterministic! Move to a