Title
inout_ptr — inconsistent release() in destructor
Status
new
Section
[inout.ptr.t]
Submitter
JeanHeyd Meneide

Created on 2021-09-16.00:00:00 last changed 4 weeks ago

Messages

Date: 2021-10-30.13:56:17

Proposed resolution:

This wording is relative to N4901.

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

    ~inout_ptr_t();
    

    -9- Let SP be POINTER_OF_OR(Smart, Pointer) ([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.

Date: 2021-10-15.00:00:00

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

Date: 2021-09-15.00:00:00

[ 2021-09-24; Reflector poll ]

Set priority to 1 after reflector poll.

Previous resolution [SUPERSEDED]:

This wording is relative to N4892.

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

    ~inout_ptr_t();
    

    -9- Let SP be POINTER_OF_OR(Smart, Pointer) ([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.

Date: 2021-09-16.00:00:00

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 ([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 [inout.ptr.t] p11.

History
Date User Action Args
2021-10-30 13:56:17adminsetmessages: + msg12201
2021-09-24 17:55:39adminsetmessages: + msg12076
2021-09-18 17:24:56adminsetmessages: + msg12042
2021-09-16 00:00:00admincreate