3594. inout_ptr — inconsistent release() in destructor

Section: 20.3.4.3 [inout.ptr.t] Status: C++23 Submitter: JeanHeyd Meneide Opened: 2021-09-16 Last modified: 2023-11-22

Priority: 1

View all other issues in [inout.ptr.t].

View all issues with C++23 status.

Discussion:

More succinctly, the model for std::out_ptr_t and std::inout_ptr_t both have a conditional release in their destructors as part of their specification (20.3.4.1 [out.ptr.t] p8) (Option #2 below). But, if the wording is followed, they have an unconditional release in their constructor (Option #1 below). This is not exactly correct and can cause issues with double-free in inout_ptr in particular.

Consider a function MyFunc that sets rawPtr to nullptr when freeing an old value and deciding not to produce a new value, as shown below:

// 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);
}

This is no problem if the implementation selected Option #1 (release in the constructor), but results in double-free if the implementation selected option #2 (release in the destructor).

As the paper author and after conferring with others, the intent was that the behavior was identical and whether a choice between the constructor or destructor is made. The reset should be unconditional, at least for inout_ptr_t. Suggested change for the ~inout_ptr_t destructor text is to remove the "if (p) { ... }" wrapper from around the code in 20.3.4.3 [inout.ptr.t] p11.

[2021-09-24; Reflector poll]

Set priority to 1 after reflector poll.

Previous resolution [SUPERSEDED]:

This wording is relative to N4892.

1. Modify 20.3.4.3 [inout.ptr.t] as indicated:

   ~inout_ptr_t();
   

   -9- Let SP be POINTER_OF_OR(Smart, Pointer) (20.2.1 [memory.general]).

   -10- Let release-statement be s.release(); if an implementation does not call s.release() in the constructor. Otherwise, it is empty.

   -11- Effects: Equivalent to:

   1. (11.1) —

      if (p) {
        apply([&](auto&&... args) {
        s = Smart( static_cast<SP>(p), std::forward<Args>(args)...); }, std::move(a));
      }
      

      if is_pointer_v<Smart> is true;

   2. (11.2) — otherwise,

      if (p) {
        apply([&](auto&&... args) {
        release-statement;
        s.reset(static_cast<SP>(p), std::forward<Args>(args)...); }, std::move(a));
      }
      

      if the expression s.reset(static_cast<SP>(p), std::forward<Args>(args)...) is well-formed;

   3. (11.3) — otherwise,

      if (p) {
        apply([&](auto&&... args) {
        release-statement;
        s = Smart(static_cast<SP>(p), std::forward<Args>(args)...); }, std::move(a));
      }
      

      if is_constructible_v<Smart, SP, Args...> is true;

   4. (11.4) — otherwise, the program is ill-formed.

[2021-10-28; JeanHeyd Meneide provides improved wording]

[2022-08-24 Approved unanimously in LWG telecon.]

[2022-11-12 Approved at November 2022 meeting in Kona. Status changed: Voting → WP.]

Proposed resolution:

This wording is relative to N4901.

1. Modify 20.3.4.3 [inout.ptr.t] as indicated:

   ~inout_ptr_t();
   

   -9- Let SP be POINTER_OF_OR(Smart, Pointer) (20.2.1 [memory.general]).

   -10- Let release-statement be s.release(); if an implementation does not call s.release() in the constructor. Otherwise, it is empty.

   -11- Effects: Equivalent to:

   1. (11.1) —

      if (p) {
        apply([&](auto&&... args) {
        s = Smart( static_cast<SP>(p), std::forward<Args>(args)...); }, std::move(a));
      }
      

      if is_pointer_v<Smart> is true;

   2. (11.2) — otherwise,

      release-statement;
      if (p) {
        apply([&](auto&&... args) {
        release-statement;
        s.reset(static_cast<SP>(p), std::forward<Args>(args)...); }, std::move(a));
      }
      

      if the expression s.reset(static_cast<SP>(p), std::forward<Args>(args)...) is well-formed;

   3. (11.3) — otherwise,

      release-statement;
      if (p) {
        apply([&](auto&&... args) {
        release-statement;
        s = Smart(static_cast<SP>(p), std::forward<Args>(args)...); }, std::move(a));
      }
      

      if is_constructible_v<Smart, SP, Args...> is true;

   4. (11.4) — otherwise, the program is ill-formed.