This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Use the object pointer's type info for devirtualized calls
ClosedPublic

Authored by vsk on Oct 10 2016, 12:53 PM.

Details

Summary

ubsan reports a false positive 'invalid member call' diagnostic on the
following example (PR30478):

struct Base1 {
  virtual int f1() { return 1; }
};

struct Base2 {
  virtual int f1() { return 2; }
};

struct Derived2 final : Base1, Base2 {
  int f1() override { return 3; }
};

int t1() {
  Derived2 d;
  return static_cast<Base2 *>(&d)->f1();
}

Adding the "final" attribute to a most-derived class allows clang to
devirtualize member calls into an instance of that class. We should pass
along the type info of the object pointer to avoid the FP. In this case,
that means passing along the type info for 'Derived2' instead of 'Base2'
when checking the dynamic type of 'static_cast<Base2 *>(&d2)'.

Diff Detail

Event Timeline

vsk updated this revision to Diff 74164.Oct 10 2016, 12:53 PM
vsk retitled this revision from to [ubsan] Disable -fsanitize=vptr checks for devirtualized calls.
vsk updated this object.
vsk added reviewers: pcc, rjmccall.
vsk added subscribers: krasin, cfe-commits.
rjmccall requested changes to this revision.Oct 13 2016, 10:52 AM
rjmccall edited edge metadata.
rjmccall added inline comments.
lib/CodeGen/CodeGenFunction.h
379

Please find some way to do this that doesn't require adding new tracking state like this. It should be quite easy to pass down that a call was devirtualized.

This revision now requires changes to proceed.Oct 13 2016, 10:52 AM
vsk updated this revision to Diff 74606.Oct 13 2016, 6:25 PM
vsk edited edge metadata.
  • Remove the set of blacklisted object pointers; pass down a flag which indicates whether or not the member call check should be skipped.

Wait, can you talk me through the bug here? Why is final-based devirtualization here different from, say, user-directed devirtualization via a qualified method name?

It sounds to me from your description that you're not sure why this is happening. If this indeed only triggers in the presence of multiple inheritance, it might just be the case that you're doing your object-extents analysis starting from the wrong offset.

vsk added a comment.Oct 17 2016, 12:06 PM

Thanks for your feedback so far, and sorry for the delayed response.

Wait, can you talk me through the bug here?

Derived inherits from Base1 and Base2. We upcast an instance of Derived to Base2, then call a method (f1). Clang figures out that the method has to be Derived::f1. Next, clang passes in a pointer to the vtable pointer for Base1, along with the typeinfo for Base2, into the sanitizer runtime. This confuses the runtime, which reports that the dynamic type of the object is "Base1", and that this does not match the expected type ("Base2").

With the 'final' keyword:

%6 = ptrtoint <2 x i32 (...)**>* %1 to i64
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %6, ...)

Without the 'final' keyword:

%6 = bitcast <2 x i32 (...)**>* %1 to %class.Derived*
%7 = getelementptr inbounds %class.Derived, %class.Derived* %6, i64 0, i32 1
%8 = ptrtoint %class.Base2* %7 to i64, !nosanitize !5
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %8, ...)

Why is final-based devirtualization here different from, say, user-directed devirtualization via a qualified method name?

I'm not sure. I tested this by removing the 'final' specifier from 'Derived' and calling:

obj.Derived::f1()

In this case, clang passes the correct typeinfo (for Derived) in to the runtime.

It sounds to me from your description that you're not sure why this is happening. If this indeed only triggers in the presence of multiple inheritance, it might just be the case that you're doing your object-extents analysis starting from the wrong offset.

It looks like I just haven't done a good job of explaining the issue. The bug really does seem to be that clang isn't passing the right information to the ubsan runtime. However, I'm not sure what the right fix is. Do we disable sanitization in cases where we expect FP's, do we try harder to pass in the right vptr (in this case, the vptr for Base2) into the runtime, or do we try harder to pass in the right typeinfo (in this case, the typeinfo for Derived)?

In D25448#571941, @vsk wrote:

Thanks for your feedback so far, and sorry for the delayed response.

Wait, can you talk me through the bug here?

Derived inherits from Base1 and Base2. We upcast an instance of Derived to Base2, then call a method (f1). Clang figures out that the method has to be Derived::f1. Next, clang passes in a pointer to the vtable pointer for Base1, along with the typeinfo for Base2, into the sanitizer runtime. This confuses the runtime, which reports that the dynamic type of the object is "Base1", and that this does not match the expected type ("Base2").

A pointer to the vtable pointer for Base1 is a pointer to a Derived. You have a multiple inheritance bug, or really a general inheritance bug. It's being covered up in the case of single, non-virtual inheritance because that's the case in which a pointer to a base-class object is the same as a pointer to the derived class object.

When the devirtualizer sees what's formally a call to a method on Base2, and it decides that it can instead treat it as a call to a method on Derived, it has to adjust the 'this' pointer to point to a Derived. (I don't know off-hand whether it does this by retroactively adjusting the pointer or whether it just avoids adjusting it in the first place.) When it does this, it should also be changing its notion of what class the pointer points to.

With the 'final' keyword:

%6 = ptrtoint <2 x i32 (...)**>* %1 to i64
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %6, ...)

Without the 'final' keyword:

%6 = bitcast <2 x i32 (...)**>* %1 to %class.Derived*
%7 = getelementptr inbounds %class.Derived, %class.Derived* %6, i64 0, i32 1
%8 = ptrtoint %class.Base2* %7 to i64, !nosanitize !5
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %8, ...)

Why is final-based devirtualization here different from, say, user-directed devirtualization via a qualified method name?

I'm not sure. I tested this by removing the 'final' specifier from 'Derived' and calling:

obj.Derived::f1()

In this case, clang passes the correct typeinfo (for Derived) in to the runtime.

It sounds to me from your description that you're not sure why this is happening. If this indeed only triggers in the presence of multiple inheritance, it might just be the case that you're doing your object-extents analysis starting from the wrong offset.

It looks like I just haven't done a good job of explaining the issue. The bug really does seem to be that clang isn't passing the right information to the ubsan runtime. However, I'm not sure what the right fix is. Do we disable sanitization in cases where we expect FP's, do we try harder to pass in the right vptr (in this case, the vptr for Base2) into the runtime, or do we try harder to pass in the right typeinfo (in this case, the typeinfo for Derived)?

vsk updated this revision to Diff 74916.Oct 17 2016, 3:28 PM
vsk retitled this revision from [ubsan] Disable -fsanitize=vptr checks for devirtualized calls to [ubsan] Use the object pointer's type info for devirtualized calls.
vsk updated this object.
vsk edited edge metadata.
vsk added a subscriber: rsmith.

Patch update: Pass along the type info of the derived class to the ubsan runtime when we devirtualize a method call. This squashes the FP. I tested this with 'check-ubsan' in addition to adding a lit test.

A pointer to the vtable pointer for Base1 is a pointer to a Derived. You have a multiple inheritance bug, or really a general inheritance bug. It's being covered up in the case of single, non-virtual inheritance because that's the case in which a pointer to a base-class object is the same as a pointer to the derived class object.

I imagine that it would be difficult to extend the runtime to handle this case. I.e, given a pointer to vtable pointer for Base1 and the type info for Base2, recognize that we may _actually_ be looking at an instance of Derived, and therefore claim that the types match. I wonder if that would result in a false negative in this case:

Base1 b1;
reinterpret_cast<Base2 *>(b1)->method_from_base2_only()

We are currently able to diagnose this.

... it should also be changing its notion of what class the pointer points to.

I'm taking this to mean that we should pass along the type information for 'Derived'. This is also what @rsmith suggests.

vsk updated this revision to Diff 74943.Oct 17 2016, 9:37 PM
  • Remove some default arguments left over from an older revision of this patch.
  • Simplify the test.
In D25448#572245, @vsk wrote:

Patch update: Pass along the type info of the derived class to the ubsan runtime when we devirtualize a method call. This squashes the FP. I tested this with 'check-ubsan' in addition to adding a lit test.

A pointer to the vtable pointer for Base1 is a pointer to a Derived. You have a multiple inheritance bug, or really a general inheritance bug. It's being covered up in the case of single, non-virtual inheritance because that's the case in which a pointer to a base-class object is the same as a pointer to the derived class object.

I imagine that it would be difficult to extend the runtime to handle this case. I.e, given a pointer to vtable pointer for Base1 and the type info for Base2, recognize that we may _actually_ be looking at an instance of Derived, and therefore claim that the types match.

I don't know how the runtime parts of this check are implemented. It is certainly true that an object that is a Base1 might also be a Base2 due to multiple inheritance. But given a pointer that's statically typed to be a Base2, I believe you want to be checking not "am I pointing at an object whose dynamic type derives from Base2" but "am I pointing *at the Base2 subobject* of an object whose dynamic type derives from Base1", which is a very different check and doesn't necessarily require the same runtime complexity (or might need more, depending on what you're doing).

But anyway, the pointer you're passing does not point to the Base2 subobject anymore, which is clearly a frontend bug, not a runtime bug.

I wonder if that would result in a false negative in this case:

Base1 b1;
reinterpret_cast<Base2 *>(b1)->method_from_base2_only()

We are currently able to diagnose this.

I can't imagine what on earth you would do in the runtime that would have a false-negative problem here without basically disabling the check, but let's leave that aside.

... it should also be changing its notion of what class the pointer points to.

I'm taking this to mean that we should pass along the type information for 'Derived'. This is also what @rsmith suggests.

Yes, this is what you should do. Your runtime logic is, somehow, checking whether the memory contains an object of some type that you know statically. Devirt is potentially changing where your pointer points and thus also changing the expected static type of the memory you're pointing to. Properly responding to that change will not only fix the logic here, it will strengthen your check so that you also detect the bug here:

Base1 p;
Derived *badp = static_cast<Derived*>(&p);
static_cast<Base1*>(badp)->method_that_is_final_in_derived();
rjmccall added inline comments.Oct 17 2016, 9:51 PM
lib/CodeGen/CGExprCXX.cpp
31

Shouldn't MD just be the devirtualized method? That should avoid the need to pass down something different for the devirt case here.

vsk updated this revision to Diff 75168.Oct 19 2016, 10:35 AM

Thanks for the feedback!

Patch update:

  • Pass in the right CalleeDecl to EmitCXXMemberOrOperatorCall. This lets us drop the unnecessary 'DevirtualizedClassTy' optional parameter.
  • Extend the test case with John's example (devirt for methods marked 'final'). Since this hasn't yet been implemented in clang, make the checks depend on the relevant bug (PR13127).

Tested with check-ubsan and check-clang.

vsk marked an inline comment as done.Oct 19 2016, 10:35 AM
rjmccall accepted this revision.Oct 19 2016, 11:41 AM
rjmccall edited edge metadata.

Looks great, thanks.

This revision is now accepted and ready to land.Oct 19 2016, 11:41 AM
This revision was automatically updated to reflect the committed changes.