[Bug c++/112301] New: Double destruction of returned object when exiting the scope causes an exception which gets rethrown

[Bug c++/112301] New: Double destruction of returned object when exiting the scope causes an exception which gets rethrown

[alexander.grund@tu-dresden.de]

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

            Bug ID: 112301
           Summary: Double destruction of returned object when exiting the
                    scope causes an exception which gets rethrown
           Product: gcc
           Version: 12.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: alexander.grund@tu-dresden.de
  Target Milestone: ---

Created attachment 56476
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56476&action=edit
More complete example with logging pointers

I debugged a heap corruption I traced back to a use-after-free caused by an
extra destructor call.

I suspect the cause could be the fix for
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=33799 where a throwing destructor
led to a missing destructor call.
It could be similar to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=12751 which
also had an extra destructor call generated for an already destructed instance.

Minimized code sample:

#include <stdexcept>
#include <cassert>

int num = 0;
struct ptr{
    ptr(){
        ++num;
    }
    ptr(ptr&&){
        ++num;
    }
    ~ptr(){
        assert(num-- > 0);
    }
};

struct ThrowOnExit{
    ~ThrowOnExit() noexcept(false){
        throw std::runtime_error("");
    }
};

ptr foo(ptr x){
    try{
        ThrowOnExit _;
        return x;
    }catch (const std::exception&) {
        throw;
    }
}

void wrapper(){
    try{
        foo(ptr{});
    }catch(const std::exception&){}
}

int main(){
    wrapper();
}


The assertion fails, although it should not. Logging the constructions and
destructions and removing the assert gives me this:

construct 0x7ffd4538088e
move construct 0x7ffd4538088f
free 0x7ffd4538088f
free 0x7ffd4538088f
free 0x7ffd4538088e

[Bug c++/112301] [Regression 12/13/14] Double destruction of returned object when exiting the scope causes an exception which gets rethrown

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
            Summary|Double destruction of       |[Regression 12/13/14]
                   |returned object when        |Double destruction of
                   |exiting the scope causes an |returned object when
                   |exception which gets        |exiting the scope causes an
                   |rethrown                    |exception which gets
                   |                            |rethrown
                 CC|                            |jason at gcc dot gnu.org
   Last reconfirmed|                            |2023-10-30
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Confirmed as a regression starting with r12-6333-gb10e031458d541

Author: Jason Merrill
Date:   Wed Jan 5 22:01:12 2022

    c++: destroy retval on throwing cleanup in try [PR33799]

    My earlier attempt to fix this bug didn't handle the case where both the
    return and the throwing cleanup are within a try-block that catches and
    discards the exception.  Fixed by adding the retval cleanup to any
    try-blocks as well as the function body.  PR102191 pointed out that we also
    weren't handling templates properly, so I moved the call out of the parser.

            PR c++/33799
            PR c++/102191

[Bug c++/112301] [Regression 12/13/14] Double destruction of returned object when exiting the scope causes an exception which gets rethrown

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jason at gcc dot gnu.org
   Target Milestone|---                         |12.4

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:ae07265381d934ee97fb1ce8915731158c91babc

commit r14-5086-gae07265381d934ee97fb1ce8915731158c91babc
Author: Jason Merrill <jason@redhat.com>
Date:   Mon Oct 30 17:44:54 2023 -0400

    c++: retval dtor on rethrow [PR112301]

    In r12-6333 for PR33799, I fixed the example in [except.ctor]/2.  In that
    testcase, the exception is caught and the function returns again,
    successfully.

    In this testcase, however, the exception is rethrown, and hits two separate
    cleanups: one in the try block and the other in the function body.  So we
    destroy twice an object that was only constructed once.

    Fortunately, the fix for the normal case is easy: we just need to clear the
    "return value constructed by return" flag when we do it the first time.

    This gets more complicated with the named return value optimization, since
    we don't want to destroy the return value while the NRV variable is still
in
    scope.

            PR c++/112301
            PR c++/102191
            PR c++/33799

    gcc/cp/ChangeLog:

            * except.cc (maybe_splice_retval_cleanup): Clear
            current_retval_sentinel when destroying retval.
            * semantics.cc (nrv_data): Add in_nrv_cleanup.
            (finalize_nrv): Set it.
            (finalize_nrv_r): Fix handling of throwing cleanups.

    gcc/testsuite/ChangeLog:

            * g++.dg/eh/return1.C: Add more cases.

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |14.0

--- Comment #3 from Jason Merrill <jason at gcc dot gnu.org> ---
Fixed for 14 so far.

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[alexander.grund@tu-dresden.de]

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

--- Comment #4 from Alexander Grund <alexander.grund@tu-dresden.de> ---
Thank you! Will there be backports for 12.x?

The current patch is conflicting due to "c++: enable NRVO from inner block
[PR51571]" and while it seems to be easy enough to backport both patches in
order I'm not sure about possible side effects due to that especially how some
additionally introduced NRVOs can affect interoperability of programs/libraries
compiled before and after that change.

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jason Merrill
<jason@gcc.gnu.org>:

https://gcc.gnu.org/g:7fae9873a74c7a5a62044bb6a4cde8e3ac1a5e5d

commit r12-9990-g7fae9873a74c7a5a62044bb6a4cde8e3ac1a5e5d
Author: Jason Merrill <jason@redhat.com>
Date:   Mon Oct 30 17:44:54 2023 -0400

    c++: retval dtor on rethrow [PR112301]

    In r12-6333 for PR33799, I fixed the example in [except.ctor]/2.  In that
    testcase, the exception is caught and the function returns again,
    successfully.

    In this testcase, however, the exception is rethrown, and hits two separate
    cleanups: one in the try block and the other in the function body.  So we
    destroy twice an object that was only constructed once.

    Fortunately, the fix for the normal case is easy: we just need to clear the
    "return value constructed by return" flag when we do it the first time.

    This gets more complicated with the named return value optimization, since
    we don't want to destroy the return value while the NRV variable is still
in
    scope.

            PR c++/112301
            PR c++/102191
            PR c++/33799

    gcc/cp/ChangeLog:

            * except.cc (maybe_splice_retval_cleanup): Clear
            current_retval_sentinel when destroying retval.
            * semantics.cc (nrv_data): Add in_nrv_cleanup.
            (finalize_nrv): Set it.
            (finalize_nrv_r): Fix handling of throwing cleanups.

    gcc/testsuite/ChangeLog:

            * g++.dg/eh/return1.C: Add more cases.

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jason Merrill
<jason@gcc.gnu.org>:

https://gcc.gnu.org/g:d237e7b291ff52095d600e6489a54b4ba8aaf608

commit r13-8079-gd237e7b291ff52095d600e6489a54b4ba8aaf608
Author: Jason Merrill <jason@redhat.com>
Date:   Mon Oct 30 17:44:54 2023 -0400

    c++: retval dtor on rethrow [PR112301]

    In r12-6333 for PR33799, I fixed the example in [except.ctor]/2.  In that
    testcase, the exception is caught and the function returns again,
    successfully.

    In this testcase, however, the exception is rethrown, and hits two separate
    cleanups: one in the try block and the other in the function body.  So we
    destroy twice an object that was only constructed once.

    Fortunately, the fix for the normal case is easy: we just need to clear the
    "return value constructed by return" flag when we do it the first time.

    This gets more complicated with the named return value optimization, since
    we don't want to destroy the return value while the NRV variable is still
in
    scope.

            PR c++/112301
            PR c++/102191
            PR c++/33799

    gcc/cp/ChangeLog:

            * except.cc (maybe_splice_retval_cleanup): Clear
            current_retval_sentinel when destroying retval.
            * semantics.cc (nrv_data): Add in_nrv_cleanup.
            (finalize_nrv): Set it.
            (finalize_nrv_r): Fix handling of throwing cleanups.

    gcc/testsuite/ChangeLog:

            * g++.dg/eh/return1.C: Add more cases.

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
This caused PR113347 (did you forget to mark the bug fixed?)

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[pinskia at gcc dot gnu.org]

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

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Alexander Grund from comment #4)
> Thank you! Will there be backports for 12.x?
> 
> The current patch is conflicting due to "c++: enable NRVO from inner block
> [PR51571]" and while it seems to be easy enough to backport both patches in
> order I'm not sure about possible side effects due to that especially how
> some additionally introduced NRVOs can affect interoperability of
> programs/libraries compiled before and after that change.

Hmm ....

[Bug c++/112301] [12/13/14 regression] Double destruction of returned object when exiting the scope causes an exception which gets rethrown since r12-6333-gb10e031458d541

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at gcc dot gnu.org
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #9 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Jason fixed this and backported the fix to the gcc-12 and gcc-13 branches.