r311695 - [ubsan] PR34266: When sanitizing the 'this' value for a member function that happens to be a lambda call operator, use the lambda's 'this' pointer, not the captured enclosing 'this' pointer (if any).

Fri Aug 25 13:29:13 PDT 2017

r311799. Thanks!

On Thu, Aug 24, 2017 at 7:14 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Hi Hans,
>
> This fixes a regression in ubsan's handling of lambda expressions. Seems
> like a reasonable candidate for Clang 5, but it is rather late in the day...
>
>
> On 24 August 2017 at 13:10, Richard Smith via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>>
>> Author: rsmith
>> Date: Thu Aug 24 13:10:33 2017
>> New Revision: 311695
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=311695&view=rev
>> Log:
>> [ubsan] PR34266: When sanitizing the 'this' value for a member function
>> that happens to be a lambda call operator, use the lambda's 'this' pointer,
>> not the captured enclosing 'this' pointer (if any).
>>
>> Do not sanitize the 'this' pointer of a member call operator for a lambda
>> with
>> no capture-default, since that call operator can legitimately be called
>> with a
>> null this pointer from the static invoker function. Any actual call with a
>> null
>> this pointer should still be caught in the caller (if it is being
>> sanitized).
>>
>> This reinstates r311589 (reverted in r311680) with the above fix.
>>
>> Modified:
>>     cfe/trunk/include/clang/AST/DeclCXX.h
>>     cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
>>     cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=311695&r1=311694&r2=311695&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
>> +++ cfe/trunk/include/clang/AST/DeclCXX.h Thu Aug 24 13:10:33 2017
>> @@ -2027,7 +2027,10 @@ public:
>>
>>    /// \brief Returns the type of the \c this pointer.
>>    ///
>> -  /// Should only be called for instance (i.e., non-static) methods.
>> +  /// Should only be called for instance (i.e., non-static) methods. Note
>> +  /// that for the call operator of a lambda closure type, this returns
>> the
>> +  /// desugared 'this' type (a pointer to the closure type), not the
>> captured
>> +  /// 'this' type.
>>    QualType getThisType(ASTContext &C) const;
>>
>>    unsigned getTypeQualifiers() const {
>>
>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=311695&r1=311694&r2=311695&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Thu Aug 24 13:10:33 2017
>> @@ -22,6 +22,7 @@
>>  #include "CodeGenPGO.h"
>>  #include "TargetInfo.h"
>>  #include "clang/AST/ASTContext.h"
>> +#include "clang/AST/ASTLambda.h"
>>  #include "clang/AST/Decl.h"
>>  #include "clang/AST/DeclCXX.h"
>>  #include "clang/AST/StmtCXX.h"
>> @@ -1014,11 +1015,22 @@ void CodeGenFunction::StartFunction(Glob
>>      }
>>
>>      // Check the 'this' pointer once per function, if it's available.
>> -    if (CXXThisValue) {
>> +    if (CXXABIThisValue) {
>>        SanitizerSet SkippedChecks;
>>        SkippedChecks.set(SanitizerKind::ObjectSize, true);
>>        QualType ThisTy = MD->getThisType(getContext());
>> -      EmitTypeCheck(TCK_Load, Loc, CXXThisValue, ThisTy,
>> +
>> +      // If this is the call operator of a lambda with no
>> capture-default, it
>> +      // may have a static invoker function, which may call this operator
>> with
>> +      // a null 'this' pointer.
>> +      if (isLambdaCallOperator(MD) &&
>> +          cast<CXXRecordDecl>(MD->getParent())->getLambdaCaptureDefault()
>> ==
>> +              LCD_None)
>> +        SkippedChecks.set(SanitizerKind::Null, true);
>> +
>> +      EmitTypeCheck(isa<CXXConstructorDecl>(MD) ? TCK_ConstructorCall
>> +                                                : TCK_MemberCall,
>> +                    Loc, CXXABIThisValue, ThisTy,
>>
>> getContext().getTypeAlignInChars(ThisTy->getPointeeType()),
>>                      SkippedChecks);
>>      }
>>
>> Modified: cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp?rev=311695&r1=311694&r2=311695&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp (original)
>> +++ cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp Thu Aug 24 13:10:33
>> 2017
>> @@ -449,6 +449,28 @@ void upcast_to_vbase() {
>>  }
>>  }
>>
>> +struct ThisAlign {
>> +  void this_align_lambda();
>> +  void this_align_lambda_2();
>> +};
>> +void ThisAlign::this_align_lambda() {
>> +  // CHECK-LABEL: define
>> {{.*}}@"_ZZN9ThisAlign17this_align_lambdaEvENK3$_0clEv"
>> +  // CHECK-SAME: (%{{.*}}* %[[this:[^)]*]])
>> +  // CHECK: %[[this_addr:.*]] = alloca
>> +  // CHECK: store %{{.*}}* %[[this]], %{{.*}}** %[[this_addr]],
>> +  // CHECK: %[[this_inner:.*]] = load %{{.*}}*, %{{.*}}** %[[this_addr]],
>> +  // CHECK: %[[this_outer_addr:.*]] = getelementptr inbounds %{{.*}},
>> %{{.*}}* %[[this_inner]], i32 0, i32 0
>> +  // CHECK: %[[this_outer:.*]] = load %{{.*}}*, %{{.*}}**
>> %[[this_outer_addr]],
>> +  //
>> +  // CHECK: %[[this_inner_isnonnull:.*]] = icmp ne %{{.*}}*
>> %[[this_inner]], null
>> +  // CHECK: %[[this_inner_asint:.*]] = ptrtoint %{{.*}}* %[[this_inner]]
>> to i
>> +  // CHECK: %[[this_inner_misalignment:.*]] = and i{{32|64}}
>> %[[this_inner_asint]], {{3|7}},
>> +  // CHECK: %[[this_inner_isaligned:.*]] = icmp eq i{{32|64}}
>> %[[this_inner_misalignment]], 0
>> +  // CHECK: %[[this_inner_valid:.*]] = and i1 %[[this_inner_isnonnull]],
>> %[[this_inner_isaligned]],
>> +  // CHECK: br i1 %[[this_inner_valid:.*]]
>> +  [&] { return this; } ();
>> +}
>> +
>>  namespace CopyValueRepresentation {
>>    // CHECK-LABEL: define {{.*}} @_ZN23CopyValueRepresentation2S3aSERKS0_
>>    // CHECK-NOT: call {{.*}} @__ubsan_handle_load_invalid_value
>> @@ -532,4 +554,18 @@ namespace CopyValueRepresentation {
>>    }
>>  }
>>
>> +void ThisAlign::this_align_lambda_2() {
>> +  // CHECK-LABEL: define
>> {{.*}}@"_ZZN9ThisAlign19this_align_lambda_2EvENK3$_1clEv"
>> +  // CHECK-SAME: (%{{.*}}* %[[this:[^)]*]])
>> +  // CHECK: %[[this_addr:.*]] = alloca
>> +  // CHECK: store %{{.*}}* %[[this]], %{{.*}}** %[[this_addr]],
>> +  // CHECK: %[[this_inner:.*]] = load %{{.*}}*, %{{.*}}** %[[this_addr]],
>> +  //
>> +  // Do not perform a null check on the 'this' pointer if the function
>> might be
>> +  // called from a static invoker.
>> +  // CHECK-NOT: icmp ne %{{.*}}* %[[this_inner]], null
>> +  auto *p = +[] {};
>> +  p();
>> +}
>> +
>>  // CHECK: attributes [[NR_NUW]] = { noreturn nounwind }
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>