Re: [PATCH 03/31] gdb/linux: Delete all other LWPs immediately on ptrace exec event

[Pedro Alves <pedro@palves.net>]

Hi!

At long last, I've resumed work on this again.  A belated thank you so much for
the reviews.

On 2023-05-26 16:04, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:

>> +  /* Display one table row for each lwp_info.  */
>> +  for (lwp_info *lp : all_lwps ())
>> +    {
>> +      ui_out_emit_tuple tuple_emitter (uiout, "lwp-entry");
>> +
>> +      struct thread_info *th = find_thread_ptid (linux_target, lp->ptid);
> 
> After recent changes this line becomes:
> 
>   struct thread_info *th = linux_target->find_thread (lp->ptid);

Thanks, done.

>> From ee0a276c08b829ae504fe0eba5badc4f7faf3676 Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <pedro@palves.net>
>> Date: Wed, 13 Jul 2022 17:16:38 +0100
>> Subject: [PATCH 2/2] gdb/linux: Delete all other LWPs immediately on ptrace
>>  exec event
>>
>> I noticed that on an Ubuntu 20.04 system, after a following patch
>> ("Step over clone syscall w/ breakpoint,
>> TARGET_WAITKIND_THREAD_CLONED"), the gdb.threads/step-over-exec.exp
>> was passing cleanly, but still, we'd end up with four new unexpected
>> GDB core dumps:
>>
>> 		 === gdb Summary ===
>>
>>  # of unexpected core files      4
>>  # of expected passes            48
>>
>> That said patch is making the pre-existing
>> gdb.threads/step-over-exec.exp testcase (almost silently) expose a
>> latent problem in gdb/linux-nat.c, resulting in a GDB crash when:
>>
>>  #1 - a non-leader thread execs
>>  #2 - the post-exec program stops somewhere
>>  #3 - you kill the inferior
>>
>> Instead of #3 directly, the testcase just returns, which ends up in
>> gdb_exit, tearing down GDB, which kills the inferior, and is thus
>> equivalent to #3 above.
>>
>> Vis:
>>
>>  $ gdb --args ./gdb /home/pedro/gdb/build/gdb/testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-other-diff-text-segs-true
>>  ...
>>  (top-gdb) r
>>  ...
>>  (gdb) b main
>>  ...
>>  (gdb) r
>>  ...
>>  Breakpoint 1, main (argc=1, argv=0x7fffffffdb88) at /home/pedro/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/step-over-exec.c:69
>>  69        argv0 = argv[0];
>>  (gdb) c
>>  Continuing.
>>  [New Thread 0x7ffff7d89700 (LWP 2506975)]
>>  Other going in exec.
>>  Exec-ing /home/pedro/gdb/build/gdb/testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-other-diff-text-segs-true-execd
>>  process 2506769 is executing new program: /home/pedro/gdb/build/gdb/testsuite/outputs/gdb.threads/step-over-exec/step-over-exec-execr-thread-other-diff-text-segs-true-execd
>>
>>  Thread 1 "step-over-exec-" hit Breakpoint 1, main () at /home/pedro/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/step-over-exec-execd.c:28
>>  28        foo ();
>>  (gdb) k
>>  ...
>>  Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
>>  0x000055555574444c in thread_info::has_pending_waitstatus (this=0x0) at ../../src/gdb/gdbthread.h:393
>>  393         return m_suspend.waitstatus_pending_p;
>>  (top-gdb) bt
>>  #0  0x000055555574444c in thread_info::has_pending_waitstatus (this=0x0) at ../../src/gdb/gdbthread.h:393
>>  #1  0x0000555555a884d1 in get_pending_child_status (lp=0x5555579b8230, ws=0x7fffffffd130) at ../../src/gdb/linux-nat.c:1345
>>  #2  0x0000555555a8e5e6 in kill_unfollowed_child_callback (lp=0x5555579b8230) at ../../src/gdb/linux-nat.c:3564
>>  #3  0x0000555555a92a26 in gdb::function_view<int (lwp_info*)>::bind<int, lwp_info*>(int (*)(lwp_info*))::{lambda(gdb::fv_detail::erased_callable, lwp_info*)#1}::operator()(gdb::fv_detail::erased_callable, lwp_info*) const (this=0x0, ecall=..., args#0=0x5555579b8230) at ../../src/gdb/../gdbsupport/function-view.h:284
>>  #4  0x0000555555a92a51 in gdb::function_view<int (lwp_info*)>::bind<int, lwp_info*>(int (*)(lwp_info*))::{lambda(gdb::fv_detail::erased_callable, lwp_info*)#1}::_FUN(gdb::fv_detail::erased_callable, lwp_info*) () at ../../src/gdb/../gdbsupport/function-view.h:278
>>  #5  0x0000555555a91f84 in gdb::function_view<int (lwp_info*)>::operator()(lwp_info*) const (this=0x7fffffffd210, args#0=0x5555579b8230) at ../../src/gdb/../gdbsupport/function-view.h:247
>>  #6  0x0000555555a87072 in iterate_over_lwps(ptid_t, gdb::function_view<int (lwp_info*)>) (filter=..., callback=...) at ../../src/gdb/linux-nat.c:864
>>  #7  0x0000555555a8e732 in linux_nat_target::kill (this=0x55555653af40 <the_amd64_linux_nat_target>) at ../../src/gdb/linux-nat.c:3590
>>  #8  0x0000555555cfdc11 in target_kill () at ../../src/gdb/target.c:911
>>  ...
> 
> As I mentioned in my other message, this backtrace includes
> kill_unfollowed_child_callback, which doesn't exist yet!  I think that's
> OK though, the text before the backtrace does make it clear that you saw
> this problem only after applying a later patch.

I've tweaked the commit log to make that more explicit.

> 
>>
>> The root of the problem is that when a non-leader LWP execs, it just
>> changes its tid to the tgid, replacing the pre-exec leader thread,
>> becoming the new leader.  There's no thread exit event for the execing
>> thread.  It's as if the old pre-exec LWP vanishes without trace.  The
>> ptrace man page says:
>>
>> "PTRACE_O_TRACEEXEC (since Linux 2.5.46)
>> 	Stop the tracee at the next execve(2).  A waitpid(2) by the
>> 	tracer will return a status value such that
>>
>> 	  status>>8 == (SIGTRAP | (PTRACE_EVENT_EXEC<<8))
>>
>> 	If the execing thread is not a thread group leader, the thread
>> 	ID is reset to thread group leader's ID before this stop.
>> 	Since Linux 3.0, the former thread ID can be retrieved with
>> 	PTRACE_GETEVENTMSG."
>>
>> When the core of GDB processes an exec events, it deletes all the
>> threads of the inferior.  But, that is too late -- deleting the thread
>> does not delete the corresponding LWP, so we end leaving the pre-exec
>> non-leader LWP stale in the LWP list.  That's what leads to the crash
>> above -- linux_nat_target::kill iterates over all LWPs, and after the
>> patch in question, that code will look for the corresponding
>> thread_info for each LWP.  For the pre-exec non-leader LWP still
>> listed, won't find one.
>>
>> This patch fixes it, by deleting the pre-exec non-leader LWP (and
>> thread) from the LWP/thread lists as soon as we get an exec event out
>> of ptrace.
>>
>> GDBserver does not need an equivalent fix, because it is already doing
>> this, as side effect of mourning the pre-exec process, in
>> gdbserver/linux-low.cc:
>>
>>   else if (event == PTRACE_EVENT_EXEC && cs.report_exec_events)
>>     {
>> ...
>>       /* Delete the execing process and all its threads.  */
>>       mourn (proc);
>>       switch_to_thread (nullptr);
>>
>>
>> The crash with gdb.threads/step-over-exec.exp is not observable on
>> newer systems, which postdate the glibc change to move "libpthread.so"
>> internals to "libc.so.6", because right after the exec, GDB traps a
>> load event for "libc.so.6", which leads to GDB trying to open
>> libthread_db for the post-exec inferior, and, on such systems that
>> succeeds.  When we load libthread_db, we call
>> linux_stop_and_wait_all_lwps, which, as the name suggests, stops all
>> lwps, and then waits to see their stops.  While doing this, GDB
>> detects that the pre-exec stale LWP is gone, and deletes it.
>>
>> If we use "catch exec" to stop right at the exec before the
>> "libc.so.6" load event ever happens, and issue "kill" right there,
>> then GDB crashes on newer systems as well.  So instead of tweaking
>> gdb.threads/step-over-exec.exp to cover the fix, add a new
>> gdb.threads/threads-after-exec.exp testcase that uses "catch exec".
> 
> Maybe it's worth mentioning that because the crash itself only happens
> once a later patch is applied we use 'maint info linux-lwps' to reveal
> the issue for now?

I've done something like that.

>>
>> Change-Id: I21ec18072c7750f3a972160ae6b9e46590376643
>> ---
>>  gdb/infrun.c                                  |  8 +--
>>  gdb/linux-nat.c                               | 15 ++++
>>  .../gdb.threads/threads-after-exec.exp        | 70 +++++++++++++++++++
> 
> Oops, this diff is missing the two source files for this test (.c and
> -execd.c).  I was able to figure something out though so I could test
> the rest of this patch :)
> 

Ouch.  And enough time has passed that I completely lost those files.  But as you
discovered, they're trivial enough to rewrite.  Actually, I simplified, and
only use one .c file this time.


>>  3 files changed, 88 insertions(+), 5 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.threads/threads-after-exec.exp
>>

...

>> +
>> +standard_testfile .c -execd.c
>> +
>> +proc do_test { } {
>> +    global srcdir subdir srcfile srcfile2 binfile testfile
>> +    global decimal
>> +
>> +    # Compile main binary (the one that does the exec).
>> +    if {[gdb_compile_pthreads $srcdir/$subdir/$srcfile $binfile \
>> +	     executable {debug}] != "" } {
>> +	return -1
>> +    }
> 
> You can do:
> 
>     if {[build_executable "failed to build main executable" \
>              $binfile $srcfile {debug pthread}] == -1} {
> 	return -1
>     }
> 
>> +
>> +    # Compile the second binary (the one that gets exec'd).
>> +    if {[gdb_compile $srcdir/$subdir/$srcfile2 $binfile-execd \
>> +	     executable {debug}] != "" } {
>> +	return -1
>> +    }
> 
> And:
> 
>     if {[build_executable "failed to build execd executable" \
>              $binfile-execd $srcfile2 {debug}] == -1} {
> 	return -1
>     }
> 
> I thought we were moving away from calling the gdb_compile* functions
> directly.
> 

Done-ish -- since I only have one test program this time, I can use
prepare_for_testing, instead.

> Assuming the missing source files are added, this all looks great.
> 
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>