Re: [PATCH 2/2] Avoid letting exceptions escape gdb_bfd_iovec_fileio_close (PR gdb/28080)

[Pedro Alves <pedro@palves.net>]

On 2021-07-13 2:19 a.m., Simon Marchi wrote:
> 
> On 2021-07-12 3:33 p.m., Pedro Alves wrote:
>> BFD is written in C and thus throwing exceptions over such frames may
>> either not clean up properly, or, may abort if bfd is not compiled
>> with -​fasynchronous-unwind-tables (x86-64 defaults that on, but not
> 
> Weird characters on the line above, just before fasynchronous.

Curious, must be the result of copy/paste from the browser, for I googled the
correct spelling.  They're not visible in emacs nor my Thunderbird, nor in the
archives:
 https://sourceware.org/pipermail/gdb-patches/2021-July/180849.html
but I see them in the raw email source.  Fixed.

> 
>> all GCC ports do).
>>
>> Thus frame #8 sees like a good place to swallow exceptions.  More so
> 
> sees -> seems

Fixed.

> 
>> since in this spot we already close returning error.  That's what this
> 
> I don't understand this sentence, "in this spot we already close
> returning error".

That's because the "ignore" word is missing... :-P

I've tweaked it to:

    Thus frame #8 seems like a good place to swallow exceptions.  More so
    since in this spot we already ignore target_fileio_close return
    errors.  That's what this commit does.  Without the previous fix, we'd
    see:

> 
>> commit does.  Without the previous fix, we'd see:
>>
>>  (gdb) detach
>>  Detaching from program: target:/any/program, process 2197701
>>  Ending remote debugging.
>>  [Inferior 1 (process 2197701) detached]
>>  warning: while closing file: Remote connection closed
>>
>> Note it prints a warning, which would still be a regression compared
>> to GDB 10, if it weren't for the previous fix.
> 
> Maybe the more correct thing to do would be to make
> gdb_bfd_iovec_fileio_close return an error when it can't properly close
> the file.  That error would be communicated as a bfd return status to
> gdb_bfd_close_or_warn, which would warn.

The problem with that is that it would require losing the exception
message, because the warning gdb_bfd_close_or_warn prints is just the
text version of a bfd error:

  if (!ret)
    warning (_("cannot close \"%s\": %s"),
	     name, bfd_errmsg (bfd_get_error ()));

Maybe we could add a new type of bfd error, a custom error, where the error
reason is given by callback (which gdb would install) or a string or some such, so
we could pass the gdb error through bfd.  Not that different from how
bfd_error_on_input is a special error code too.  But I'd rather not do this now.

I've tweaked the patch to ensure that the new warning looks exactly like the
one in gdb_bfd_close_or_warn, though.  See below.

> 
> If we need to catch exceptions for the close operations, I suppose we
> need to do that as well for the other BFD iovec operations: open, pread
> and stat?  A remote communication error could happen in any of those.

Yes, though I don't think they're as likely to be in dtor/noexcept paths,
and it isn't as obvious to me what the return code should be.  With close,
there's not much else we can do other than ignore the error.  I'd rather
not touch them for now, for time constraint reasons.  :-/

If you'd prefer, I can drop this patch for now.  The issue addressed by this
patch isn't a regression.

From 56f2e825c7800eb226a916bc33e3948e2c02fa27 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 12 Jul 2021 18:03:22 +0100
Subject: [PATCH] Avoid letting exceptions escape gdb_bfd_iovec_fileio_close
 (PR gdb/28080)

Before PR gdb/28080 was fixed by the previous patch, GDB was crashing
like this:

 (gdb) detach
 Detaching from program: target:/any/program, process 3671843
 Detaching from process 3671843
 Ending remote debugging.
 [Inferior 1 (process 3671843) detached]
 In main
 terminate called after throwing an instance of 'gdb_exception_error'
 Aborted (core dumped)

Here's the exception above being thrown:

 (top-gdb) bt
 #0  throw_error (error=TARGET_CLOSE_ERROR, fmt=0x555556035588 "Remote connection closed") at src/gdbsupport/common-exceptions.cc:222
 #1  0x0000555555bbaa46 in remote_target::readchar (this=0x555556a11040, timeout=10000) at src/gdb/remote.c:9440
 #2  0x0000555555bbb9e5 in remote_target::getpkt_or_notif_sane_1 (this=0x555556a11040, buf=0x555556a11058, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9928
 #3  0x0000555555bbbda9 in remote_target::getpkt_sane (this=0x555556a11040, buf=0x555556a11058, forever=0) at src/gdb/remote.c:10030
 #4  0x0000555555bc0e75 in remote_target::remote_hostio_send_command (this=0x555556a11040, command_bytes=13, which_packet=14, remote_errno=0x7fffffffcfd0, attachment=0x0, attachment_len=0x0) at src/gdb/remote.c:12137
 #5  0x0000555555bc1b6c in remote_target::remote_hostio_close (this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12455
 #6  0x0000555555bc1bb4 in remote_target::fileio_close (During symbol reading: .debug_line address at offset 0x64f417 is 0 [in module build/gdb/gdb]
 this=0x555556a11040, fd=8, remote_errno=0x7fffffffcfd0) at src/gdb/remote.c:12462
 #7  0x0000555555c9274c in target_fileio_close (fd=3, target_errno=0x7fffffffcfd0) at src/gdb/target.c:3365
 #8  0x000055555595a19d in gdb_bfd_iovec_fileio_close (abfd=0x555556b9f8a0, stream=0x555556b11530) at src/gdb/gdb_bfd.c:439
 #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556b9f8a0) at src/bfd/opncls.c:599
 #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556b9f8a0) at src/bfd/opncls.c:847
 #11 0x0000555555e0a27a in bfd_close (abfd=0x555556b9f8a0) at src/bfd/opncls.c:814
 #12 0x000055555595a9d3 in gdb_bfd_close_or_warn (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:626
 #13 0x000055555595ad29 in gdb_bfd_unref (abfd=0x555556b9f8a0) at src/gdb/gdb_bfd.c:715
 #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573
 #15 0x0000555555ae955a in std::_Sp_counted_ptr<objfile*, (__gnu_cxx::_Lock_policy)2>::_M_dispose (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:377
 #16 0x000055555572b7c8 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x555556c20db0) at /usr/include/c++/9/bits/shared_ptr_base.h:155
 #17 0x00005555557263c3 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x555556bf0588, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:730
 #18 0x0000555555ae745e in std::__shared_ptr<objfile, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr_base.h:1169
 #19 0x0000555555ae747e in std::shared_ptr<objfile>::~shared_ptr (this=0x555556bf0580, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/shared_ptr.h:103
 #20 0x0000555555b1c1dc in __gnu_cxx::new_allocator<std::_List_node<std::shared_ptr<objfile> > >::destroy<std::shared_ptr<objfile> > (this=0x5555564cdd60, __p=0x555556bf0580) at /usr/include/c++/9/ext/new_allocator.h:153
 #21 0x0000555555b1bb1d in std::allocator_traits<std::allocator<std::_List_node<std::shared_ptr<objfile> > > >::destroy<std::shared_ptr<objfile> > (__a=..., __p=0x555556bf0580) at /usr/include/c++/9/bits/alloc_traits.h:497
 #22 0x0000555555b1b73e in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::_M_erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/stl_list.h:1921
 #23 0x0000555555b1afeb in std::__cxx11::list<std::shared_ptr<objfile>, std::allocator<std::shared_ptr<objfile> > >::erase (this=0x5555564cdd60, __position=std::shared_ptr<objfile> (expired, weak count 1) = {get() = 0x555556515540}) at /usr/include/c++/9/bits/list.tcc:158
 #24 0x0000555555b19576 in program_space::remove_objfile (this=0x5555564cdd20, objfile=0x555556515540) at src/gdb/progspace.c:210
 #25 0x0000555555ae4502 in objfile::unlink (this=0x555556515540) at src/gdb/objfiles.c:487
 #26 0x0000555555ae5a12 in objfile_purge_solibs () at src/gdb/objfiles.c:875
 #27 0x0000555555c09686 in no_shared_libraries (ignored=0x0, from_tty=1) at src/gdb/solib.c:1236
 #28 0x00005555559e3f5f in detach_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:2769

Note frame #14:

 #14 0x0000555555ae4730 in objfile::~objfile (this=0x555556515540, __in_chrg=<optimized out>) at src/gdb/objfiles.c:573

That's a dtor, thus noexcept.  That's the reason for the
std::terminate.

The previous patch fixed things such that the exception above isn't
thrown anymore.  However, it's possible that e.g., the remote
connection drops just while a user types "nosharedlibrary", or some
other reason that leads to objfile::~objfile, and then we end up the
same std::terminate problem.

Also notice that frames #9-#11 are BFD frames:

 #9  0x0000555555e09e3f in opncls_bclose (abfd=0x555556bc27e0) at src/bfd/opncls.c:599
 #10 0x0000555555e0a2c7 in bfd_close_all_done (abfd=0x555556bc27e0) at src/bfd/opncls.c:847
 #11 0x0000555555e0a27a in bfd_close (abfd=0x555556bc27e0) at src/bfd/opncls.c:814

BFD is written in C and thus throwing exceptions over such frames may
either not clean up properly, or, may abort if bfd is not compiled
with -fasynchronous-unwind-tables (x86-64 defaults that on, but not
all GCC ports do).

Thus frame #8 seems like a good place to swallow exceptions.  More so
since in this spot we already ignore target_fileio_close return
errors.  That's what this commit does.  Without the previous fix, we'd
see:

 (gdb) detach
 Detaching from program: target:/any/program, process 2197701
 Ending remote debugging.
 [Inferior 1 (process 2197701) detached]
 warning: cannot close "target:/lib64/ld-linux-x86-64.so.2": Remote connection closed

Note it prints a warning, which would still be a regression compared
to GDB 10, if it weren't for the previous fix.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	PR gdb/28080
	* gdb_bfd.c (gdb_bfd_close_warning): New.
	(gdb_bfd_iovec_fileio_close): Wrap target_fileio_close in
	try/catch and print warning on exception.
	(gdb_bfd_close_or_warn): Use gdb_bfd_close_warning.

Change-Id: Ic7a26ddba0a4444e3377b0e7c1c89934a84545d7
---
 gdb/gdb_bfd.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 3312197acd5..312442a466e 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -423,6 +423,15 @@ gdb_bfd_iovec_fileio_pread (struct bfd *abfd, void *stream, void *buf,
   return pos;
 }
 
+/* Warn that it wasn't possible to close a bfd for file NAME, because
+   of REASON.  */
+
+static void
+gdb_bfd_close_warning (const char *name, const char *reason)
+{
+  warning (_("cannot close \"%s\": %s"), name, reason);
+}
+
 /* Wrapper for target_fileio_close suitable for passing as the
    CLOSE_FUNC argument to gdb_bfd_openr_iovec.  */
 
@@ -436,7 +445,16 @@ gdb_bfd_iovec_fileio_close (struct bfd *abfd, void *stream)
 
   /* Ignore errors on close.  These may happen with remote
      targets if the connection has already been torn down.  */
-  target_fileio_close (fd, &target_errno);
+  try
+    {
+      target_fileio_close (fd, &target_errno);
+    }
+  catch (const gdb_exception &ex)
+    {
+      /* Also avoid crossing exceptions over bfd.  */
+      gdb_bfd_close_warning (bfd_get_filename (abfd),
+			     ex.message->c_str ());
+    }
 
   /* Zero means success.  */
   return 0;
@@ -626,8 +644,8 @@ gdb_bfd_close_or_warn (struct bfd *abfd)
   ret = bfd_close (abfd);
 
   if (!ret)
-    warning (_("cannot close \"%s\": %s"),
-	     name, bfd_errmsg (bfd_get_error ()));
+    gdb_bfd_close_warning (name,
+			   bfd_errmsg (bfd_get_error ()));
 
   return ret;
 }

base-commit: 6bbe1a929c69e141e63265c7013fcf7f80bc792e
-- 
2.26.2