[libc++][NFCI] Optimization to std::~unique_ptr

[atmnpatel]

Summary

The current implementation of std::~unique_ptr modifies the address held
by its ptr. This fix changes it so that it only calls the contained
destructor as suggested in the bug report.

Fixes https://llvm.org/PR45848

[zoecarver]

LGTM. Thanks :)

libcxx/include/memory

2645

Please mangle this (__ptr).

[atmnpatel]

Aye aye.

[jfb]

I agree that the modification of the pointer isn't necessary. However, this nullptr has prevented UaFs for folks, and will now stop doing so. I'm fine having a "you used UB you get whatever" as an implementation strategy, because it generally gives us better performance. However, here we're regressing a safety that we'd offered developers. I wouldn't do so lightly, even if it is UB and we're allowed to. Is there a quantifiable gain from making this change? If not, then I'd rather add a "UB but fast" and a "safe-ish and a bit slower" mode to libc++, and make this change under the umbrella. I'd make "UB but fast" the default, and publicize that we're doing such a change so that folks who want the UaF protection here and similar protections in other places can make an informed choice. Doing otherwise feeds into the narrative "compiler developers use UB to do unexpected things to my code with no actual performance gain", which isn't something I want to encourage.

[thakis]

[…} then I'd rather add a "UB but fast" and a "safe-ish and a bit slower" mode to libc++, and make this change under the umbrella. I'd make "UB but fast" the default, […]

See also D70343.

[zoecarver]

What if we did the check before modifying the pointer?

~unique_ptr() {
    pointer __tmp = __ptr_.first();
    if (!__tmp)
        return
    __ptr_.first() = __p;
    __ptr_.second()(__tmp);
}

We only get a performance improvement when the pointer is null so, this should be just as fast. And it will prevent just as many UaFs.

[ldionne]

In D80057#2045081, @zoecarver wrote:

What if we did the check before modifying the pointer?

~unique_ptr() {
    pointer __tmp = __ptr_.first();
    if (!__tmp)
        return
    __ptr_.first() = __p;
    __ptr_.second()(__tmp);
}

We only get a performance improvement when the pointer is null so, this should be just as fast. And it will prevent just as many UaFs.

In your example, I don't see what __p is.

I think the overall question is:

Do we want to set the value of the pointer in its destructor to <SOMETHING> in order to prevent UaFs.

1. The current behavior is that we set it to nullptr, which removes the possibility of UaFs, but might pessimize code and might also hide some convoluted bugs (i.e. if someone has a dangling pointer to the unique_ptr's pointer, they could fail to trigger their UB if they check it against nullptr first).
2. The behavior proposed in this patch is to simply not set the pointer in the destructor, which might open the door to some UaFs for already-buggy code.
3. Another possible behavior would be to set the pointer to some non-null-but-known-to-be-incorrect value, which would both prevent UaFs and also probably not hide convoluted bugs.

I would suggest we go with this patch, but also consider implementing (3) in a hardened mode of libc++ if there's a desire for it. Note that I think such hardened mode should be separate from whether assertions are enabled, but that's a different topic.

So, any objections to moving forward with this patch @jfb ?

[jfb]

Fine by me as long as it's heavily publicized, so folks realize what's being changed. I'd like to see a measured perf upside proving that this is worth doing, and I'd have hoped that it would have been easy enough to provide. Does the compiler usually remove the store anyways, in which case this patch isn't actually doing anything useful?

[zoecarver]

@ldionne

In your example, I don't see what __p is.

__p is nullptr. I still think this might be a good solution. Essentially exiting reset early if __ptr is null. It eliminates the store without adding any compares and it will set the pointer to null the exact same number of times (so we don't add any unsafe memory issues).

@jfb It looks like (in clang, at least) as long as everything can be inlined it can get rid of the store but, if the unique_ptr escapes, it can't. I would be really surprised if this change made a meaningful impact on performance, though. I think you'd need to create a lot of null unique_ptrs.

[ldionne]

In D80057#2049216, @zoecarver wrote:

@ldionne

In your example, I don't see what __p is.

__p is nullptr. I still think this might be a good solution. Essentially exiting reset early if __ptr is null.

We can't do that, because p.reset(x) needs to set p.__ptr_ to x, regardless of whether p.__ptr_ is null or not. Or are we talking past each other?

[...]
I think you'd need to create a lot of null unique_ptrs.

As you mentioned, there's a dead store whenever the unique_ptr escapes, so it's more than just null unique_ptrs. However, since it's in the destructor of the object, why does it matter whether the unique_ptr has escaped? If the answer is "it doesn't matter", then I think we could improve the codegen here such that the dead store is always removed. Naively, I would certainly have expected that the dead store is always removed, regardless of escaping.

However, that change would have the same security implications, except even wider because it would impact all code, not only ~unique_ptr(). @jfb Would you too expect the dead store to always be removed?

In D80057#2049148, @jfb wrote:

Fine by me as long as it's heavily publicized, so folks realize what's being changed.

We can add a release note. @atmnpatel can you add one to libcxx/docs/ReleaseNotes.rst?

[atmnpatel]

@jfb I did a microbenchmark yesterday and the upside is very small and basically non-existent for slightly non-trivial classes. In fact, there is a performance regression for the case that @zoecarver made on compiler explorer of using moves. Furthermore, for simple unique pointers such as to int, it also leads to a performance regression (for reasons which are not entirely clear to me). I think it's best to table this until it can be concretely established that this does lead to a performance improvement.

[zoecarver]

We can't do that, because p.reset(x) needs to set p.ptr_ to x, regardless of whether p.ptr_ is null or not. Or are we talking past each other?

I'm not talking about reset. I'm saying inline reset into the destructor and then exit early. Basically, replace the destructor with this:

~unique_ptr() {
    pointer __tmp = __ptr_.first();
    if (!__tmp)
        return;
    __ptr_.first() = pointer(); // pointer() = nullptr
    __ptr_.second()(__tmp);
}

As you mentioned, there's a dead store whenever the unique_ptr escapes, so it's more than just null unique_ptrs.

You're right. This probably isn't because of the destructor.

However, since it's in the destructor of the object, why does it matter whether the unique_ptr has escaped?

My guess is that it can't prove that do_complex_things doesn't make the pointer not-null.

If the answer is "it doesn't matter", then I think we could improve the codegen here such that the dead store is always removed.

I think I found the cause. This isn't the destructor, it's the move constructor. We call release() which is where we set the pointer to null.

[ldionne]

This comment was removed by ldionne.

[ldionne]

In D80057#2049512, @zoecarver wrote:

We can't do that, because p.reset(x) needs to set p.ptr_ to x, regardless of whether p.ptr_ is null or not. Or are we talking past each other?

I'm not talking about reset. I'm saying inline reset into the destructor and then exit early. Basically, replace the destructor with this:

~unique_ptr() {
    pointer __tmp = __ptr_.first();
    if (!__tmp)
        return;
    __ptr_.first() = pointer(); // pointer() = nullptr
    __ptr_.second()(__tmp);
}

Ok, got it. Alternatively:

~unique_ptr() {
    pointer __tmp = __ptr_.first();
    if (__tmp) {
        __ptr_.first() = pointer();
        __ptr_.second()(__tmp);
    }
}

Yeah, I guess this gets rid of an assignment to the pointer when it's null. But really, at that point we're not getting a lot of bang for the buck. I'd rather leave the implementation as reset(), or acknowledge the more important issue that we're setting something that's about to die, and use your original implementation.

For the time being, I'd be tempted to do nothing until we have a better way to enable assertions in libc++. If we change this and it causes even just one security vulnerability, I think we'll be losing overall because the perf benefits are most likely non-existent (at the very least the bug reporter in PR45848 doesn't prove there's a slowdown in a very convincing way IMO).

Once we have a hardened mode for libc++, we can revisit this issue and avoid setting the pointer in the normal mode. Doing so might even make it easier to find problems with ubsan.

@atmnpatel @zoecarver WDYT?

[zoecarver]

That sounds good to me.

[atmnpatel]

I agree with @ldionne

libcxx/include/memory

 
 #ifdef _LIBCPP_CXX03_LANG
   unique_ptr(unique_ptr const&) = delete;
   unique_ptr& operator=(unique_ptr const&) = delete;
 #endif
 
 
   _LIBCPP_INLINE_VISIBILITY
-  ~unique_ptr() { reset(); }
+  ~unique_ptr() {
+    const pointer ptr = __ptr_.first();
+    if (ptr)
+        __ptr_.second()(ptr);
+  }
 
   _LIBCPP_INLINE_VISIBILITY
   unique_ptr& operator=(nullptr_t) _NOEXCEPT {
     reset();
     return *this;
   }
 
   _LIBCPP_INLINE_VISIBILITY

 
 #ifdef _LIBCPP_CXX03_LANG
   unique_ptr(unique_ptr const&) = delete;
   unique_ptr& operator=(unique_ptr const&) = delete;
 #endif
 
 public:
   _LIBCPP_INLINE_VISIBILITY
-  ~unique_ptr() { reset(); }
+  ~unique_ptr() {
+    const pointer ptr = __ptr_.first();
+    if (ptr)
+        __ptr_.second()(ptr);
+  }
 
   _LIBCPP_INLINE_VISIBILITY
   unique_ptr& operator=(nullptr_t) _NOEXCEPT {
     reset();
     return *this;
   }
 
   _LIBCPP_INLINE_VISIBILITY