libgcc: fix SEH C++ rethrow semantics [PR113337]
Checks
Commit Message
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(-)
Comments
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
@@ -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 ();
}