[Bug libstdc++/58159] New: unique_ptr::reset should have debug assertion for "self-reset"

[Bug libstdc++/58159] New: unique_ptr::reset should have debug assertion for "self-reset"

[gromer at google dot com]

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

            Bug ID: 58159
           Summary: unique_ptr::reset should have debug assertion for
                    "self-reset"
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gromer at google dot com

unique_ptr::reset() should have a debug assertion for the case that the input
pointer is not null, and is equal to the pointer already stored. This is
virtually always an error: it violates the basic ownership logic of unique_ptr,
and leaves unique_ptr holding a dangling pointer. Strictly speaking, this is
not undefined behavior in itself, but almost any subsequent operation other
than release() (even destroying the unique_ptr) will produce undefined
behavior, so this seems like a highly defensible condition to assert on, at
least in debug mode.

This kind of error is rare, but it does happen; see e.g.
https://code.google.com/p/chromium/issues/detail?id=162971.

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[gromer at google dot com]

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

Geoff Romer <gromer at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gromer at google dot com
           Severity|normal                      |enhancement

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[redi at gcc dot gnu.org]

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

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
What if the deleter doesn't actually destroy the object, and doing self-reset
is used as a crazy way to trigger the deleter to do something with the pointer,
but not to alter the value of the pointer?

If that's valid then we should only do this for default_delete specializations.

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I'm also a little concerned that doing a self-reset followed by release() is
indeed valid ... but probably rare enough that we can still assert anyway at
the time of the self-reset.

I think this is a good idea, but we should be careful to do it right.

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[gromer at google dot com]

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

--- Comment #3 from Geoff Romer <gromer at google dot com> ---
What's the standard of review here? If we can only assert on undefined
behavior, even in debug mode, then this just can't be done (although maybe we
should make this undefined in the Standard). If we can assert on behavior
that's technically well-defined, but very likely to lead to undefined behavior,
and very far from accepted usage norms, then I think this is OK (even for
custom deleters). The kinds of uses you're concerned about (e.g. using
p.reset(p.get()) to invoke some operation on *p) seem really marginal to me.

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think all existing Debug Mode checks only trigger for genuine undefined
behaviour

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[ppluzhnikov at google dot com]

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

Paul Pluzhnikov <ppluzhnikov at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ppluzhnikov at google dot com

--- Comment #5 from Paul Pluzhnikov <ppluzhnikov at google dot com> ---
Google ref b/6572217

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[gromer at google dot com]

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

--- Comment #6 from Geoff Romer <gromer at google dot com> ---
A Chromium maintainer privately pointed out a use case that would be thwarted
by a check like this: basically, unique_ptr is used to hold pointers from a
legacy API, using a custom deleter that decrements a reference count rather
than actually destroying the object. If the legacy API returns the same pointer
from multiple calls, this can lead to reset() being legitimately called with a
pointer equal to the stored value.

So on reflection, I agree this can only be done for default_delete.

[Bug libstdc++/58159] unique_ptr::reset should have debug assertion for "self-reset"

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-09-18
     Ever confirmed|0                           |1

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Confirming as a valid enhancement (for the default_delete case only).