[Bug libstdc++/54351] New: ~unique_ptr() should not set __p to null

[Bug libstdc++/54351] New: ~unique_ptr() should not set __p to null

[gromer at google dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

             Bug #: 54351
           Summary: ~unique_ptr() should not set __p to null
    Classification: Unclassified
           Product: gcc
           Version: 4.7.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: gromer@google.com


libstdc++'s unique_ptr destructor is currently implemented as:

~unique_ptr() noexcept { reset(); }

This has the effect of resetting the stored pointer to null, and then invoking
the deleter on the formerly-stored pointer. I believe this is inconsistent with
the language standard, which specifies the destructor as "If get() == nullptr
there are no effects. Otherwise get_deleter()(get())." (note no mention of any
side effects on the value of the stored pointer). This is a problem because
this implementation will break code that (legitimately, AFAICT) relies on being
able to continue to invoke operations on the scoped_ptr while the destructor is
executing.

The fix is to reimplement the destructor (in both the base template and the
array specialization) as

~unique_ptr() noexcept {
 if (__p != pointer())
   get_deleter()(__p);
}

If the intent is to zero out __p to help catch use-after-destruction errors, I
believe it would be permissible to set __p to null after the call to
get_deleter(), because at that point the change would no longer be visible to
conforming code.

To make this concrete, here's an example: the following program prints "bad"
under libstdc++, but I believe a standard-conforming implementation is required
to print "good":

=============================
#include <iostream>
#include <memory>

using std::cout;
using std::endl;
using std::unique_ptr;

struct A;

struct B {
 unique_ptr<A> a;
};

struct A {
 B* b;
 ~A() {
   if (b->a == nullptr) {
     cout << "bad" << endl;
   } else {
     cout << "good" << endl;
   }
 }
};

int main(int argc, char** argv) {
 B b;
 b.a.reset(new A);
 b.a->b = &b;
}
===============================

As a point of comparison, MSVC++ 2010 prints "good" on this example program.

[Bug libstdc++/54351] ~unique_ptr() should not set __p to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-22 18:43:47 UTC ---
Hmm, the behaviour was probalby correct prior to fixing PR 43183, as the old
implementation of reset() did exactly what is required of the destructor.

However, the lifetime of the unique_ptr ends when the destructor starts
([basic.life]/1) so I don't believe it is legitimate to access it at that
point.

[Bug libstdc++/54351] ~unique_ptr() should not set __p to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-22 18:47:07 UTC ---
That said, whether the testcase is valid or not, I don't see any harm in making
the change.

[Bug libstdc++/54351] ~unique_ptr() should not set __p to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-22
     Ever Confirmed|0                           |1

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-22 18:51:27 UTC ---
Looking more closely, [basic.life]/5 and [class.cdtor]/1 do seem to allow your
testcase. So confirmed.

[Bug libstdc++/54351] ~unique_ptr() should not set __p to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-22 18:58:22 UTC ---
I'll test this change:

@@ -169,7 +169,13 @@
 #endif

       // Destructor.
-      ~unique_ptr() noexcept { reset(); }
+      ~unique_ptr() noexcept
+      {
+       auto& __ptr = std::get<0>(_M_t);
+       if (__ptr != pointer())
+         get_deleter()(__ptr);
+       __ptr = pointer();
+      }

       // Assignment.
       unique_ptr&

[Bug libstdc++/54351] ~unique_ptr() should not set __p to null

[gromer at google dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #5 from Geoff Romer <gromer at google dot com> 2012-08-22 19:11:17 UTC ---
Don't forget the array specialization.

Doesn't the first line of your new destructor shadow the __p member with a __p
local variable? Why is that line needed at all?

[Bug libstdc++/54351] ~unique_ptr() should not set __p to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-22 19:28:15 UTC ---
(In reply to comment #5)
> Don't forget the array specialization.

I won't :-)

> Doesn't the first line of your new destructor shadow the __p member with a __p
> local variable? Why is that line needed at all?

There is no __p member.

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[gromer at google dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #7 from Geoff Romer <gromer at google dot com> 2012-08-22 19:49:28 UTC ---
(In reply to comment #6)
> (In reply to comment #5)
> > Don't forget the array specialization.
> 
> I won't :-)
> 
> > Doesn't the first line of your new destructor shadow the __p member with a __p
> > local variable? Why is that line needed at all?
> 
> There is no __p member.

Ah, sorry, I was evidently misreading something. I've corrected the bug
description.

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-26 00:12:46 UTC ---
Author: redi
Date: Sun Aug 26 00:12:40 2012
New Revision: 190676

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190676
Log:
    PR libstdc++/54351
    * include/bits/unique_ptr.h (unique_ptr<T>::~unique_ptr): Do not use
    reset().
    (unique_ptr<T[]>::~unique_ptr()): Likewise.
    * testsuite/20_util/unique_ptr/54351.cc: New.
    * testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-error
    line numbers.

Added:
    trunk/libstdc++-v3/testsuite/20_util/unique_ptr/54351.cc
      - copied, changed from r190674,
trunk/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/unique_ptr.h
    trunk/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-26 00:29:46 UTC ---
Author: redi
Date: Sun Aug 26 00:29:41 2012
New Revision: 190681

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190681
Log:
2012-08-26  Jonathan Wakely  <jwakely.gcc@gmail.com>
        Geoff Romer  <gromer@google.com>

    PR libstdc++/54351
    * include/bits/unique_ptr.h (unique_ptr<T>::~unique_ptr): Do not use
    reset().
    (unique_ptr<T[]>::~unique_ptr()): Likewise.
    * testsuite/20_util/unique_ptr/54351.cc: New.
    * testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-error
    line numbers.

Added:
    branches/gcc-4_7-branch/libstdc++-v3/testsuite/20_util/unique_ptr/54351.cc
      - copied, changed from r190679,
branches/gcc-4_7-branch/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/include/bits/unique_ptr.h
   
branches/gcc-4_7-branch/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-26 00:31:08 UTC ---
fixed for 4.7.2

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[gromer at google dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #11 from Geoff Romer <gromer at google dot com> 2012-12-18 21:59:23 UTC ---
>From discussion on the C++ LWG reflector, it appears that the standard's
requirements on library types are intended to apply only during their lifetime,
although the standard does not currently make this clear. LWG 2224
(http://cplusplus.github.com/LWG/lwg-active.html#2224) is tracking this issue,
and its resolution should give a definitive answer.

That being the case, the old behavior was not a bug, but conformant with the
the intent of the standard (if not the precise wording). The new behavior is
conformant as well, of course, so it's up to you whether to revert the changes;
I just wanted to document for future reference that ~unique_ptr is in fact
permitted to modify the stored pointer before invoking the deleter, so this bug
report should not be an obstacle to e.g. having ~unique_ptr store a poison
value in order to catch client bugs.

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[rs2740 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

TC <rs2740 at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rs2740 at gmail dot com

--- Comment #12 from TC <rs2740 at gmail dot com> ---
~unique_ptr()'s specification doesn't say that it assigns to the stored pointer
at any time. Since unique_ptr can be used with pointer-like things whose
assignment operator can have observable side effects, is doing this assignment
in the destructor conforming?

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It isn't specified whether it is assigned to or not in the destructor, so I
think it's conforming.

D::pointer is required to meet the NullablePointer requirements, which includes
CopyAssignable, so in the absence of any restrictions to the contrary the
implementation can rely on assignment (and typically both release() and reset()
will assign to the pointer, and reset() is a valid implementation of the
destructor).

[Bug libstdc++/54351] ~unique_ptr() should not set stored pointer to null

[rs2740 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54351

--- Comment #14 from TC <rs2740 at gmail dot com> ---
Well, I would have argued that if the specification doesn't say that a function
does X, then it doesn't do X. NullablePointer/CopyAssignable only means that
the assignment operation must be supported.

But then I realized that a literal reading of the specification would mean that
the deleter must be called using a copy of the stored pointer - the return
value of get() - rather than the stored pointer itself (again, hypothetically
detectable using an instrumented fancy pointer-like thing). Which would be just
absurd.