[PATCH] libstdc++: fix pointer type exception catch [PR105387]

[PATCH] libstdc++: fix pointer type exception catch [PR105387]

[Jakob Hasse]

[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]

This is a patch for the bug 105387 reported in bugzilla: 105387 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105387. This report should contain all the necessary information about the issue. But the patch there was just a preliminary one.
I created a proper patch with the fix and a test case, based on TAG releases/gcc-11.2.0 (7ca388565af176bd4efd4f8db1e5e9e11e98ef45). The changelog is part of the commit message in the patch.

# TESTS
First, I tested only the test case with and without the actual fix, gcc configuration:  ../configure -prefix=/usr --enable-languages=c,c++ --enable-multiarch --host=x86_64-linux-gnu --build=x86_64-linux-gnu --target=x86_64-linux-gnu --enable-cxx-flags=-fno-rtti
		=== libstdc++ Summary ===

# of expected passes		1
# of unexpected failures	1

With the fix:
		=== libstdc++ Summary ===

# of expected passes		2

Then, I tested without RTTI. The gcc configuration was: ../configure -prefix=/usr --enable-languages=c,c++ --enable-multiarch --host=x86_64-linux-gnu --build=x86_64-linux-gnu --target=x86_64-linux-gnu --enable-cxx-flags=-fno-rtti
Without the patch:
		=== libstdc++ Summary ===

# of expected passes		14006
# of unexpected failures	114
# of expected failures		104
# of unresolved testcases	84
# of unsupported tests		663

With the patch:
		=== libstdc++ Summary ===

# of expected passes		14007
# of unexpected failures	113
# of expected failures		104
# of unresolved testcases	84
# of unsupported tests		664

Number of unexpected failures went down by one and the number of unsupported tests went  up by one. 17_intro/headers/c++1998/49745.cc suddenly passes, 22_locale/time_get/get_date/wchar_t/4.cc is suddenly unsupported. I don't know why.

I also tested the entire libstdc++ test suite with RTTI enabled, i.e.: ../configure -prefix=/usr --enable-languages=c,c++ --enable-multiarch --host=x86_64-linux-gnu --build=x86_64-linux-gnu --target=x86_64-linux-gnu --enable-cxx-flags=-frtti

Without patch:
		=== libstdc++ Summary ===

# of expected passes		14201
# of unexpected failures	3
# of expected failures		104
# of unsupported tests		663

With patch:
		=== libstdc++ Summary ===

# of expected passes		14202
# of unexpected failures	2
# of expected failures		104
# of unsupported tests		664

Same symptoms as when running the entire libstdc++ test suite without RTTI.
I'm not sure why the tests behave different, they seem unrelated.

I would be happy to receive feedback, in particular if parts have to be improved or additional information is necessary. I'm completely new to gcc, I'd appreciate concrete recommendations what to do.

Thanks,
Jakob Hasse

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libstdc-v3-Check-for-ptr-in-__pbase_type_info-__do_c.patch --]
[-- Type: text/x-patch; name="0001-libstdc-v3-Check-for-ptr-in-__pbase_type_info-__do_c.patch", Size: 4386 bytes --]

From c258b231c0737ac02ddba88ab0e54a96ab9c04b4 Mon Sep 17 00:00:00 2001
From: Jakob Hasse <0xjakob@users.noreply.github.com>
Date: Tue, 26 Apr 2022 12:03:47 +0800
Subject: [PATCH] libstdc++-v3: Check for ptr in
 __pbase_type_info::__do_catch() [105387]

PR libstdc++/105387

__pbase_type_info::__do_catch(), used to catch pointer type exceptions,
did not check if the type info object to compare against is a pointer
type info object before doing a static down-cast to a pointer type info
object. Since a pointer type info object has additional fields, they would
end up being undefined if the actual type info object was not a pointer
type info object.

A simple check has been added before the down-cast happens.

In case RTTI is enabled, this does not seem to be a problem because
RTTI-based checks would run before and prevent running into the bad
down-cast. However, since the check is very simple and I'm not 100% sure
about the RTTI-case, it has been left for both cases (RTTI and no-RTTI).

libstdc++-v3/ChangeLog:

	* libsupc++/pbase_type_info.cc (__do_catch):
	* testsuite/18_support/105387.cc: New test.
---
 libstdc++-v3/libsupc++/pbase_type_info.cc   |  5 ++-
 libstdc++-v3/testsuite/18_support/105387.cc | 46 +++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 libstdc++-v3/testsuite/18_support/105387.cc

diff --git a/libstdc++-v3/libsupc++/pbase_type_info.cc b/libstdc++-v3/libsupc++/pbase_type_info.cc
index ed1ad3da98a..d06dc11e460 100644
--- a/libstdc++-v3/libsupc++/pbase_type_info.cc
+++ b/libstdc++-v3/libsupc++/pbase_type_info.cc
@@ -74,7 +74,10 @@ __do_catch (const type_info *thr_type,
     // Therefore there must at least be a qualification conversion involved
     // But for that to be valid, our outer pointers must be const qualified.
     return false;
-  
+
+  if (!thr_type->__is_pointer_p ())
+    return false;
+
   const __pbase_type_info *thrown_type =
     static_cast <const __pbase_type_info *> (thr_type);
 
diff --git a/libstdc++-v3/testsuite/18_support/105387.cc b/libstdc++-v3/testsuite/18_support/105387.cc
new file mode 100644
index 00000000000..b099814847c
--- /dev/null
+++ b/libstdc++-v3/testsuite/18_support/105387.cc
@@ -0,0 +1,46 @@
+// Copyright (C) 2022 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <stdexcept>
+#include <cxxabi.h>
+#include <testsuite_hooks.h>
+
+// This test case checks that __pbase_type_info::__do_catch() behaves 
+// correctly when called with a non-pointer type info object as argument.
+// In particular, __pbase_type_info::__do_catch() should not cast
+// the given type object into a pointer type and try to access the
+// extended fields.
+int main(int argc, char **argv) 
+{
+  // Create a zero-initialized buffer for allocation of the type object
+  uint8_t buffer [sizeof(__cxxabiv1::__fundamental_type_info) * 2] = {};
+
+  // Use placement-new to create the fundamental type info object in the
+  // first half of the buffer. Whenever that type info object will be
+  // casted to a pointer type info object, the extended fields of the
+  // pointer type info object will be in the second half of the buffer
+  // and hence be guaranteed zero.
+  __cxxabiv1::__fundamental_type_info *p_fund_info = 
+            new(buffer) __cxxabiv1::__fundamental_type_info("fund_type");
+
+  __cxxabiv1::__pointer_type_info ptr_info("ptr_type", 0, p_fund_info);
+
+  // __do_catch is declared protected in __pointer_type_info, but public in
+  // type_info, so we upcast it here
+  std::type_info *abstract_ptr_info = static_cast<std::type_info*>(&ptr_info);
+  VERIFY(abstract_ptr_info->__do_catch(p_fund_info, nullptr, 1) == false);
+}
-- 
2.25.1

Re: [PATCH] libstdc++: fix pointer type exception catch [PR105387]

[Jonathan Wakely]

On Tue, 3 May 2022 at 11:57, Jakob Hasse via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
> This is a patch for the bug 105387 reported in bugzilla: 105387 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105387. This report should contain all the necessary information about the issue. But the patch there was just a preliminary one.
> I created a proper patch with the fix and a test case, based on TAG releases/gcc-11.2.0 (7ca388565af176bd4efd4f8db1e5e9e11e98ef45). The changelog is part of the commit message in the patch.

Thanks for the patch!

Some boring administrative comments:

In the summary line of the git commit message:
- The component should be libstdc++ not libstdc++-v3.
- The PR number should include "PR" i.e. [PR105387].
Your email Subject: gets this right, but the patch doesn't.
This is (not very well) documented at
https://gcc.gnu.org/contribute.html#patches

Your new testcase has a FSF copyright notice. Unless you have already
completed the paperwork to assign your work (either for just this
change, or this and all future changes) to the FSF then you need to
either do that legal paperwork, or alternatively contribute under the
DCO terms without assigning copyright. See
https://gcc.gnu.org/contribute.html#legal
For simplicity and to expedite the process, I suggest just removing
the copyright notice and license notice from the testcase, and adding
a Signed-off-by: tag to the commit message, as per
https://gcc.gnu.org/dco.html

As for the actual code ...

The testcase has unused parameters for the 'main' function:
+int main(int argc, char **argv)
That should be just 'int main()' because the parameters aren't used.

The testcase should mention the PR, e.g.
// PR libstdc++/105387

I'd prefer if the test contains the original reproducer from the PR,
which doesn't rely on internal details like __pointer_type_info. You
can put the portable test from the PR into one function and the
non-portable test into another function, and call them both from
main().

I'd prefer using unsigned char for the buffer, rather than uint8_t
(which requires the <stdint.h> header, and might be a typedef for
'int' on 8-bit targets, rather than a character type).

The test uses nullptr which is not available in C++98. It should
either require c++11 so it's UNSUPPORTED for c++98 mode:
// { dg-do run { target c++11 } }
or just use 0 instead of nullptr.



> Number of unexpected failures went down by one and the number of unsupported tests went  up by one. 17_intro/headers/c++1998/49745.cc suddenly passes, 22_locale/time_get/get_date/wchar_t/4.cc is suddenly unsupported. I don't know why.

I don't understand that either. I'll try to reproduce that.

Re: [PATCH] libstdc++: fix pointer type exception catch [PR105387]

[Jonathan Wakely]

On Wed, 4 May 2022 at 12:14, Jonathan Wakely wrote:
>
> On Tue, 3 May 2022 at 11:57, Jakob Hasse via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
> >
> > This is a patch for the bug 105387 reported in bugzilla: 105387 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105387. This report should contain all the necessary information about the issue. But the patch there was just a preliminary one.
> > I created a proper patch with the fix and a test case, based on TAG releases/gcc-11.2.0 (7ca388565af176bd4efd4f8db1e5e9e11e98ef45). The changelog is part of the commit message in the patch.
>
> Thanks for the patch!
>
> Some boring administrative comments:
>
> In the summary line of the git commit message:
> - The component should be libstdc++ not libstdc++-v3.
> - The PR number should include "PR" i.e. [PR105387].
> Your email Subject: gets this right, but the patch doesn't.
> This is (not very well) documented at
> https://gcc.gnu.org/contribute.html#patches

Oh, and libstdc++ patches need to be CC'd to both the libstdc++ list
*and* the gcc-patches list, as per
https://gcc.gnu.org/lists.html


>
> Your new testcase has a FSF copyright notice. Unless you have already
> completed the paperwork to assign your work (either for just this
> change, or this and all future changes) to the FSF then you need to
> either do that legal paperwork, or alternatively contribute under the
> DCO terms without assigning copyright. See
> https://gcc.gnu.org/contribute.html#legal
> For simplicity and to expedite the process, I suggest just removing
> the copyright notice and license notice from the testcase, and adding
> a Signed-off-by: tag to the commit message, as per
> https://gcc.gnu.org/dco.html
>
> As for the actual code ...
>
> The testcase has unused parameters for the 'main' function:
> +int main(int argc, char **argv)
> That should be just 'int main()' because the parameters aren't used.
>
> The testcase should mention the PR, e.g.
> // PR libstdc++/105387
>
> I'd prefer if the test contains the original reproducer from the PR,
> which doesn't rely on internal details like __pointer_type_info. You
> can put the portable test from the PR into one function and the
> non-portable test into another function, and call them both from
> main().
>
> I'd prefer using unsigned char for the buffer, rather than uint8_t
> (which requires the <stdint.h> header, and might be a typedef for
> 'int' on 8-bit targets, rather than a character type).
>
> The test uses nullptr which is not available in C++98. It should
> either require c++11 so it's UNSUPPORTED for c++98 mode:
> // { dg-do run { target c++11 } }
> or just use 0 instead of nullptr.
>
>
>
> > Number of unexpected failures went down by one and the number of unsupported tests went  up by one. 17_intro/headers/c++1998/49745.cc suddenly passes, 22_locale/time_get/get_date/wchar_t/4.cc is suddenly unsupported. I don't know why.
>
> I don't understand that either. I'll try to reproduce that.

Re: [PATCH] libstdc++: fix pointer type exception catch [PR105387]

[Jonathan Wakely]

On Wed, 4 May 2022 at 12:14, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 3 May 2022 at 11:57, Jakob Hasse via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
> >
> > This is a patch for the bug 105387 reported in bugzilla: 105387 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105387. This report should contain all the necessary information about the issue. But the patch there was just a preliminary one.
> > I created a proper patch with the fix and a test case, based on TAG releases/gcc-11.2.0 (7ca388565af176bd4efd4f8db1e5e9e11e98ef45). The changelog is part of the commit message in the patch.
>
> Thanks for the patch!
>
> Some boring administrative comments:
>
> In the summary line of the git commit message:
> - The component should be libstdc++ not libstdc++-v3.
> - The PR number should include "PR" i.e. [PR105387].
> Your email Subject: gets this right, but the patch doesn't.
> This is (not very well) documented at
> https://gcc.gnu.org/contribute.html#patches
>
> Your new testcase has a FSF copyright notice. Unless you have already
> completed the paperwork to assign your work (either for just this
> change, or this and all future changes) to the FSF then you need to
> either do that legal paperwork, or alternatively contribute under the
> DCO terms without assigning copyright. See
> https://gcc.gnu.org/contribute.html#legal
> For simplicity and to expedite the process, I suggest just removing
> the copyright notice and license notice from the testcase, and adding
> a Signed-off-by: tag to the commit message, as per
> https://gcc.gnu.org/dco.html
>
> As for the actual code ...

The commit msg says:

"In case RTTI is enabled, this does not seem to be a problem because
RTTI-based checks would run before and prevent running into the bad
down-cast. However, since the check is very simple and I'm not 100% sure
about the RTTI-case, it has been left for both cases (RTTI and no-RTTI)."

I think we don't want to do the redundant __is_pointer_p() call for
the RTTI-enabled case. For that case we know it's a pointer, or we'd
have returned early, so making an extra virtual call is just
unnecessary overhead. So I think your new check should be done in an
#else branch of the existing #if __cpp_rtti block.

More importantly, I think your patch breaks this case, which currently
works with or without RTTI:

struct X { int i; };
try {  throw &X::i; }
catch (const int X::*) { }

For this case __is_pointer_p() is false, but the cast to
__pbase_type_info is safe, and required for correctness. I think with
your patch the exception won't be caught.

Maybe that's a sacrifice we have to make in order to avoid undefined
behaviour for your original example, but it's unfortunate if that's
the case.

Re: [PATCH] libstdc++: fix pointer type exception catch [PR105387]

[Jonathan Wakely]

On Wed, 4 May 2022 at 12:14, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 3 May 2022 at 11:57, Jakob Hasse via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
> > Number of unexpected failures went down by one and the number of unsupported tests went  up by one. 17_intro/headers/c++1998/49745.cc suddenly passes, 22_locale/time_get/get_date/wchar_t/4.cc is suddenly unsupported. I don't know why.
>
> I don't understand that either. I'll try to reproduce that.

I have no idea why you're seeing that, but I have pushed a change to
the master branch that fixes most of the test failures when using
-fno-rtti.

Re: [PATCH] libstdc++: fix pointer type exception catch [PR105387]

[Jakob Hasse]

I'll re-run tests on current master.

About the pointer-to-member issue: You're right, I missed that. I don't know a solution so far. Should I continue preparing the patch anyway, as a temporary solution?

If so, I'll adjust the patch according to the comments earlier (formalities, coding rules, etc.).

Does GCC's libstdc++-v3 have some kind of "errata"?

Thanks,
Jakob
________________________________
From: Jonathan Wakely <jwakely.gcc@gmail.com>
Sent: Friday, May 6, 2022 4:19 AM
To: Jakob Hasse <jakob.hasse@espressif.com>
Cc: libstdc++@gcc.gnu.org <libstdc++@gcc.gnu.org>; Rocha Euripedes <rocha.euripedes@espressif.com>; Ivan Grokhotkov <ivan@espressif.com>
Subject: Re: [PATCH] libstdc++: fix pointer type exception catch [PR105387]

[External: This email originated outside Espressif]

On Wed, 4 May 2022 at 12:14, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 3 May 2022 at 11:57, Jakob Hasse via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
> > Number of unexpected failures went down by one and the number of unsupported tests went  up by one. 17_intro/headers/c++1998/49745.cc suddenly passes, 22_locale/time_get/get_date/wchar_t/4.cc is suddenly unsupported. I don't know why.
>
> I don't understand that either. I'll try to reproduce that.

I have no idea why you're seeing that, but I have pushed a change to
the master branch that fixes most of the test failures when using
-fno-rtti.

Re: [PATCH] libstdc++: fix pointer type exception catch [PR105387]

[Jonathan Wakely]

On Fri, 6 May 2022, 02:43 Jakob Hasse, <jakob.hasse@espressif.com> wrote:

> I'll re-run tests on current master.
>
> About the pointer-to-member issue: You're right, I missed that. I don't
> know a solution so far. Should I continue preparing the patch anyway, as a
> temporary solution?
>

Yes, please do. I think losing the ability to catch pointers to member via
qualification conversions might be a necessary consequence of avoiding
undefined behaviour and crashes in other cases. It only affects people
building libstdc++ without RTTI anyway, which is unusual.


> If so, I'll adjust the patch according to the comments earlier
> (formalities, coding rules, etc.).
>
> Does GCC's libstdc++-v3 have some kind of "errata"?
>

No, bugzilla serves that purpose, or the release notes for larger changes.

doc/xml/manual/evolution.xml documents user-visible API changes, but this
doesn't qualify, it's just a bug fix.

doc/xml/manual/intro.xml documents implemented defects fixed in the C++
standard itself.