public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Tom de Vries <tdevries@suse.de>
To: Simon Marchi <simon.marchi@efficios.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb: fix dealloc function not being called for frame 0
Date: Wed, 15 Feb 2023 19:01:52 +0100	[thread overview]
Message-ID: <5be9ed74-04cf-b909-9e8c-d0f38cf28501@suse.de> (raw)
In-Reply-To: <20230209195037.100368-1-simon.marchi@efficios.com>

On 2/9/23 20:50, Simon Marchi wrote:
> Tom de Vries reported [1] a regression in gdb.btrace/record_goto.exp
> caused by 6d3717d4c4 ("gdb: call frame unwinders' dealloc_cache methods
> through destroying the frame cache").  This issue is caught by ASan.  On
> a non-ASan build, it may or may not cause a crash or some other issue, I
> haven't tried.
> 
> I managed to narrow it down to:
> 
>      $ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.btrace/record_goto/record_goto -ex "start" -ex "record btrace" -ex "next"
> 
> ... and then doing repeatedly "record goto 19" and "record goto 27".
> Eventually, I get:
> 
>      (gdb) record goto 27
>      =================================================================
>      ==1527735==ERROR: AddressSanitizer: heap-use-after-free on address 0x6210003392a8 at pc 0x55e4c26eef86 bp 0x7ffd229f24e0 sp 0x7ffd229f24d8
>      READ of size 8 at 0x6210003392a8 thread T0
>          #0 0x55e4c26eef85 in bfcache_eq /home/simark/src/binutils-gdb/gdb/record-btrace.c:1639
>          #1 0x55e4c37cdeff in htab_find_slot_with_hash /home/simark/src/binutils-gdb/libiberty/hashtab.c:659
>          #2 0x55e4c37ce24a in htab_find_slot /home/simark/src/binutils-gdb/libiberty/hashtab.c:703
>          #3 0x55e4c26ef0c6 in bfcache_new /home/simark/src/binutils-gdb/gdb/record-btrace.c:1653
>          #4 0x55e4c26f1242 in record_btrace_frame_sniffer /home/simark/src/binutils-gdb/gdb/record-btrace.c:1820
>          #5 0x55e4c1b926a1 in frame_unwind_try_unwinder /home/simark/src/binutils-gdb/gdb/frame-unwind.c:136
>          #6 0x55e4c1b930d7 in frame_unwind_find_by_frame(frame_info_ptr, void**) /home/simark/src/binutils-gdb/gdb/frame-unwind.c:196
>          #7 0x55e4c1bb867f in get_frame_type(frame_info_ptr) /home/simark/src/binutils-gdb/gdb/frame.c:2925
>          #8 0x55e4c2ae6798 in print_frame_info(frame_print_options const&, frame_info_ptr, int, print_what, int, int) /home/simark/src/binutils-gdb/gdb/stack.c:1049
>          #9 0x55e4c2ade3e1 in print_stack_frame(frame_info_ptr, int, print_what, int) /home/simark/src/binutils-gdb/gdb/stack.c:367
>          #10 0x55e4c26fda03 in record_btrace_set_replay /home/simark/src/binutils-gdb/gdb/record-btrace.c:2779
>          #11 0x55e4c26fddc3 in record_btrace_target::goto_record(unsigned long) /home/simark/src/binutils-gdb/gdb/record-btrace.c:2843
>          #12 0x55e4c2de2bb2 in target_goto_record(unsigned long) /home/simark/src/binutils-gdb/gdb/target.c:4169
>          #13 0x55e4c275ed98 in record_goto(char const*) /home/simark/src/binutils-gdb/gdb/record.c:372
>          #14 0x55e4c275edba in cmd_record_goto /home/simark/src/binutils-gdb/gdb/record.c:383
> 
>      0x6210003392a8 is located 424 bytes inside of 4064-byte region [0x621000339100,0x62100033a0e0)
>      freed by thread T0 here:
>          #0 0x7f6ca34a5b6f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:123
>          #1 0x55e4c38a4c17 in rpl_free /home/simark/src/binutils-gdb/gnulib/import/free.c:44
>          #2 0x55e4c1bbd378 in xfree<void> /home/simark/src/binutils-gdb/gdb/../gdbsupport/gdb-xfree.h:37
>          #3 0x55e4c37d1b63 in call_freefun /home/simark/src/binutils-gdb/libiberty/obstack.c:103
>          #4 0x55e4c37d25a2 in _obstack_free /home/simark/src/binutils-gdb/libiberty/obstack.c:280
>          #5 0x55e4c1bad701 in reinit_frame_cache() /home/simark/src/binutils-gdb/gdb/frame.c:2112
>          #6 0x55e4c27705a3 in registers_changed_ptid(process_stratum_target*, ptid_t) /home/simark/src/binutils-gdb/gdb/regcache.c:564
>          #7 0x55e4c27708c7 in registers_changed_thread(thread_info*) /home/simark/src/binutils-gdb/gdb/regcache.c:573
>          #8 0x55e4c26fd922 in record_btrace_set_replay /home/simark/src/binutils-gdb/gdb/record-btrace.c:2772
>          #9 0x55e4c26fddc3 in record_btrace_target::goto_record(unsigned long) /home/simark/src/binutils-gdb/gdb/record-btrace.c:2843
>          #10 0x55e4c2de2bb2 in target_goto_record(unsigned long) /home/simark/src/binutils-gdb/gdb/target.c:4169
>          #11 0x55e4c275ed98 in record_goto(char const*) /home/simark/src/binutils-gdb/gdb/record.c:372
>          #12 0x55e4c275edba in cmd_record_goto /home/simark/src/binutils-gdb/gdb/record.c:383
> 
>      previously allocated by thread T0 here:
>          #0 0x7f6ca34a5e8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
>          #1 0x55e4c0b55c60 in xmalloc /home/simark/src/binutils-gdb/gdb/alloc.c:57
>          #2 0x55e4c37d1a6d in call_chunkfun /home/simark/src/binutils-gdb/libiberty/obstack.c:94
>          #3 0x55e4c37d1c20 in _obstack_begin_worker /home/simark/src/binutils-gdb/libiberty/obstack.c:141
>          #4 0x55e4c37d1ed7 in _obstack_begin /home/simark/src/binutils-gdb/libiberty/obstack.c:164
>          #5 0x55e4c1bad728 in reinit_frame_cache() /home/simark/src/binutils-gdb/gdb/frame.c:2113
>          #6 0x55e4c27705a3 in registers_changed_ptid(process_stratum_target*, ptid_t) /home/simark/src/binutils-gdb/gdb/regcache.c:564
>          #7 0x55e4c27708c7 in registers_changed_thread(thread_info*) /home/simark/src/binutils-gdb/gdb/regcache.c:573
>          #8 0x55e4c26fd922 in record_btrace_set_replay /home/simark/src/binutils-gdb/gdb/record-btrace.c:2772
>          #9 0x55e4c26fddc3 in record_btrace_target::goto_record(unsigned long) /home/simark/src/binutils-gdb/gdb/record-btrace.c:2843
>          #10 0x55e4c2de2bb2 in target_goto_record(unsigned long) /home/simark/src/binutils-gdb/gdb/target.c:4169
>          #11 0x55e4c275ed98 in record_goto(char const*) /home/simark/src/binutils-gdb/gdb/record.c:372
>          #12 0x55e4c275edba in cmd_record_goto /home/simark/src/binutils-gdb/gdb/record.c:383
> 
> The problem is a stale entry in the bfcache hash table (in
> record-btrace.c), left across a reinit_frame_cache.  This entry points
> to something that used to be allocated on the frame obstack, that has
> since been wiped by reinit_frame_cache.
> 
> Before the aforementioned, unwinder deallocation functions were called
> by iterating on the frame chain, starting with the sentinel frame, like
> so:
> 
>    /* Tear down all frame caches.  */
>    for (frame_info *fi = sentinel_frame; fi != NULL; fi = fi->prev)
>      {
>        if (fi->prologue_cache && fi->unwind->dealloc_cache)
> 	fi->unwind->dealloc_cache (fi, fi->prologue_cache);
>        if (fi->base_cache && fi->base->unwind->dealloc_cache)
> 	fi->base->unwind->dealloc_cache (fi, fi->base_cache);
>      }
> 
> After that patch, we relied on the fact that all frames are (supposedly)
> in the frame_stash.  A deletion function was added to the frame_stash
> hash table, so that dealloc functions would be called when emptying the
> frame stash.  There is one case, however, where a frame_info is not in
> the frame stash.  That is when we create the frame_info for the current
> frame (level 0, unwound from the sentinel frame), but don't computed its

computed -> compute

> frame id.  The computation of the frame id for that frame (and only that
> frame, AFAIK) is done lazily.  And putting a frame_info in the frame stash
> requires knowing its id.  So a frame 0 whose frame id is not computed
> yet is necessarily not in the frame stash.
> 
> When replaying with btrace, record_btrace_frame_sniffer insert entries
> corresponding to frames in the "bfcache" hash table.  It then relies on
> record_btrace_frame_dealloc_cache being called for each frame to remove
> all those entries when the frames get invalidated.  If a frame reinit
> happens while frame 0's id is not computed  (and thefore that frame is

thefore -> therefore

> not in frame_stash), record_btrace_frame_dealloc_cache does not get
> called for it, and it leaves a stale entry in bfcache.  That then leads
> to a use-after-free when that entry is accessed later, which ASan
> catches.
> 
> The proposed solution is to explicitly call frame_info_del on frame 0,
> if it exists, and if its frame id is not computed.  If it's frame id is

it's -> its

Thanks,
- Tom

> computed, it is expected that it will be in the frame stash, so it will
> be "deleted" through that.
> 
> Reported-By: Tom de Vries <tdevries@suse.de>


  parent reply	other threads:[~2023-02-15 18:01 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-09 19:50 Simon Marchi
2023-02-10  3:10 ` Simon Marchi
2023-02-15  9:39   ` Tom de Vries
2023-02-15 16:42     ` Simon Marchi
2023-02-15 17:56       ` Tom de Vries
2023-02-15 18:01 ` Tom de Vries [this message]
2023-02-15 18:45   ` Simon Marchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5be9ed74-04cf-b909-9e8c-d0f38cf28501@suse.de \
    --to=tdevries@suse.de \
    --cc=gdb-patches@sourceware.org \
    --cc=simon.marchi@efficios.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).