libsupc++: Fix UB terminating on foreign exception

  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(-)
  

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?