Re: [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2

[Simon Marchi <simark@simark.ca>]

On 11/7/23 08:19, Tom de Vries wrote:
> The previous commit describes PR gdb/30547, a segfault when running test-case
> gdb.base/vfork-follow-parent.exp on powerpc64 (likewise on s390x).
> 
> The root cause for the segmentation fault is that linux_is_uclinux gives an
> incorrect result: it returns true instead of false.
> 
> So, why does linux_is_uclinux:
> ...
> int
> linux_is_uclinux (void)
> {
>   CORE_ADDR dummy;
> 
>   return (target_auxv_search (AT_NULL, &dummy) > 0
> 	  && target_auxv_search (AT_PAGESZ, &dummy) == 0);
> ...
> return true?
> 
> This is because ppc_linux_target_wordsize returns 4 instead of 8, causing
> ppc_linux_nat_target::auxv_parse to misinterpret the auxv vector.
> 
> So, why does ppc_linux_target_wordsize:
> ...
> int
> ppc_linux_target_wordsize (int tid)
> {
>   int wordsize = 4;
> 
>   /* Check for 64-bit inferior process.	 This is the case when the host is
>      64-bit, and in addition the top bit of the MSR register is set.  */
>   long msr;
> 
>   errno = 0;
>   msr = (long) ptrace (PTRACE_PEEKUSER, tid, PT_MSR * 8, 0);
>   if (errno == 0 && ppc64_64bit_inferior_p (msr))
>     wordsize = 8;
> 
>   return wordsize;
> }
> ...
> return 4?
> 
> Specifically, we get this result because because tid == 0, so we get
> errno == ESRCH.

I would suggest adding some assertion:

 - assert that inferior_ptid != null_ptid in
   ppc_linux_nat_target::auxv_parse and
   s390_linux_nat_target::auxv_parse
 - assert that tid != 0 in ppc_linux_target_wordsize (and in
   s390_target_wordsize), though it could use a bit of refactoring to
   take tid as a parameter like ppc_linux_target_wordsize)


> The tid == 0 is caused by the switch_to_no_thread in
> handle_vfork_child_exec_or_exit:
> ...
> 	  /* Switch to no-thread while running clone_program_space, so
> 	     that clone_program_space doesn't want to read the
> 	     selected frame of a dead process.  */
> 	  scoped_restore_current_thread restore_thread;
> 	  switch_to_no_thread ();
> 
> 	  inf->pspace = new program_space (maybe_new_address_space ());
> ...
> but moving the maybe_new_address_space call to before that gives us the
> same result.  The tid is no longer 0, but we still get ESRCH because the
> thread has exited.
> 
> Fix this in handle_vfork_child_exec_or_exit by doing the
> maybe_new_address_space call in the context of the vfork parent.
> 
> Tested on top of trunk on x86_64-linux and ppc64le-linux.
> Tested on top of gdb-14-branch on ppc64-linux.
> 
> PR gdb/30547
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547
> ---
>  gdb/infrun.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index f0f35c23c9c..94a0924653f 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec)
>  	     go ahead and create a new one for this exiting
>  	     inferior.  */
>  
> +	  std::shared_ptr<address_space> aspace;
> +	  {
> +	    /* Temporarily switch to the vfork parent, to facilitate ptrace
> +	       calls done during maybe_new_address_space.  */
> +	    scoped_restore save_inferior_ptid
> +	      = make_scoped_restore (&inferior_ptid);
> +	    inferior_ptid = ptid_t (vfork_parent->pid);
> +	    aspace = maybe_new_address_space ();
> +	  }

I guess that's fine as a minimal patch, but it scares me when we just
set part of the global state and inferior_ptid / current_inferior /
current_program_space don't match.  maybe_new_address_space calls
gdbarch_has_shared_address_space, which calls linux_is_uclinux, which
reads auxv.  auxv can be read in different ways.  It's hard to get
convinced that only setting inferior_ptid is enough.

Since we need a scoped_restore_current_thread in the current scope, can
you move it a bit higher?  Something like (untested):

  scoped_restore_current_thread restore_thread;

  /* Temporarily switch to the vfork parent, to facilitate ptrace
     calls done during maybe_new_address_space.  */
  switch_to_thread (any_live_thread_of_inferior (vfork_parent));
  std::shared_ptr<address_space> aspace = maybe_new_address_space ();

  /* Switch back to the vfork child inferior.  Switch to no-thread while
     running clone_program_space, so that clone_program_space doesn't
     want to read the selected frame of a dead process.  */
  switch_to_inferior_no_thread (inf);

Below, you can std::move the aspace when creating the new program_space:

  inf->pspace = new program_space (std::move (aspace));

Even better, I think, would be to pass the vfork parent inferior as a
new `existing_inferior` parameter to maybe_new_address_space, and pass
it all the way through linux_is_uclinux (and do some inferior switching
there in order to do the target calls).  The existing_inferior parameter
would be an inferior that maybe_new_address_space can inspect to
determine if a new address space is needed, and from which it can get
an existing address space, if it determines that the address space
should be shared between the existing and the new inferior.  But I think
it's something that can be done later, switching to the parent here is
fine to keep the patch small and easily backportable.

Simon