Date: Mon, 2 Aug 2021 22:37:33 +0000
Consider:
auto uptr = std::make_unique<BYTE[]>(25);
MyFunc(std::inout_ptr(uptr));
In the case where MyFunc sets the result to NULL, this has inconsistent behavior depending on the location of the release-statement. The implementation is expressly given two options for the placement of the release-statement by the standard, but the two options have significant semantic differences, one of them being a dangerous foot-gun (double-free).
// Option #1:
auto uptr = std::make_unique<BYTE[]>(25);
auto rawPtr = uptr.get();
uptr.release(); // UNCONDITIONAL
MyFunc(&rawPtr);
If (rawPtr)
{
uptr.reset(rawPtr);
}
// Option #2:
auto uptr = std::make_unique<BYTE[]>(25);
auto rawPtr = uptr.get();
MyFunc(&rawPtr);
If (rawPtr)
{
uptr.release(); // CONDITIONAL
uptr.reset(rawPtr);
}
If MyFunc sets rawPtr to NULL, I would assume that MyPtr has freed the old value and decided not to produce a new value. This is no problem if the implementation selected option #1, but results in double-free if the implementation selected option #2. The issue is that the "cleanup" performed by inout_ptr_t (and out_ptr_t) is conditional - setup is unconditional, but cleanup occurs only if MyFunc sets rawPtr to non-NULL.
I note a similar inconsistency with plain pointers when MyFunc returns NULL. The plain pointer retains its original value even though MyFunc has freed it. Again, this is likely going to result in a double-free.
Minimal fixes required:
* Mandate that release() occur unconditionally. It can occur in the constructor or it can occur outside of the IF statement in the destructor, but it must occur even when MyFunc sets rawPtr to NULL.
* The equivalent of release() needs to happen for plain pointers, and it also needs to be unconditional. Probably the simplest way to accomplish this is to remove the condition from the destructor for the plain pointer case so that plainPointer = rawPtr occurs even when rawPtr is NULL.
I believe that the motivation for if(rawPtr) in out_ptr_t is to avoid invoking reset(NULL), which is a non-trivial and wasteful operation for shared_ptr. While shared_ptr cannot be used with inout_ptr_t, I can understand that this might be a useful optimization for some other smart pointer types. This is a reasonable goal, but it must not interfere with the superceding goal of avoiding unexpected behavior.
>From my perspective, the expected behavior is that the "smart" pointer will ALWAYS be updated to the value returned by MyPtr, even when that value is NULL. Ideally, the same idea should apply to both out_ptr and inout_ptr, though the consequences of not meeting this expectation are far worse in the case of inout_ptr.
Additional suggested fix: out_ptr_t should set the smart pointer to "NULL" even when the returned rawPtr is NULL. Probably calling reset() or assigning sp = SmartPtr() would be appropriate in the NULL case.
Thanks!
Doug
auto uptr = std::make_unique<BYTE[]>(25);
MyFunc(std::inout_ptr(uptr));
In the case where MyFunc sets the result to NULL, this has inconsistent behavior depending on the location of the release-statement. The implementation is expressly given two options for the placement of the release-statement by the standard, but the two options have significant semantic differences, one of them being a dangerous foot-gun (double-free).
// Option #1:
auto uptr = std::make_unique<BYTE[]>(25);
auto rawPtr = uptr.get();
uptr.release(); // UNCONDITIONAL
MyFunc(&rawPtr);
If (rawPtr)
{
uptr.reset(rawPtr);
}
// Option #2:
auto uptr = std::make_unique<BYTE[]>(25);
auto rawPtr = uptr.get();
MyFunc(&rawPtr);
If (rawPtr)
{
uptr.release(); // CONDITIONAL
uptr.reset(rawPtr);
}
If MyFunc sets rawPtr to NULL, I would assume that MyPtr has freed the old value and decided not to produce a new value. This is no problem if the implementation selected option #1, but results in double-free if the implementation selected option #2. The issue is that the "cleanup" performed by inout_ptr_t (and out_ptr_t) is conditional - setup is unconditional, but cleanup occurs only if MyFunc sets rawPtr to non-NULL.
I note a similar inconsistency with plain pointers when MyFunc returns NULL. The plain pointer retains its original value even though MyFunc has freed it. Again, this is likely going to result in a double-free.
Minimal fixes required:
* Mandate that release() occur unconditionally. It can occur in the constructor or it can occur outside of the IF statement in the destructor, but it must occur even when MyFunc sets rawPtr to NULL.
* The equivalent of release() needs to happen for plain pointers, and it also needs to be unconditional. Probably the simplest way to accomplish this is to remove the condition from the destructor for the plain pointer case so that plainPointer = rawPtr occurs even when rawPtr is NULL.
I believe that the motivation for if(rawPtr) in out_ptr_t is to avoid invoking reset(NULL), which is a non-trivial and wasteful operation for shared_ptr. While shared_ptr cannot be used with inout_ptr_t, I can understand that this might be a useful optimization for some other smart pointer types. This is a reasonable goal, but it must not interfere with the superceding goal of avoiding unexpected behavior.
>From my perspective, the expected behavior is that the "smart" pointer will ALWAYS be updated to the value returned by MyPtr, even when that value is NULL. Ideally, the same idea should apply to both out_ptr and inout_ptr, though the consequences of not meeting this expectation are far worse in the case of inout_ptr.
Additional suggested fix: out_ptr_t should set the smart pointer to "NULL" even when the returned rawPtr is NULL. Probably calling reset() or assigning sp = SmartPtr() would be appropriate in the NULL case.
Thanks!
Doug
Received on 2021-08-02 17:37:38