* [PATCH][gdb/remote] Fix invalid pointer in remote_async_serial_handler
@ 2021-01-07 13:39 Tom de Vries
2021-01-07 15:15 ` Andrew Burgess
0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2021-01-07 13:39 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Simon Marchi
Hi,
On rare occasions, we run into this ERROR/UNRESOLVED on gdb-10-branch:
...
(gdb) PASS: gdb.multi/multi-target.exp: continue: non-stop=on: inferior 2
Remote debugging from host ::1, port 34088^M
Process outputs/gdb.multi/multi-target/multi-target created; pid = 8649^M
monitor exit^M
(gdb) Killing process(es): 8649^M
ERROR: GDB process no longer exists
GDB process exited with wait status 8627 exp14 0 0 CHILDKILLED SIGABRT SIGABRT
UNRESOLVED: gdb.multi/multi-target.exp: continue: non-stop=on: inferior 5
...
A trigger patch makes the crash happen all the time:
...
diff --git a/gdb/remote.c b/gdb/remote.c
index 71f814efb365..53ff8b63a1dc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14161,14 +14161,12 @@ remote_target::is_async_p ()
will be able to delay notifying the client of an event until the
point where an entire packet has been received. */
-static serial_event_ftype remote_async_serial_handler;
-
static void
remote_async_serial_handler (struct serial *scb, void *context)
{
- /* Don't propogate error information up to the client. Instead let
- the client find out about the error by querying the target. */
- inferior_event_handler (INF_REG_EVENT);
+ remote_state *rs = (remote_state *) context;
+
+ mark_async_event_handler (rs->remote_async_inferior_event_token);
}
static void
...
And using -fsanitizer=address we can get a more elaborate error message:
...
==7196==ERROR: AddressSanitizer: heap-use-after-free on address \
0x6170000bf258 at pc 0x000001481755 bp 0x7fff05b20840 sp 0x7fff05b20838
READ of size 8 at 0x6170000bf258 thread T0
#0 0x1481754 in std::_Hashtable<gdbarch*, std::pair<gdbarch* const,
remote_arch_state>, std::allocator<std::pair<gdbarch* const,
remote_arch_state> >, std::__detail::_Select1st, std::equal_to<gdbarch*>,
std::hash<gdbarch*>, std::__detail::_Mod_range_hashing,
std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy,
std::__detail::_Hashtable_traits<false, false, true>
>::_M_bucket_index(unsigned long) const
/usr/include/c++/11/bits/hashtable.h:719
#1 0x147c8ab in std::_Hashtable<gdbarch*, std::pair<gdbarch* const,
remote_arch_state>, std::allocator<std::pair<gdbarch* const,
remote_arch_state> >, std::__detail::_Select1st, std::equal_to<gdbarch*>,
std::hash<gdbarch*>, std::__detail::_Mod_range_hashing,
std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy,
std::__detail::_Hashtable_traits<false, false, true> >::find(gdbarch*
const&) /usr/include/c++/11/bits/hashtable.h:1500
#2 0x147852c in std::unordered_map<gdbarch*, remote_arch_state,
std::hash<gdbarch*>, std::equal_to<gdbarch*>,
std::allocator<std::pair<gdbarch* const, remote_arch_state> >
>::find(gdbarch* const&) /usr/include/c++/11/bits/unordered_map.h:869
#3 0x14306db in remote_state::get_remote_arch_state(gdbarch*)
src/gdb/remote.c:1203
#4 0x14309dc in remote_target::get_remote_state() src/gdb/remote.c:1232
#5 0x1470c08 in remote_async_inferior_event_handler src/gdb/remote.c:14169
#6 0xaa9f6b in check_async_event_handlers() src/gdb/async-event.c:295
#7 0x1e93ab4 in gdb_do_one_event() src/gdbsupport/event-loop.cc:194
#8 0x118f5f9 in start_event_loop src/gdb/main.c:356
#9 0x118f8ed in captured_command_loop src/gdb/main.c:416
#10 0x1192d6a in captured_main src/gdb/main.c:1253
#11 0x1192dfa in gdb_main(captured_main_args*) src/gdb/main.c:1268
#12 0x97b380 in main src/gdb/gdb.c:32
#13 0x7f550c211349 in __libc_start_main ../csu/libc-start.c:308
#14 0x97b199 in _start (build/gdb/gdb+0x97b199)
0x6170000bf258 is located 600 bytes inside of 648-byte region \
[0x6170000bf000,0x6170000bf288)
freed by thread T0 here:
#0 0x7f550f516a57 in operator delete(void*, unsigned long)
(/usr/lib64/libasan.so.6+0xaea57)
#1 0x148b1fe in extended_remote_target::~extended_remote_target()
src/gdb/remote.c:958
#2 0x143b483 in remote_target::close() src/gdb/remote.c:4074
#3 0x16cb90f in target_close(target_ops*) src/gdb/target.c:3230
#4 0x16a2635 in decref_target(target_ops*) src/gdb/target.c:557
#5 0x16a2abb in target_stack::unpush(target_ops*) src/gdb/target.c:645
#6 0x16d01ef in inferior::unpush_target(target_ops*)
src/gdb/inferior.h:356
#7 0x16a2877 in unpush_target(target_ops*) src/gdb/target.c:607
#8 0x16a2adf in unpush_target_and_assert src/gdb/target.c:655
#9 0x16a2c57 in pop_all_targets_at_and_above(strata) src/gdb/target.c:678
#10 0x1442749 in remote_unpush_target src/gdb/remote.c:5522
#11 0x1458c16 in remote_target::readchar(int) src/gdb/remote.c:9137
#12 0x145b25b in remote_target::getpkt_or_notif_sane_1(std::vector<char,
gdb::default_init_allocator<char, std::allocator<char> > >*, int, int,
int*) src/gdb/remote.c:9683
#13 0x145bc9a in remote_target::getpkt_sane(std::vector<char,
gdb::default_init_allocator<char, std::allocator<char> > >*, int)
src/gdb/remote.c:9790
#14 0x145b040 in remote_target::getpkt(std::vector<char,
gdb::default_init_allocator<char, std::allocator<char> > >*, int)
src/gdb/remote.c:9623
#15 0x145780b in remote_target::remote_read_bytes_1(unsigned long,
unsigned char*, unsigned long, int, unsigned long*) src/gdb/remote.c:8860
#16 0x145805e in remote_target::remote_read_bytes(unsigned long,
unsigned char*, unsigned long, int, unsigned long*) src/gdb/remote.c:8987
#17 0x146113a in remote_target::xfer_partial(target_object, char const*,
unsigned char*, unsigned char const*, unsigned long, unsigned long,
unsigned long*) src/gdb/remote.c:10987
#18 0x16a4004 in raw_memory_xfer_partial(target_ops*, unsigned char*,
unsigned char const*, unsigned long, long, unsigned long*)
src/gdb/target.c:918
#19 0x16a4fcf in target_xfer_partial(target_ops*, target_object, char
const*, unsigned char*, unsigned char const*, unsigned long, unsigned
long, unsigned long*) src/gdb/target.c:1156
#20 0x16a5d65 in target_read_partial src/gdb/target.c:1387
#21 0x16a5f19 in target_read(target_ops*, target_object, char const*,
unsigned char*, unsigned long, long) src/gdb/target.c:1427
#22 0x16a5666 in target_read_raw_memory(unsigned long, unsigned char*,
long) src/gdb/target.c:1260
#23 0xd22f2a in dcache_read_line src/gdb/dcache.c:336
#24 0xd232b7 in dcache_peek_byte src/gdb/dcache.c:403
#25 0xd23845 in dcache_read_memory_partial(target_ops*, dcache_struct*,
unsigned long, unsigned char*, unsigned long, unsigned long*) src/gdb/dcache.c:484
#26 0x16a47da in memory_xfer_partial_1 src/gdb/target.c:1041
#27 0x16a4a1e in memory_xfer_partial src/gdb/target.c:1084
#28 0x16a4f44 in target_xfer_partial(target_ops*, target_object,
char const*, unsigned char*, unsigned char const*, unsigned long,
unsigned long, unsigned long*) src/gdb/target.c:1141
#29 0x18203d4 in read_value_memory(value*, long, int, unsigned long,
unsigned char*, unsigned long) src/gdb/valops.c:956
previously allocated by thread T0 here:
#0 0x7f550f515c37 in operator new(unsigned long)
(/usr/lib64/libasan.so.6+0xadc37)
#1 0x14429f0 in remote_target::open_1(char const*, int, int)
src/gdb/remote.c:5562
#2 0x14405e6 in extended_remote_target::open(char const*, int)
src/gdb/remote.c:4907
#3 0x16a0f3c in open_target src/gdb/target.c:242
#4 0xc19ff5 in do_sfunc src/gdb/cli/cli-decode.c:111
#5 0xc221db in cmd_func(cmd_list_element*, char const*, int)
src/gdb/cli/cli-decode.c:2181
#6 0x16feda6 in execute_command(char const*, int) src/gdb/top.c:668
#7 0xee9dc9 in command_handler(char const*) src/gdb/event-top.c:588
#8 0xeea6a8 in command_line_handler(std::unique_ptr<char,
gdb::xfree_deleter<char> >&&) src/gdb/event-top.c:773
#9 0xee8a12 in gdb_rl_callback_handler src/gdb/event-top.c:219
#10 0x7f550f24aead in rl_callback_read_char
(/lib64/libreadline.so.7+0x31ead)
...
The problem is here in remote_async_inferior_event_handler:
...
static void
remote_async_inferior_event_handler (gdb_client_data data)
{
inferior_event_handler (INF_REG_EVENT);
remote_target *remote = (remote_target *) data;
remote_state *rs = remote->get_remote_state ();
...
The remote target (passed in the data argument) can be destroyed during the
call to inferior_event_handler. If so, the call to remote->get_remote_state
() is done using a dangling pointer.
Fix this by increasing the reference count on the remote target before calling
inferior_event_handler, such that it won't get destroyed until right before
returning from remote_async_inferior_event_handler.
Tested on x86_64-linux.
Intended for gdb-10-branch.
The problem has stopped reproducing with the trigger patch since master commit
79952e69634 "Make scoped_restore_current_thread's cdtors exception free
(RFC)". We could still apply this to master though.
Any comments?
Thanks,
- Tom
[gdb/remote] Fix invalid pointer in remote_async_serial_handler
gdb/ChangeLog:
2021-01-07 Pedro Alves <pedro@palves.net>
Simon Marchi <simon.marchi@polymtl.ca>
Tom de Vries <tdevries@suse.de>
PR remote/26614
* remote.c (remote_async_inferior_event_handler): Hold a strong
reference to the remote target while handling an event.
---
gdb/remote.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 1407c23c3ce..4896c2af4d9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14163,9 +14163,13 @@ remote_async_serial_handler (struct serial *scb, void *context)
static void
remote_async_inferior_event_handler (gdb_client_data data)
{
+ remote_target *remote = (remote_target *) data;
+ /* Hold a strong reference to the remote target while handling an
+ event, since that could result in closing the connection. */
+ auto remote_ref = target_ops_ref::new_reference (remote);
+
inferior_event_handler (INF_REG_EVENT);
- remote_target *remote = (remote_target *) data;
remote_state *rs = remote->get_remote_state ();
/* inferior_event_handler may have consumed an event pending on the
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][gdb/remote] Fix invalid pointer in remote_async_serial_handler
2021-01-07 13:39 [PATCH][gdb/remote] Fix invalid pointer in remote_async_serial_handler Tom de Vries
@ 2021-01-07 15:15 ` Andrew Burgess
2021-01-07 16:27 ` Tom de Vries
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Burgess @ 2021-01-07 15:15 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches, Pedro Alves
* Tom de Vries <tdevries@suse.de> [2021-01-07 14:39:27 +0100]:
> Hi,
>
> On rare occasions, we run into this ERROR/UNRESOLVED on gdb-10-branch:
> ...
> (gdb) PASS: gdb.multi/multi-target.exp: continue: non-stop=on: inferior 2
> Remote debugging from host ::1, port 34088^M
> Process outputs/gdb.multi/multi-target/multi-target created; pid = 8649^M
> monitor exit^M
> (gdb) Killing process(es): 8649^M
> ERROR: GDB process no longer exists
> GDB process exited with wait status 8627 exp14 0 0 CHILDKILLED SIGABRT SIGABRT
> UNRESOLVED: gdb.multi/multi-target.exp: continue: non-stop=on: inferior 5
> ...
>
> A trigger patch makes the crash happen all the time:
> ...
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 71f814efb365..53ff8b63a1dc 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14161,14 +14161,12 @@ remote_target::is_async_p ()
> will be able to delay notifying the client of an event until the
> point where an entire packet has been received. */
>
> -static serial_event_ftype remote_async_serial_handler;
> -
> static void
> remote_async_serial_handler (struct serial *scb, void *context)
> {
> - /* Don't propogate error information up to the client. Instead let
> - the client find out about the error by querying the target. */
> - inferior_event_handler (INF_REG_EVENT);
> + remote_state *rs = (remote_state *) context;
> +
> + mark_async_event_handler (rs->remote_async_inferior_event_token);
> }
>
> static void
> ...
>
> And using -fsanitizer=address we can get a more elaborate error message:
> ...
> ==7196==ERROR: AddressSanitizer: heap-use-after-free on address \
> 0x6170000bf258 at pc 0x000001481755 bp 0x7fff05b20840 sp 0x7fff05b20838
> READ of size 8 at 0x6170000bf258 thread T0
> #0 0x1481754 in std::_Hashtable<gdbarch*, std::pair<gdbarch* const,
> remote_arch_state>, std::allocator<std::pair<gdbarch* const,
> remote_arch_state> >, std::__detail::_Select1st, std::equal_to<gdbarch*>,
> std::hash<gdbarch*>, std::__detail::_Mod_range_hashing,
> std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy,
> std::__detail::_Hashtable_traits<false, false, true>
> >::_M_bucket_index(unsigned long) const
> /usr/include/c++/11/bits/hashtable.h:719
> #1 0x147c8ab in std::_Hashtable<gdbarch*, std::pair<gdbarch* const,
> remote_arch_state>, std::allocator<std::pair<gdbarch* const,
> remote_arch_state> >, std::__detail::_Select1st, std::equal_to<gdbarch*>,
> std::hash<gdbarch*>, std::__detail::_Mod_range_hashing,
> std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy,
> std::__detail::_Hashtable_traits<false, false, true> >::find(gdbarch*
> const&) /usr/include/c++/11/bits/hashtable.h:1500
> #2 0x147852c in std::unordered_map<gdbarch*, remote_arch_state,
> std::hash<gdbarch*>, std::equal_to<gdbarch*>,
> std::allocator<std::pair<gdbarch* const, remote_arch_state> >
> >::find(gdbarch* const&) /usr/include/c++/11/bits/unordered_map.h:869
> #3 0x14306db in remote_state::get_remote_arch_state(gdbarch*)
> src/gdb/remote.c:1203
> #4 0x14309dc in remote_target::get_remote_state() src/gdb/remote.c:1232
> #5 0x1470c08 in remote_async_inferior_event_handler src/gdb/remote.c:14169
> #6 0xaa9f6b in check_async_event_handlers() src/gdb/async-event.c:295
> #7 0x1e93ab4 in gdb_do_one_event() src/gdbsupport/event-loop.cc:194
> #8 0x118f5f9 in start_event_loop src/gdb/main.c:356
> #9 0x118f8ed in captured_command_loop src/gdb/main.c:416
> #10 0x1192d6a in captured_main src/gdb/main.c:1253
> #11 0x1192dfa in gdb_main(captured_main_args*) src/gdb/main.c:1268
> #12 0x97b380 in main src/gdb/gdb.c:32
> #13 0x7f550c211349 in __libc_start_main ../csu/libc-start.c:308
> #14 0x97b199 in _start (build/gdb/gdb+0x97b199)
>
> 0x6170000bf258 is located 600 bytes inside of 648-byte region \
> [0x6170000bf000,0x6170000bf288)
> freed by thread T0 here:
> #0 0x7f550f516a57 in operator delete(void*, unsigned long)
> (/usr/lib64/libasan.so.6+0xaea57)
> #1 0x148b1fe in extended_remote_target::~extended_remote_target()
> src/gdb/remote.c:958
> #2 0x143b483 in remote_target::close() src/gdb/remote.c:4074
> #3 0x16cb90f in target_close(target_ops*) src/gdb/target.c:3230
> #4 0x16a2635 in decref_target(target_ops*) src/gdb/target.c:557
> #5 0x16a2abb in target_stack::unpush(target_ops*) src/gdb/target.c:645
> #6 0x16d01ef in inferior::unpush_target(target_ops*)
> src/gdb/inferior.h:356
> #7 0x16a2877 in unpush_target(target_ops*) src/gdb/target.c:607
> #8 0x16a2adf in unpush_target_and_assert src/gdb/target.c:655
> #9 0x16a2c57 in pop_all_targets_at_and_above(strata) src/gdb/target.c:678
> #10 0x1442749 in remote_unpush_target src/gdb/remote.c:5522
> #11 0x1458c16 in remote_target::readchar(int) src/gdb/remote.c:9137
> #12 0x145b25b in remote_target::getpkt_or_notif_sane_1(std::vector<char,
> gdb::default_init_allocator<char, std::allocator<char> > >*, int, int,
> int*) src/gdb/remote.c:9683
> #13 0x145bc9a in remote_target::getpkt_sane(std::vector<char,
> gdb::default_init_allocator<char, std::allocator<char> > >*, int)
> src/gdb/remote.c:9790
> #14 0x145b040 in remote_target::getpkt(std::vector<char,
> gdb::default_init_allocator<char, std::allocator<char> > >*, int)
> src/gdb/remote.c:9623
> #15 0x145780b in remote_target::remote_read_bytes_1(unsigned long,
> unsigned char*, unsigned long, int, unsigned long*) src/gdb/remote.c:8860
> #16 0x145805e in remote_target::remote_read_bytes(unsigned long,
> unsigned char*, unsigned long, int, unsigned long*) src/gdb/remote.c:8987
> #17 0x146113a in remote_target::xfer_partial(target_object, char const*,
> unsigned char*, unsigned char const*, unsigned long, unsigned long,
> unsigned long*) src/gdb/remote.c:10987
> #18 0x16a4004 in raw_memory_xfer_partial(target_ops*, unsigned char*,
> unsigned char const*, unsigned long, long, unsigned long*)
> src/gdb/target.c:918
> #19 0x16a4fcf in target_xfer_partial(target_ops*, target_object, char
> const*, unsigned char*, unsigned char const*, unsigned long, unsigned
> long, unsigned long*) src/gdb/target.c:1156
> #20 0x16a5d65 in target_read_partial src/gdb/target.c:1387
> #21 0x16a5f19 in target_read(target_ops*, target_object, char const*,
> unsigned char*, unsigned long, long) src/gdb/target.c:1427
> #22 0x16a5666 in target_read_raw_memory(unsigned long, unsigned char*,
> long) src/gdb/target.c:1260
> #23 0xd22f2a in dcache_read_line src/gdb/dcache.c:336
> #24 0xd232b7 in dcache_peek_byte src/gdb/dcache.c:403
> #25 0xd23845 in dcache_read_memory_partial(target_ops*, dcache_struct*,
> unsigned long, unsigned char*, unsigned long, unsigned long*) src/gdb/dcache.c:484
> #26 0x16a47da in memory_xfer_partial_1 src/gdb/target.c:1041
> #27 0x16a4a1e in memory_xfer_partial src/gdb/target.c:1084
> #28 0x16a4f44 in target_xfer_partial(target_ops*, target_object,
> char const*, unsigned char*, unsigned char const*, unsigned long,
> unsigned long, unsigned long*) src/gdb/target.c:1141
> #29 0x18203d4 in read_value_memory(value*, long, int, unsigned long,
> unsigned char*, unsigned long) src/gdb/valops.c:956
>
> previously allocated by thread T0 here:
> #0 0x7f550f515c37 in operator new(unsigned long)
> (/usr/lib64/libasan.so.6+0xadc37)
> #1 0x14429f0 in remote_target::open_1(char const*, int, int)
> src/gdb/remote.c:5562
> #2 0x14405e6 in extended_remote_target::open(char const*, int)
> src/gdb/remote.c:4907
> #3 0x16a0f3c in open_target src/gdb/target.c:242
> #4 0xc19ff5 in do_sfunc src/gdb/cli/cli-decode.c:111
> #5 0xc221db in cmd_func(cmd_list_element*, char const*, int)
> src/gdb/cli/cli-decode.c:2181
> #6 0x16feda6 in execute_command(char const*, int) src/gdb/top.c:668
> #7 0xee9dc9 in command_handler(char const*) src/gdb/event-top.c:588
> #8 0xeea6a8 in command_line_handler(std::unique_ptr<char,
> gdb::xfree_deleter<char> >&&) src/gdb/event-top.c:773
> #9 0xee8a12 in gdb_rl_callback_handler src/gdb/event-top.c:219
> #10 0x7f550f24aead in rl_callback_read_char
> (/lib64/libreadline.so.7+0x31ead)
> ...
>
> The problem is here in remote_async_inferior_event_handler:
> ...
> static void
> remote_async_inferior_event_handler (gdb_client_data data)
> {
> inferior_event_handler (INF_REG_EVENT);
>
> remote_target *remote = (remote_target *) data;
> remote_state *rs = remote->get_remote_state ();
> ...
>
> The remote target (passed in the data argument) can be destroyed during the
> call to inferior_event_handler. If so, the call to remote->get_remote_state
> () is done using a dangling pointer.
>
> Fix this by increasing the reference count on the remote target before calling
> inferior_event_handler, such that it won't get destroyed until right before
> returning from remote_async_inferior_event_handler.
>
> Tested on x86_64-linux.
>
> Intended for gdb-10-branch.
>
> The problem has stopped reproducing with the trigger patch since master commit
> 79952e69634 "Make scoped_restore_current_thread's cdtors exception free
> (RFC)". We could still apply this to master though.
>
> Any comments?
Simon already posted this patch series:
https://sourceware.org/pipermail/gdb-patches/2020-November/173633.html
Which should address this issue, I don't know if you'd seen this or
not?
You said above that your patch is proposed for gdb-10-branch, but also
that it could be applied to master. I think that this would be fine
applied to the gdb-10-branch, but I think Simon's fix would be better
for master.
Though that said, I notice your credit Simon & Pedro in the ChangeLog,
so maybe they've expressed a preference for this solution somewhere
and I've just missed it?
Thanks,
Andrew
>
> Thanks,
> - Tom
>
> [gdb/remote] Fix invalid pointer in remote_async_serial_handler
>
> gdb/ChangeLog:
>
> 2021-01-07 Pedro Alves <pedro@palves.net>
> Simon Marchi <simon.marchi@polymtl.ca>
> Tom de Vries <tdevries@suse.de>
>
> PR remote/26614
> * remote.c (remote_async_inferior_event_handler): Hold a strong
> reference to the remote target while handling an event.
>
> ---
> gdb/remote.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 1407c23c3ce..4896c2af4d9 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14163,9 +14163,13 @@ remote_async_serial_handler (struct serial *scb, void *context)
> static void
> remote_async_inferior_event_handler (gdb_client_data data)
> {
> + remote_target *remote = (remote_target *) data;
> + /* Hold a strong reference to the remote target while handling an
> + event, since that could result in closing the connection. */
> + auto remote_ref = target_ops_ref::new_reference (remote);
> +
> inferior_event_handler (INF_REG_EVENT);
>
> - remote_target *remote = (remote_target *) data;
> remote_state *rs = remote->get_remote_state ();
>
> /* inferior_event_handler may have consumed an event pending on the
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][gdb/remote] Fix invalid pointer in remote_async_serial_handler
2021-01-07 15:15 ` Andrew Burgess
@ 2021-01-07 16:27 ` Tom de Vries
2021-01-07 16:29 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2021-01-07 16:27 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, Pedro Alves, Simon Marchi
On 1/7/21 4:15 PM, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2021-01-07 14:39:27 +0100]:
>
>> Hi,
>>
>> On rare occasions, we run into this ERROR/UNRESOLVED on gdb-10-branch:
>> ...
>> (gdb) PASS: gdb.multi/multi-target.exp: continue: non-stop=on: inferior 2
>> Remote debugging from host ::1, port 34088^M
>> Process outputs/gdb.multi/multi-target/multi-target created; pid = 8649^M
>> monitor exit^M
>> (gdb) Killing process(es): 8649^M
>> ERROR: GDB process no longer exists
>> GDB process exited with wait status 8627 exp14 0 0 CHILDKILLED SIGABRT SIGABRT
>> UNRESOLVED: gdb.multi/multi-target.exp: continue: non-stop=on: inferior 5
>> ...
>>
>> A trigger patch makes the crash happen all the time:
>> ...
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 71f814efb365..53ff8b63a1dc 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -14161,14 +14161,12 @@ remote_target::is_async_p ()
>> will be able to delay notifying the client of an event until the
>> point where an entire packet has been received. */
>>
>> -static serial_event_ftype remote_async_serial_handler;
>> -
>> static void
>> remote_async_serial_handler (struct serial *scb, void *context)
>> {
>> - /* Don't propogate error information up to the client. Instead let
>> - the client find out about the error by querying the target. */
>> - inferior_event_handler (INF_REG_EVENT);
>> + remote_state *rs = (remote_state *) context;
>> +
>> + mark_async_event_handler (rs->remote_async_inferior_event_token);
>> }
>>
>> static void
>> ...
>>
>> And using -fsanitizer=address we can get a more elaborate error message:
>> ...
>> ==7196==ERROR: AddressSanitizer: heap-use-after-free on address \
>> 0x6170000bf258 at pc 0x000001481755 bp 0x7fff05b20840 sp 0x7fff05b20838
>> READ of size 8 at 0x6170000bf258 thread T0
>> #0 0x1481754 in std::_Hashtable<gdbarch*, std::pair<gdbarch* const,
>> remote_arch_state>, std::allocator<std::pair<gdbarch* const,
>> remote_arch_state> >, std::__detail::_Select1st, std::equal_to<gdbarch*>,
>> std::hash<gdbarch*>, std::__detail::_Mod_range_hashing,
>> std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy,
>> std::__detail::_Hashtable_traits<false, false, true>
>> >::_M_bucket_index(unsigned long) const
>> /usr/include/c++/11/bits/hashtable.h:719
>> #1 0x147c8ab in std::_Hashtable<gdbarch*, std::pair<gdbarch* const,
>> remote_arch_state>, std::allocator<std::pair<gdbarch* const,
>> remote_arch_state> >, std::__detail::_Select1st, std::equal_to<gdbarch*>,
>> std::hash<gdbarch*>, std::__detail::_Mod_range_hashing,
>> std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy,
>> std::__detail::_Hashtable_traits<false, false, true> >::find(gdbarch*
>> const&) /usr/include/c++/11/bits/hashtable.h:1500
>> #2 0x147852c in std::unordered_map<gdbarch*, remote_arch_state,
>> std::hash<gdbarch*>, std::equal_to<gdbarch*>,
>> std::allocator<std::pair<gdbarch* const, remote_arch_state> >
>> >::find(gdbarch* const&) /usr/include/c++/11/bits/unordered_map.h:869
>> #3 0x14306db in remote_state::get_remote_arch_state(gdbarch*)
>> src/gdb/remote.c:1203
>> #4 0x14309dc in remote_target::get_remote_state() src/gdb/remote.c:1232
>> #5 0x1470c08 in remote_async_inferior_event_handler src/gdb/remote.c:14169
>> #6 0xaa9f6b in check_async_event_handlers() src/gdb/async-event.c:295
>> #7 0x1e93ab4 in gdb_do_one_event() src/gdbsupport/event-loop.cc:194
>> #8 0x118f5f9 in start_event_loop src/gdb/main.c:356
>> #9 0x118f8ed in captured_command_loop src/gdb/main.c:416
>> #10 0x1192d6a in captured_main src/gdb/main.c:1253
>> #11 0x1192dfa in gdb_main(captured_main_args*) src/gdb/main.c:1268
>> #12 0x97b380 in main src/gdb/gdb.c:32
>> #13 0x7f550c211349 in __libc_start_main ../csu/libc-start.c:308
>> #14 0x97b199 in _start (build/gdb/gdb+0x97b199)
>>
>> 0x6170000bf258 is located 600 bytes inside of 648-byte region \
>> [0x6170000bf000,0x6170000bf288)
>> freed by thread T0 here:
>> #0 0x7f550f516a57 in operator delete(void*, unsigned long)
>> (/usr/lib64/libasan.so.6+0xaea57)
>> #1 0x148b1fe in extended_remote_target::~extended_remote_target()
>> src/gdb/remote.c:958
>> #2 0x143b483 in remote_target::close() src/gdb/remote.c:4074
>> #3 0x16cb90f in target_close(target_ops*) src/gdb/target.c:3230
>> #4 0x16a2635 in decref_target(target_ops*) src/gdb/target.c:557
>> #5 0x16a2abb in target_stack::unpush(target_ops*) src/gdb/target.c:645
>> #6 0x16d01ef in inferior::unpush_target(target_ops*)
>> src/gdb/inferior.h:356
>> #7 0x16a2877 in unpush_target(target_ops*) src/gdb/target.c:607
>> #8 0x16a2adf in unpush_target_and_assert src/gdb/target.c:655
>> #9 0x16a2c57 in pop_all_targets_at_and_above(strata) src/gdb/target.c:678
>> #10 0x1442749 in remote_unpush_target src/gdb/remote.c:5522
>> #11 0x1458c16 in remote_target::readchar(int) src/gdb/remote.c:9137
>> #12 0x145b25b in remote_target::getpkt_or_notif_sane_1(std::vector<char,
>> gdb::default_init_allocator<char, std::allocator<char> > >*, int, int,
>> int*) src/gdb/remote.c:9683
>> #13 0x145bc9a in remote_target::getpkt_sane(std::vector<char,
>> gdb::default_init_allocator<char, std::allocator<char> > >*, int)
>> src/gdb/remote.c:9790
>> #14 0x145b040 in remote_target::getpkt(std::vector<char,
>> gdb::default_init_allocator<char, std::allocator<char> > >*, int)
>> src/gdb/remote.c:9623
>> #15 0x145780b in remote_target::remote_read_bytes_1(unsigned long,
>> unsigned char*, unsigned long, int, unsigned long*) src/gdb/remote.c:8860
>> #16 0x145805e in remote_target::remote_read_bytes(unsigned long,
>> unsigned char*, unsigned long, int, unsigned long*) src/gdb/remote.c:8987
>> #17 0x146113a in remote_target::xfer_partial(target_object, char const*,
>> unsigned char*, unsigned char const*, unsigned long, unsigned long,
>> unsigned long*) src/gdb/remote.c:10987
>> #18 0x16a4004 in raw_memory_xfer_partial(target_ops*, unsigned char*,
>> unsigned char const*, unsigned long, long, unsigned long*)
>> src/gdb/target.c:918
>> #19 0x16a4fcf in target_xfer_partial(target_ops*, target_object, char
>> const*, unsigned char*, unsigned char const*, unsigned long, unsigned
>> long, unsigned long*) src/gdb/target.c:1156
>> #20 0x16a5d65 in target_read_partial src/gdb/target.c:1387
>> #21 0x16a5f19 in target_read(target_ops*, target_object, char const*,
>> unsigned char*, unsigned long, long) src/gdb/target.c:1427
>> #22 0x16a5666 in target_read_raw_memory(unsigned long, unsigned char*,
>> long) src/gdb/target.c:1260
>> #23 0xd22f2a in dcache_read_line src/gdb/dcache.c:336
>> #24 0xd232b7 in dcache_peek_byte src/gdb/dcache.c:403
>> #25 0xd23845 in dcache_read_memory_partial(target_ops*, dcache_struct*,
>> unsigned long, unsigned char*, unsigned long, unsigned long*) src/gdb/dcache.c:484
>> #26 0x16a47da in memory_xfer_partial_1 src/gdb/target.c:1041
>> #27 0x16a4a1e in memory_xfer_partial src/gdb/target.c:1084
>> #28 0x16a4f44 in target_xfer_partial(target_ops*, target_object,
>> char const*, unsigned char*, unsigned char const*, unsigned long,
>> unsigned long, unsigned long*) src/gdb/target.c:1141
>> #29 0x18203d4 in read_value_memory(value*, long, int, unsigned long,
>> unsigned char*, unsigned long) src/gdb/valops.c:956
>>
>> previously allocated by thread T0 here:
>> #0 0x7f550f515c37 in operator new(unsigned long)
>> (/usr/lib64/libasan.so.6+0xadc37)
>> #1 0x14429f0 in remote_target::open_1(char const*, int, int)
>> src/gdb/remote.c:5562
>> #2 0x14405e6 in extended_remote_target::open(char const*, int)
>> src/gdb/remote.c:4907
>> #3 0x16a0f3c in open_target src/gdb/target.c:242
>> #4 0xc19ff5 in do_sfunc src/gdb/cli/cli-decode.c:111
>> #5 0xc221db in cmd_func(cmd_list_element*, char const*, int)
>> src/gdb/cli/cli-decode.c:2181
>> #6 0x16feda6 in execute_command(char const*, int) src/gdb/top.c:668
>> #7 0xee9dc9 in command_handler(char const*) src/gdb/event-top.c:588
>> #8 0xeea6a8 in command_line_handler(std::unique_ptr<char,
>> gdb::xfree_deleter<char> >&&) src/gdb/event-top.c:773
>> #9 0xee8a12 in gdb_rl_callback_handler src/gdb/event-top.c:219
>> #10 0x7f550f24aead in rl_callback_read_char
>> (/lib64/libreadline.so.7+0x31ead)
>> ...
>>
>> The problem is here in remote_async_inferior_event_handler:
>> ...
>> static void
>> remote_async_inferior_event_handler (gdb_client_data data)
>> {
>> inferior_event_handler (INF_REG_EVENT);
>>
>> remote_target *remote = (remote_target *) data;
>> remote_state *rs = remote->get_remote_state ();
>> ...
>>
>> The remote target (passed in the data argument) can be destroyed during the
>> call to inferior_event_handler. If so, the call to remote->get_remote_state
>> () is done using a dangling pointer.
>>
>> Fix this by increasing the reference count on the remote target before calling
>> inferior_event_handler, such that it won't get destroyed until right before
>> returning from remote_async_inferior_event_handler.
>>
>> Tested on x86_64-linux.
>>
>> Intended for gdb-10-branch.
>>
>> The problem has stopped reproducing with the trigger patch since master commit
>> 79952e69634 "Make scoped_restore_current_thread's cdtors exception free
>> (RFC)". We could still apply this to master though.
>>
>> Any comments?
>
> Simon already posted this patch series:
>
> https://sourceware.org/pipermail/gdb-patches/2020-November/173633.html
>
> Which should address this issue, I don't know if you'd seen this or
> not?
>
Hi Andrew,
[ cc-ing Simon again ]
Simon did mention in the discussion in the PR that he would submit the
series, so yes, I was aware of that.
> You said above that your patch is proposed for gdb-10-branch, but also
> that it could be applied to master. I think that this would be fine
> applied to the gdb-10-branch,
Thanks for the review.
> but I think Simon's fix would be better
> for master.
>
Sure, that's fine by me.
I just wanted to mention the possibility of committing this to master,
and let others decide whether that's a good idea.
> Though that said, I notice your credit Simon & Pedro in the ChangeLog,
> so maybe they've expressed a preference for this solution somewhere
> and I've just missed it?
The patch was written by Pedro, and Simon provided the trigger patch.
The patch was discussed as possibility for gdb-10-branch, and this is
the submission for that.
Hope this clarifies things a bit.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][gdb/remote] Fix invalid pointer in remote_async_serial_handler
2021-01-07 16:27 ` Tom de Vries
@ 2021-01-07 16:29 ` Simon Marchi
0 siblings, 0 replies; 4+ messages in thread
From: Simon Marchi @ 2021-01-07 16:29 UTC (permalink / raw)
To: Tom de Vries, Andrew Burgess; +Cc: gdb-patches, Pedro Alves, Simon Marchi
On 2021-01-07 11:27 a.m., Tom de Vries wrote:
> Hi Andrew,
>
> [ cc-ing Simon again ]
>
> Simon did mention in the discussion in the PR that he would submit the
> series, so yes, I was aware of that.
>
>> You said above that your patch is proposed for gdb-10-branch, but also
>> that it could be applied to master. I think that this would be fine
>> applied to the gdb-10-branch,
> Thanks for the review.
>
>> but I think Simon's fix would be better
>> for master.
>>
> Sure, that's fine by me.
>
> I just wanted to mention the possibility of committing this to master,
> and let others decide whether that's a good idea.
>
>> Though that said, I notice your credit Simon & Pedro in the ChangeLog,
>> so maybe they've expressed a preference for this solution somewhere
>> and I've just missed it?
> The patch was written by Pedro, and Simon provided the trigger patch.
> The patch was discussed as possibility for gdb-10-branch, and this is
> the submission for that.
>
> Hope this clarifies things a bit.
>
> Thanks,
> - Tom
I agree that the refcount patch would be good for GDB 10, it's less
invasive. I still intend to merge my patch series in master, but I
am waiting for an ack from Pedro, who is quite busy at the moment.
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-07 16:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-07 13:39 [PATCH][gdb/remote] Fix invalid pointer in remote_async_serial_handler Tom de Vries
2021-01-07 15:15 ` Andrew Burgess
2021-01-07 16:27 ` Tom de Vries
2021-01-07 16:29 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).