libgcc: fix SEH C++ rethrow semantics [PR113337]

  SEH _Unwind_Resume_or_Rethrow invokes abort directly if
_Unwind_RaiseException doesn't manage to find a handler for the rethrown
exception; this is incorrect, as in this case std::terminate should be
invoked, allowing an application-provided terminate handler to handle
the situation instead of straight crashing the application through
abort.

The bug can be demonstrated with this simple test case:
===
static void custom_terminate_handler() {
    fprintf(stderr, "custom_terminate_handler invoked\n");
    std::exit(1);
}

int main(int argc, char *argv[]) {
    std::set_terminate(&custom_terminate_handler);
    if (argc < 2) return 1;
    const char *mode = argv[1];
    fprintf(stderr, "%s\n", mode);
    if (strcmp(mode, "throw") == 0) {
        throw std::exception();
    } else if (strcmp(mode, "rethrow") == 0) {
        try {
            throw std::exception();
        } catch (...) {
            throw;
        }
    } else {
        return 1;
    }
    return 0;
}
===

On all gcc builds with non-SEH exceptions, this will print
"custom_terminate_handler invoked" both if launched as ./a.out throw or
as ./a.out rethrow, on SEH builds instead if will work as expected only
with ./a.exe throw, but will crash with the "built-in" abort message
with ./a.exe rethrow.

This patch fixes the problem, forwarding back the error code to the
caller (__cxa_rethrow), that calls std::terminate if
_Unwind_Resume_or_Rethrow returns.

The change makes the code path coherent with SEH _Unwind_RaiseException,
and with the generic _Unwind_Resume_or_Rethrow from libgcc/unwind.inc
(used for SjLj and Dw2 exception backend).

libgcc/ChangeLog:

        * unwind-seh.c (_Unwind_Resume_or_Rethrow): forward
        _Unwind_RaiseException return code back to caller instead of
        calling abort, allowing __cxa_rethrow to invoke std::terminate
        in case of uncaught rethrown exception
---
 libgcc/unwind-seh.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Ping! That's a one-line fix, and you can find all the details in the 
bugzilla entry. Also, I can provide executables built with the affected 
toolchains, demonstrating the problem and the fix.

Thanks,
Matteo

Il 17/01/24 12:51, Matteo Italia ha scritto:
> SEH _Unwind_Resume_or_Rethrow invokes abort directly if
> _Unwind_RaiseException doesn't manage to find a handler for the rethrown
> exception; this is incorrect, as in this case std::terminate should be
> invoked, allowing an application-provided terminate handler to handle
> the situation instead of straight crashing the application through
> abort.
>
> The bug can be demonstrated with this simple test case:
> ===
> static void custom_terminate_handler() {
>      fprintf(stderr, "custom_terminate_handler invoked\n");
>      std::exit(1);
> }
>
> int main(int argc, char *argv[]) {
>      std::set_terminate(&custom_terminate_handler);
>      if (argc < 2) return 1;
>      const char *mode = argv[1];
>      fprintf(stderr, "%s\n", mode);
>      if (strcmp(mode, "throw") == 0) {
>          throw std::exception();
>      } else if (strcmp(mode, "rethrow") == 0) {
>          try {
>              throw std::exception();
>          } catch (...) {
>              throw;
>          }
>      } else {
>          return 1;
>      }
>      return 0;
> }
> ===
>
> On all gcc builds with non-SEH exceptions, this will print
> "custom_terminate_handler invoked" both if launched as ./a.out throw or
> as ./a.out rethrow, on SEH builds instead if will work as expected only
> with ./a.exe throw, but will crash with the "built-in" abort message
> with ./a.exe rethrow.
>
> This patch fixes the problem, forwarding back the error code to the
> caller (__cxa_rethrow), that calls std::terminate if
> _Unwind_Resume_or_Rethrow returns.
>
> The change makes the code path coherent with SEH _Unwind_RaiseException,
> and with the generic _Unwind_Resume_or_Rethrow from libgcc/unwind.inc
> (used for SjLj and Dw2 exception backend).
>
> libgcc/ChangeLog:
>
>          * unwind-seh.c (_Unwind_Resume_or_Rethrow): forward
>          _Unwind_RaiseException return code back to caller instead of
>          calling abort, allowing __cxa_rethrow to invoke std::terminate
>          in case of uncaught rethrown exception
> ---
>   libgcc/unwind-seh.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libgcc/unwind-seh.c b/libgcc/unwind-seh.c
> index 8ef0257b616..f1b8f5a8519 100644
> --- a/libgcc/unwind-seh.c
> +++ b/libgcc/unwind-seh.c
> @@ -395,9 +395,9 @@ _Unwind_Reason_Code
>   _Unwind_Resume_or_Rethrow (struct _Unwind_Exception *exc)
>   {
>     if (exc->private_[0] == 0)
> -    _Unwind_RaiseException (exc);
> -  else
> -    _Unwind_ForcedUnwind_Phase2 (exc);
> +    return _Unwind_RaiseException (exc);
> +
> +  _Unwind_ForcedUnwind_Phase2 (exc);
>     abort ();
>   }
>
  

On 1/24/24 15:17, Matteo Italia wrote:
> Ping! That's a one-line fix, and you can find all the details in the 
> bugzilla entry. Also, I can provide executables built with the affected 
> toolchains, demonstrating the problem and the fix.
> 
> Thanks,
> Matteo
> 

I was away last week. LH, care to comment? Changes look fine to me.

> Il 17/01/24 12:51, Matteo Italia ha scritto:
>> SEH _Unwind_Resume_or_Rethrow invokes abort directly if
>> _Unwind_RaiseException doesn't manage to find a handler for the rethrown
>> exception; this is incorrect, as in this case std::terminate should be
>> invoked, allowing an application-provided terminate handler to handle
>> the situation instead of straight crashing the application through
>> abort.
>>
>> The bug can be demonstrated with this simple test case:
>> ===
>> static void custom_terminate_handler() {
>>      fprintf(stderr, "custom_terminate_handler invoked\n");
>>      std::exit(1);
>> }
>>
>> int main(int argc, char *argv[]) {
>>      std::set_terminate(&custom_terminate_handler);
>>      if (argc < 2) return 1;
>>      const char *mode = argv[1];
>>      fprintf(stderr, "%s\n", mode);
>>      if (strcmp(mode, "throw") == 0) {
>>          throw std::exception();
>>      } else if (strcmp(mode, "rethrow") == 0) {
>>          try {
>>              throw std::exception();
>>          } catch (...) {
>>              throw;
>>          }
>>      } else {
>>          return 1;
>>      }
>>      return 0;
>> }
>> ===
>>
>> On all gcc builds with non-SEH exceptions, this will print
>> "custom_terminate_handler invoked" both if launched as ./a.out throw or
>> as ./a.out rethrow, on SEH builds instead if will work as expected only
>> with ./a.exe throw, but will crash with the "built-in" abort message
>> with ./a.exe rethrow.
>>
>> This patch fixes the problem, forwarding back the error code to the
>> caller (__cxa_rethrow), that calls std::terminate if
>> _Unwind_Resume_or_Rethrow returns.
>>
>> The change makes the code path coherent with SEH _Unwind_RaiseException,
>> and with the generic _Unwind_Resume_or_Rethrow from libgcc/unwind.inc
>> (used for SjLj and Dw2 exception backend).
>>
>> libgcc/ChangeLog:
>>
>>          * unwind-seh.c (_Unwind_Resume_or_Rethrow): forward
>>          _Unwind_RaiseException return code back to caller instead of
>>          calling abort, allowing __cxa_rethrow to invoke std::terminate
>>          in case of uncaught rethrown exception
>> ---
>>   libgcc/unwind-seh.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/libgcc/unwind-seh.c b/libgcc/unwind-seh.c
>> index 8ef0257b616..f1b8f5a8519 100644
>> --- a/libgcc/unwind-seh.c
>> +++ b/libgcc/unwind-seh.c
>> @@ -395,9 +395,9 @@ _Unwind_Reason_Code
>>   _Unwind_Resume_or_Rethrow (struct _Unwind_Exception *exc)
>>   {
>>     if (exc->private_[0] == 0)
>> -    _Unwind_RaiseException (exc);
>> -  else
>> -    _Unwind_ForcedUnwind_Phase2 (exc);
>> +    return _Unwind_RaiseException (exc);
>> +
>> +  _Unwind_ForcedUnwind_Phase2 (exc);
>>     abort ();
>>   }
  

在 2024-01-31 08:08, Jonathan Yong 写道:
> On 1/24/24 15:17, Matteo Italia wrote:
>> Ping! That's a one-line fix, and you can find all the details in the bugzilla entry. Also, I can 
>> provide executables built with the affected toolchains, demonstrating the problem and the fix.
>>
>> Thanks,
>> Matteo
>>
> 
> I was away last week. LH, care to comment? Changes look fine to me.
> 

The change looks good to me, too.

I haven't tested it though. According to a similar construction around 'libgcc/unwind.inc:265' it 
should be that way.



-- 
Best regards,
LIU Hao
  

Il 31/01/24 04:24, LIU Hao ha scritto:
> 在 2024-01-31 08:08, Jonathan Yong 写道:
>> On 1/24/24 15:17, Matteo Italia wrote:
>>> Ping! That's a one-line fix, and you can find all the details in the 
>>> bugzilla entry. Also, I can provide executables built with the 
>>> affected toolchains, demonstrating the problem and the fix.
>>>
>>> Thanks,
>>> Matteo
>>>
>>
>> I was away last week. LH, care to comment? Changes look fine to me.
>>
>
> The change looks good to me, too.
>
> I haven't tested it though. According to a similar construction around 
> 'libgcc/unwind.inc:265' it should be that way.

Hello,

thank you for the replies, is there anything else I can do to help push 
this forward?
  

On Mon, Feb 5, 2024, 06:53 Matteo Italia <matteo@mitalia.net> wrote:

> Il 31/01/24 04:24, LIU Hao ha scritto:
> > 在 2024-01-31 08:08, Jonathan Yong 写道:
> >> On 1/24/24 15:17, Matteo Italia wrote:
> >>> Ping! That's a one-line fix, and you can find all the details in the
> >>> bugzilla entry. Also, I can provide executables built with the
> >>> affected toolchains, demonstrating the problem and the fix.
> >>>
> >>> Thanks,
> >>> Matteo
> >>>
> >>
> >> I was away last week. LH, care to comment? Changes look fine to me.
> >>
> >
> > The change looks good to me, too.
> >
> > I haven't tested it though. According to a similar construction around
> > 'libgcc/unwind.inc:265' it should be that way.
>
> Hello,
>
> thank you for the replies, is there anything else I can do to help push
> this forward?
>

Remember to mention the pr with the right syntax in the ChangeLog so the
bot adds a comment field. I didn't see it in yours, but I might have missed
it.

>
  

On 2/6/24 05:31, NightStrike wrote:
> On Mon, Feb 5, 2024, 06:53 Matteo Italia <matteo@mitalia.net> wrote:
> 
>> Il 31/01/24 04:24, LIU Hao ha scritto:
>>> 在 2024-01-31 08:08, Jonathan Yong 写道:
>>>> On 1/24/24 15:17, Matteo Italia wrote:
>>>>> Ping! That's a one-line fix, and you can find all the details in the
>>>>> bugzilla entry. Also, I can provide executables built with the
>>>>> affected toolchains, demonstrating the problem and the fix.
>>>>>
>>>>> Thanks,
>>>>> Matteo
>>>>>
>>>>
>>>> I was away last week. LH, care to comment? Changes look fine to me.
>>>>
>>>
>>> The change looks good to me, too.
>>>
>>> I haven't tested it though. According to a similar construction around
>>> 'libgcc/unwind.inc:265' it should be that way.
>>
>> Hello,
>>
>> thank you for the replies, is there anything else I can do to help push
>> this forward?
>>
> 
> Remember to mention the pr with the right syntax in the ChangeLog so the
> bot adds a comment field. I didn't see it in yours, but I might have missed
> it.
> 
>>
> 

Thanks all, pushed to master branch.
  

Il 06/02/24 10:17, Jonathan Yong ha scritto:
> On 2/6/24 05:31, NightStrike wrote:
>> On Mon, Feb 5, 2024, 06:53 Matteo Italia <matteo@mitalia.net> wrote:
>>
>>> Il 31/01/24 04:24, LIU Hao ha scritto:
>>>> 在 2024-01-31 08:08, Jonathan Yong 写道:
>>>>> On 1/24/24 15:17, Matteo Italia wrote:
>>>>>> Ping! That's a one-line fix, and you can find all the details in the
>>>>>> bugzilla entry. Also, I can provide executables built with the
>>>>>> affected toolchains, demonstrating the problem and the fix.
>>>>>>
>>>>>> Thanks,
>>>>>> Matteo
>>>>>>
>>>>>
>>>>> I was away last week. LH, care to comment? Changes look fine to me.
>>>>>
>>>>
>>>> The change looks good to me, too.
>>>>
>>>> I haven't tested it though. According to a similar construction around
>>>> 'libgcc/unwind.inc:265' it should be that way.
>>>
>>> Hello,
>>>
>>> thank you for the replies, is there anything else I can do to help push
>>> this forward?
>>>
>>
>> Remember to mention the pr with the right syntax in the ChangeLog so the
>> bot adds a comment field. I didn't see it in yours, but I might have 
>> missed
>> it.
>>
>>>
>>
>
> Thanks all, pushed to master branch.

Thanks all :-) do you think this warrants backports? On one hand this is 
a pretty niche feature, and I am probably the first to notice the 
problem in ~12 years since that code was written, OTOH Win64/SEH was not 
super widespread for a long time, and seems like a safe enough change.

Also: should I explicitly mark PR113337 as resolved? The bot added the 
reference to the commit, but the PR is still marked as "UNCONFIRMED".
  

On Wed, Feb 7, 2024 at 4:23 AM Matteo Italia <matteo@mitalia.net> wrote:
>
> Il 06/02/24 10:17, Jonathan Yong ha scritto:
> > On 2/6/24 05:31, NightStrike wrote:
> >> On Mon, Feb 5, 2024, 06:53 Matteo Italia <matteo@mitalia.net> wrote:
> >>
> >>> Il 31/01/24 04:24, LIU Hao ha scritto:
> >>>> 在 2024-01-31 08:08, Jonathan Yong 写道:
> >>>>> On 1/24/24 15:17, Matteo Italia wrote:
> >>>>>> Ping! That's a one-line fix, and you can find all the details in the
> >>>>>> bugzilla entry. Also, I can provide executables built with the
> >>>>>> affected toolchains, demonstrating the problem and the fix.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Matteo
> >>>>>>
> >>>>>
> >>>>> I was away last week. LH, care to comment? Changes look fine to me.
> >>>>>
> >>>>
> >>>> The change looks good to me, too.
> >>>>
> >>>> I haven't tested it though. According to a similar construction around
> >>>> 'libgcc/unwind.inc:265' it should be that way.
> >>>
> >>> Hello,
> >>>
> >>> thank you for the replies, is there anything else I can do to help push
> >>> this forward?
> >>>
> >>
> >> Remember to mention the pr with the right syntax in the ChangeLog so the
> >> bot adds a comment field. I didn't see it in yours, but I might have
> >> missed
> >> it.
> >>
> >>>
> >>
> >
> > Thanks all, pushed to master branch.
>
> Thanks all :-) do you think this warrants backports? On one hand this is
> a pretty niche feature, and I am probably the first to notice the
> problem in ~12 years since that code was written, OTOH Win64/SEH was not
> super widespread for a long time, and seems like a safe enough change.

It's mostly up to you whether you want to make the patch and test it.

> Also: should I explicitly mark PR113337 as resolved? The bot added the
> reference to the commit, but the PR is still marked as "UNCONFIRMED".

Looks like Jon did that a few days ago.
  

Il 26/02/24 02:41, NightStrike ha scritto:
> It's mostly up to you whether you want to make the patch and test it.

I mean, the whole file has no code modifications since bd6ecbe48ada 
(2020), and that specific function is the same since it was first 
committed (bf1431e3596b, from 2012). I don't think I should make a 
separate patch for backports: the one I originally posted cherry-picks 
cleanly even to releases/gcc-4.8.0 (first release containing 
bf1431e3596b, at least according to git tag --contains), and the caller 
code is just the same as well, so I expect that technically it could be 
applied pretty much up to there without any modification.

Now, having a look at https://gcc.gnu.org/releases.html I seem to 
understand that the current "active" major releases are 11, 12 and 13. I 
would surely backport to 13, especially given that the patch was 
developed and tested on that release in first place. 11 and 12 would be 
nice too, although I made no explicit tests there; a quick check with

git diff releases/gcc-11.1.0 origin/master -- libgcc/unwind.inc 
libgcc/unwind-seh.c libstdc++-v3/libsupc++/eh_throw.cc

doesn't show any difference that is relevant for my patch. Still, if I 
find some time for that I could compile these patched releases and see 
if the patch still works correctly for extra caution.

Matteo