Message ID | 20240117115144.50181-1-matteo@mitalia.net |
---|---|
State | New |
Headers | show |
Series | libgcc: fix SEH C++ rethrow semantics [PR113337] | expand |
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.
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
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 (); }