[libcxx-commits] [PATCH] D80057: [libc++][NFCI] Optimization to std::~unique_ptr

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

In D80057#2045081 <https://reviews.llvm.org/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 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80057/new/

https://reviews.llvm.org/D80057