Stl: ~unique_ptr is not safe to self reassignment

[webreh]

https://github.com/microsoft/STL/blob/92508bed6387cbdae433fc86279bc446af6f1b1a/stl/inc/memory#L1871

This should be reset(). Imagine unique_ptr<_Ty> pointer and ~_Ty calling pointer = nullptr;
See also: https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2588

[BillyONeal]

This is not a bug. When the object has been destroyed, the object is destroyed. Introducing extra writes to null out this value enables nothing the Standard allows, does not result in a reasonable extension, and has a non-trivial performance cost (particularly in cases like vector).

[webreh]

This is not a bug. When the object has been destroyed, the object is destroyed. Introducing extra writes to null out this value enables nothing the Standard allows, does not result in a reasonable extension, and has a non-trivial performance cost (particularly in cases like vector).

The object has not been destroyed as reset is called directly from unique_ptr destructor. This case is exactly the case causing reset to first reassign the pointer and then calling deleter (this is specially noted in the standard). The case workaround is directly call reset in the unique_ptr owner destructor and that is the only way to be safe working with so called smart pointer. There is no performance issues here as if the _Ty dtor is accesible the assignment will be optimized out and in the case of virtual destructor delete and virtual call costs are larger a lot.
Once again. WHILE destructing (not after its lifetime is ended!) unique_ptr is in invalid state and any access (like shell delete file causing examining message queue causing SendMessage delivered causing heap corruption) leads to a major problem.

[BillyONeal]

The object has not been destroyed as reset is called directly from unique_ptr destructor.

Then that's undefined behavior in 2 ways. First, under [reentrancy] ( http://eel.is/c++draft/reentrancy ) the standard library does not allow functions to be recursively reentered unless indicated otherwise, and unique_ptr's dtor doesn't say that. Second, the Core language considers the unique_ptr destroyed when the destructor begins execution, not when it completes execution, and Core says you can't destroy an object that's already destroyed. ( http://eel.is/c++draft/basic.life#1.4 )

[webreh]

The object has not been destroyed as reset is called directly from unique_ptr destructor.

Then that's undefined behavior in 2 ways. First, under [reentrancy] ( http://eel.is/c++draft/reentrancy ) the standard library does not allow functions to be recursively reentered unless indicated otherwise, and unique_ptr's dtor doesn't say that. Second, the Core language considers the unique_ptr destroyed when the destructor _begins execution_, not when it completes execution, and Core says you can't destroy an object that's already destroyed. ( http://eel.is/c++draft/basic.life#1.4 )

There is neither reentrance nor twice destruction of unique_ptr, there is reset() called because of the get_deleter()(pointer()) call. I agree that formally unique_ptr lifetime ended due to basic.life#1.4 but the destruction time is regulated by class.cdtor that allows any functions (including reset() of unique_ptr!) to be called (http://eel.is/c++draft/class.cdtor#4)

[CaseyCarter]

under [reentrancy] ( http://eel.is/c++draft/reentrancy ) the standard library does not allow functions to be recursively reentered unless indicated otherwise

IIUC, the scenario @webreh is concerned with is something like:

struct S {
  std::unique_ptr<S>* ptr = nullptr;
  ~S() { *ptr = nullptr; }
};

void foo() {
  std::unique_ptr<S> x = std::make_unique<S>(&x);
}

when x is destroyed at the closing brace in foo, it destroys its owned S object, which calls the unique_ptr's nullptr_t assignment operator. This doesn't match the precise wording in [reentrancy], since the function being re-entered here is S's destructor and not a Standard Library function.

That said, I've discussed with other LWG members in the past that we should forbid simultaneous active calls to member functions of the same standard library object when at least one of the member functions potentially modifies the object. In other words, we should forbid exactly the kinds of simultaneous active calls that would produce data races if they were made simultaneously on different threads. The design intent is that standard library objects are in valid states between calls to member functions, not that they must be in valid states at all times when user code runs which would be prohibitively expensive. I suppose we could submit an issue to update and clarify the wording in [reentrancy]?

[webreh]

@BillyONeal yes, this is a good way @CaseyCarter shown. Here is

#include <memory>

struct guard {
    std::unique_ptr<guard> & guarded;
    guard(std::unique_ptr<guard> & guarded) : guarded(guarded) {}
    ~guard() { guarded = nullptr; }
};

struct test_ok {
    std::unique_ptr<guard> pointer;
    test_ok() { pointer = std::make_unique<guard>(pointer); }
    ~test_ok() { pointer.reset(); } //this will be ok 
};

struct test_failed {
    std::unique_ptr<guard> pointer;
    test_failed() { pointer = std::make_unique<guard>(pointer); }
};

int main() {
    test_ok{};
    test_failed{}; //heap corruption
}

Even if the reentrance wordings changed this behavior is superdangerous and basically means the current unique_ptr implementation MUST NOT be used for objects with virtual dtor (that is almost all the cases it is intended to use).

[BillyONeal]

@CaseyCarter is saying "we might want an LWG issue to clarify that what you are doing is forbidden", not "the implementation here should be changed".

Note also http://eel.is/c++draft/unique.ptr#single.dtor-2 which explicitly says that the dtor just calls the deleter.

[StephanTLavavej]

This is LWG 2414 “Member function reentrancy should be implementation-defined”.

[webreh]

I know about unique.ptr#single.dtor-2 and this looks like a potential defect report.

I agree with @CaseyCarter that this kind of behavior should be implementation defined for e.g. std::vector, but std::unique_ptr is a sui generis as every its reentrance (except for this) is well-defined.

Well... we will wait for security vulnerability caused by this as no one expects reset() for noexcept destructible _Ty under smart pointer to be a potential cause of heap corruption and this is undetectable by any static analyzers for polymorphic objects.