Re: [PATCH] [master & gdb 12] Fix core-file -> detach -> crash (corefiles/29275)

[Pedro Alves <pedro@palves.net>]

I've merged this to both master and gdb 12 branch.

On 2022-06-22 8:20 p.m., Pedro Alves wrote:
> After loading a core file, you're supposed to be able to use "detach"
> to unload the core file.  That unfortunately regressed starting with
> GDB 11, with these commits:
> 
>  1192f124a308 - gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses
>  408f66864a1a - detach in all-stop with threads running
> 
> resulting in a GDB crash:
> 
>  ...
>  Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
>  0x0000555555e842bf in maybe_set_commit_resumed_all_targets () at ../../src/gdb/infrun.c:2899
>  2899          if (proc_target->commit_resumed_state)
>  (top-gdb) bt
>  #0  0x0000555555e842bf in maybe_set_commit_resumed_all_targets () at ../../src/gdb/infrun.c:2899
>  #1  0x0000555555e848bf in scoped_disable_commit_resumed::reset (this=0x7fffffffd440) at ../../src/gdb/infrun.c:3023
>  #2  0x0000555555e84a0c in scoped_disable_commit_resumed::reset_and_commit (this=0x7fffffffd440) at ../../src/gdb/infrun.c:3049
>  #3  0x0000555555e739cd in detach_command (args=0x0, from_tty=1) at ../../src/gdb/infcmd.c:2791
>  #4  0x0000555555c0ba46 in do_simple_func (args=0x0, from_tty=1, c=0x55555662a600) at ../../src/gdb/cli/cli-decode.c:95
>  #5  0x0000555555c112b0 in cmd_func (cmd=0x55555662a600, args=0x0, from_tty=1) at ../../src/gdb/cli/cli-decode.c:2514
>  #6  0x0000555556173b1f in execute_command (p=0x5555565c5916 "", from_tty=1) at ../../src/gdb/top.c:699
> 
> The code that crashes looks like:
> 
>  static void
>  maybe_set_commit_resumed_all_targets ()
>  {
>    scoped_restore_current_thread restore_thread;
> 
>    for (inferior *inf : all_non_exited_inferiors ())
>      {
>        process_stratum_target *proc_target = inf->process_target ();
> 
>        if (proc_target->commit_resumed_state)
>            ^^^^^^^^^^^
> 
> With 'proc_target' above being null.  all_non_exited_inferiors filters
> out inferiors that have pid==0.  We get here at the end of
> detach_command, after core_target::detach has already run, at which
> point the inferior _should_ have pid==0 and no process target.  It is
> clear it no longer has a process target, but, it still has a pid!=0
> somehow.
> 
> The reason the inferior still has pid!=0, is that core_target::detach
> just unpushes, and relies on core_target::close to actually do the
> getting rid of the core and exiting the inferior.  The problem with
> that is that detach_command grabs an extra strong reference to the
> process stratum target, so the unpush_target inside
> core_target::detach doesn't actually result in a call to
> core_target::close.
> 
> Fix this my moving the cleaning up the core inferior to a shared
> routine called by both core_target::close and core_target::detach.  We
> still need to cleanup the inferior from within core_file::close
> because there are paths to it that want to get rid of the core without
> going through detach.  E.g., "core-file" -> "run".
> 
> This commit includes a new test added to gdb.base/corefile.exp to
> cover the "core-file core" -> "detach" scenario.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29275
> 
> Change-Id: Ic42bdd03182166b19f598428b0dbc2ce6f67c893
> ---
>  gdb/corelow.c                       | 27 +++++++++++++++++++++------
>  gdb/testsuite/gdb.base/corefile.exp | 12 ++++++++++++
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 8c33fb7ebb2..2037ef3dc89 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -122,6 +122,9 @@ class core_target final : public process_stratum_target
>  
>  private: /* per-core data */
>  
> +  /* Get rid of the core inferior.  */
> +  void clear_core ();
> +
>    /* The core's section table.  Note that these target sections are
>       *not* mapped in the current address spaces' set of target
>       sections --- those should come only from pure executable or
> @@ -308,10 +311,8 @@ core_target::build_file_mappings ()
>  /* An arbitrary identifier for the core inferior.  */
>  #define CORELOW_PID 1
>  
> -/* Close the core target.  */
> -
>  void
> -core_target::close ()
> +core_target::clear_core ()
>  {
>    if (core_bfd)
>      {
> @@ -325,6 +326,14 @@ core_target::close ()
>  
>        current_program_space->cbfd.reset (nullptr);
>      }
> +}
> +
> +/* Close the core target.  */
> +
> +void
> +core_target::close ()
> +{
> +  clear_core ();
>  
>    /* Core targets are heap-allocated (see core_target_open), so here
>       we delete ourselves.  */
> @@ -631,9 +640,15 @@ core_target_open (const char *arg, int from_tty)
>  void
>  core_target::detach (inferior *inf, int from_tty)
>  {
> -  /* Note that 'this' is dangling after this call.  unpush_target
> -     closes the target, and our close implementation deletes
> -     'this'.  */
> +  /* Get rid of the core.  Don't rely on core_target::close doing it,
> +     because target_detach may be called with core_target's refcount > 1,
> +     meaning core_target::close may not be called yet by the
> +     unpush_target call below.  */
> +  clear_core ();
> +
> +  /* Note that 'this' may be dangling after this call.  unpush_target
> +     closes the target if the refcount reaches 0, and our close
> +     implementation deletes 'this'.  */
>    inf->unpush_target (this);
>  
>    /* Clear the register cache and the frame cache.  */
> diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
> index 4ed92a02955..7f3d2efe3a2 100644
> --- a/gdb/testsuite/gdb.base/corefile.exp
> +++ b/gdb/testsuite/gdb.base/corefile.exp
> @@ -207,6 +207,16 @@ gdb_test "up" "#\[0-9\]* *\[0-9xa-fH'\]* in .* \\(.*\\).*" "up in corefile.exp (
>  
>  gdb_test "core" "No core file now."
>  
> +# Test that we can unload the core with the "detach" command.
> +
> +proc_with_prefix corefile_detach {} {
> +    clean_restart $::binfile
> +
> +    gdb_test "core-file $::corefile" "Core was generated by .*" "load core"
> +    gdb_test "detach" "No core file now\\." "detach core"
> +}
> +
> +corefile_detach
>  
>  # Test a run (start) command will clear any loaded core file.
>  
> @@ -222,6 +232,8 @@ proc corefile_test_run {} {
>  	return
>      }
>  
> +    clean_restart $::binfile
> +
>      gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
>      gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
>  
> 
> base-commit: 90b7a5df152a64d2bea20beb438e8b81049a5c30
>