Re: [PATCH] gdb: fix dealloc function not being called for frame 0

[Simon Marchi <simon.marchi@efficios.com>]

On 2/15/23 13:01, Tom de Vries wrote:
> 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

Fixed.

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

Fixed.

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

Fixed.

I will push the patch shortly, thanks.

Simon