Message ID | 20240114000534.1775261-1-me@jdemille.com |
---|---|
State | New |
Headers | show |
Series | libsupc++: Fix UB terminating on foreign exception | expand |
On Sat, Jan 13, 2024 at 5:10 PM Julia DeMille <me@jdemille.com> wrote: > > Currently, when std::terminate() is called with a foreign exception > active, since nothing in the path checks whether the exception matches > the `GNUCC++\0` personality, a foreign exception can go into the verbose > terminate handler, and get treated as though it were a C++ exception. > > Reflection is attempted, and boom. UB. This patch should eliminate that > UB. 2 things, changelogs go into the email message rather than directly as part of the patch., Second I wonder if you could add a multiple language testcase using GNU objective-C exceptions as an example. If not directly adding a testcase there, at least a simple test that shows the issue outside of the testsuite? Thanks, Andrew Pinski > > Signed-off-by: Julia DeMille <me@jdemille.com> > --- > libstdc++-v3/ChangeLog | 9 +++++++++ > libstdc++-v3/libsupc++/eh_type.cc | 11 +++++++++++ > libstdc++-v3/libsupc++/vterminate.cc | 25 ++++++++++++++++++++----- > 3 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog > index 36257cc4427..bfef0ed8ef1 100644 > --- a/libstdc++-v3/ChangeLog > +++ b/libstdc++-v3/ChangeLog > @@ -1,3 +1,12 @@ > +2024-01-13 Julia DeMille <me@jdemille.com> > + * libsupc++/eh_type.cc (__cxa_current_exception_type): > + Return NULL if the current exception is not the `GNUCC++\0` > + personality. > + * libsupc++/vterminate.cc: > + Check for both exception header and exception type. If there > + is an exception header, but no exception type, state in termination > + message that a foreign exception was active. > + > 2024-01-13 Jonathan Wakely <jwakely@redhat.com> > > PR libstdc++/107466 > diff --git a/libstdc++-v3/libsupc++/eh_type.cc b/libstdc++-v3/libsupc++/eh_type.cc > index 03c677b7e13..e0824eab4d4 100644 > --- a/libstdc++-v3/libsupc++/eh_type.cc > +++ b/libstdc++-v3/libsupc++/eh_type.cc > @@ -36,9 +36,20 @@ extern "C" > std::type_info *__cxa_current_exception_type () _GLIBCXX_NOTHROW > { > __cxa_eh_globals *globals = __cxa_get_globals (); > + > + if (!globals) > + return 0; > + > __cxa_exception *header = globals->caughtExceptions; > + > if (header) > { > + // It is UB to try and inspect an exception that isn't ours. > + // This extends to attempting to perform run-time reflection, as the ABI > + // is unknown. > + if (!__is_gxx_exception_class (header->unwindHeader.exception_class)) > + return 0; > + > if (__is_dependent_exception (header->unwindHeader.exception_class)) > { > __cxa_dependent_exception *de = > diff --git a/libstdc++-v3/libsupc++/vterminate.cc b/libstdc++-v3/libsupc++/vterminate.cc > index 23deeef5289..f931d951526 100644 > --- a/libstdc++-v3/libsupc++/vterminate.cc > +++ b/libstdc++-v3/libsupc++/vterminate.cc > @@ -25,11 +25,12 @@ > #include <bits/c++config.h> > > #if _GLIBCXX_HOSTED > -#include <cstdlib> > -#include <exception> > +#include "unwind-cxx.h" > #include <bits/exception_defines.h> > +#include <cstdio> > +#include <cstdlib> > #include <cxxabi.h> > -# include <cstdio> > +#include <exception> > > using namespace std; > using namespace abi; > @@ -51,10 +52,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > terminating = true; > > + __cxa_eh_globals *globals = __cxa_get_globals (); > + if (!globals) > + { > + fputs ("terminate called", stderr); > + abort (); > + } > + > // Make sure there was an exception; terminate is also called for an > // attempt to rethrow when there is no suitable exception. > - type_info *t = __cxa_current_exception_type(); > - if (t) > + type_info *t = __cxa_current_exception_type (); > + __cxa_exception *header = globals->caughtExceptions; > + > + if (t && header) > { > // Note that "name" is the mangled name. > char const *name = t->name(); > @@ -89,6 +99,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > #endif > __catch(...) { } > } > + else if (header) > + { > + fputs ("terminate called after a foreign exception was thrown\n", > + stderr); > + } > else > fputs("terminate called without an active exception\n", stderr); > > -- > 2.43.0 >
On 1/13/24 19:17, Andrew Pinski wrote: > 2 things, changelogs go into the email message rather than directly as > part of the patch., Apologies. I have prepared a revised patch, and will send it when applicable. > Second I wonder if you could add a multiple language testcase using > GNU objective-C exceptions as an example. > If not directly adding a testcase there, at least a simple test that > shows the issue outside of the testsuite? I initially discovered this during experimenting with unwinds from a Rust library ("C-unwind" ABI) into a C++ application. I can upload the code I used for that.
On Sun, 14 Jan 2024, 01:36 Julia DeMille, <me@jdemille.com> wrote: > On 1/13/24 19:17, Andrew Pinski wrote: > > 2 things, changelogs go into the email message rather than directly as > > part of the patch., > The reason for this is that the ChangeLog files are auto-generated from the git commit messages, not edited by hand. Patches to those files rarely apply cleanly anyway, because they change so frequently that patches are stale almost immediately. > Apologies. I have prepared a revised patch, and will send it when > applicable. > Thanks. > > Second I wonder if you could add a multiple language testcase using > > GNU objective-C exceptions as an example. > > If not directly adding a testcase there, at least a simple test that > > shows the issue outside of the testsuite? > > I initially discovered this during experimenting with unwinds from a > Rust library ("C-unwind" ABI) into a C++ application. I can upload the > code I used for that. > That would be great thanks. If not obvious, easy instructions for building the test would be helpful for Rust newbs such as myself!
On 2024-01-14 01:52, Jonathan Wakely wrote: > The reason for this is that the ChangeLog files are auto-generated from > the git commit messages, not edited by hand. Patches to those files > rarely apply cleanly anyway, because they change so frequently that > patches are stale almost immediately. Makes sense. I'm new to the GCC mailing lists, so that one was unfamiliar to me. > That would be great thanks. If not obvious, easy instructions for > building the test would be helpful for Rust newbs such as myself! I've actually managed to come up with a much more concise Objective-C demonstration. I've uploaded it at: https://codeberg.org/jdemille/gcc-exception-ub-demo I'm unsure if my patch actually fixes it with this demo -- I need to work out how to use a patched GCC without installing it on my system, but without it breaking from not having things it expects to exist on the system. I'm also going to go make sure that the Objective-C unwind personality is unique, otherwise we could have trouble.
On 2024-01-14 18:51, Julia DeMille wrote: > I'm unsure if my patch actually fixes it with this demo -- I need to > work out how to use a patched GCC without installing it on my system, > but without it breaking from not having things it expects to exist on > the system. I've gotten this to work, and run into an unexpected situation. Something about the personality routine is causing a SIGABRT. Investigating further. > I'm also going to go make sure that the Objective-C unwind personality > is unique, otherwise we could have trouble. Checked this -- it is.
Some more info: On 2024-01-14 21:39, Julia DeMille wrote: > I've gotten this to work, and run into an unexpected situation. > Something about the personality routine is causing a SIGABRT. > Investigating further. This occurs due to an assertion in _Unwind_SetGR. Seemingly, the compiler intrinsic `__builtin_eh_return_data_regno` is doing something it *really* should not. I'm not a compiler developer, and have no clue how to investigate this. This issue does not occur with Rust. Additionally, LLVM's libc++abi manages not only to cleanly handle a Rust panic, but also, through some voodoo magic that took me by surprise, recognize Objective-C exceptions (and provide info on them) in its terminate handler. Perhaps due to Objective-C++? Hell if I know. Thought it was worth mentioning that other implementations *have* gotten this working, though.
I would like to move forward on this patch. Are there any concerns, or just the formatting of the patch, that needs to be addressed?
diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 36257cc4427..bfef0ed8ef1 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,3 +1,12 @@ +2024-01-13 Julia DeMille <me@jdemille.com> + * libsupc++/eh_type.cc (__cxa_current_exception_type): + Return NULL if the current exception is not the `GNUCC++\0` + personality. + * libsupc++/vterminate.cc: + Check for both exception header and exception type. If there + is an exception header, but no exception type, state in termination + message that a foreign exception was active. + 2024-01-13 Jonathan Wakely <jwakely@redhat.com> PR libstdc++/107466 diff --git a/libstdc++-v3/libsupc++/eh_type.cc b/libstdc++-v3/libsupc++/eh_type.cc index 03c677b7e13..e0824eab4d4 100644 --- a/libstdc++-v3/libsupc++/eh_type.cc +++ b/libstdc++-v3/libsupc++/eh_type.cc @@ -36,9 +36,20 @@ extern "C" std::type_info *__cxa_current_exception_type () _GLIBCXX_NOTHROW { __cxa_eh_globals *globals = __cxa_get_globals (); + + if (!globals) + return 0; + __cxa_exception *header = globals->caughtExceptions; + if (header) { + // It is UB to try and inspect an exception that isn't ours. + // This extends to attempting to perform run-time reflection, as the ABI + // is unknown. + if (!__is_gxx_exception_class (header->unwindHeader.exception_class)) + return 0; + if (__is_dependent_exception (header->unwindHeader.exception_class)) { __cxa_dependent_exception *de = diff --git a/libstdc++-v3/libsupc++/vterminate.cc b/libstdc++-v3/libsupc++/vterminate.cc index 23deeef5289..f931d951526 100644 --- a/libstdc++-v3/libsupc++/vterminate.cc +++ b/libstdc++-v3/libsupc++/vterminate.cc @@ -25,11 +25,12 @@ #include <bits/c++config.h> #if _GLIBCXX_HOSTED -#include <cstdlib> -#include <exception> +#include "unwind-cxx.h" #include <bits/exception_defines.h> +#include <cstdio> +#include <cstdlib> #include <cxxabi.h> -# include <cstdio> +#include <exception> using namespace std; using namespace abi; @@ -51,10 +52,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } terminating = true; + __cxa_eh_globals *globals = __cxa_get_globals (); + if (!globals) + { + fputs ("terminate called", stderr); + abort (); + } + // Make sure there was an exception; terminate is also called for an // attempt to rethrow when there is no suitable exception. - type_info *t = __cxa_current_exception_type(); - if (t) + type_info *t = __cxa_current_exception_type (); + __cxa_exception *header = globals->caughtExceptions; + + if (t && header) { // Note that "name" is the mangled name. char const *name = t->name(); @@ -89,6 +99,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #endif __catch(...) { } } + else if (header) + { + fputs ("terminate called after a foreign exception was thrown\n", + stderr); + } else fputs("terminate called without an active exception\n", stderr);
Currently, when std::terminate() is called with a foreign exception active, since nothing in the path checks whether the exception matches the `GNUCC++\0` personality, a foreign exception can go into the verbose terminate handler, and get treated as though it were a C++ exception. Reflection is attempted, and boom. UB. This patch should eliminate that UB. Signed-off-by: Julia DeMille <me@jdemille.com> --- libstdc++-v3/ChangeLog | 9 +++++++++ libstdc++-v3/libsupc++/eh_type.cc | 11 +++++++++++ libstdc++-v3/libsupc++/vterminate.cc | 25 ++++++++++++++++++++----- 3 files changed, 40 insertions(+), 5 deletions(-)