[PATCH] gdb/linux-nat: use LP's inferior when handling vfork done in linux_handle_extended_wait

[PATCH] gdb/linux-nat: use LP's inferior when handling vfork done in linux_handle_extended_wait

[Simon Marchi]

I spotted this bug by reading the code and subsequently wrote a test to
reproduce it.  The bug is caught by the assertions that are added.
Otherwise the bug wouldn't cause a visible problem, but GDB would still
be in a wrong state.

When receiving a PTRACE_EVENT_VFORK_DONE, we do:

  if (event == PTRACE_EVENT_VFORK_DONE)
    {
      if (current_inferior ()->waiting_for_vfork_done)
	{
	  linux_nat_debug_printf
	    ("Got expected PTRACE_EVENT_VFORK_DONE from LWP %ld: stopping",
	     lp->ptid.lwp ());

	  ourstatus->kind = TARGET_WAITKIND_VFORK_DONE;
	  return 0;
	}

      linux_nat_debug_printf
	("Got PTRACE_EVENT_VFORK_DONE from LWP %ld: ignoring", lp->ptid.lwp ());

      return 1;
    }

However, the current inferior may not be the inferior for which we have
received the event, it could be another inferior of the linux-nat
target.  The current inferior is set in do_target_wait_1 before calling
target_wait, so that target_wait hits the target stack we want.  And
there isn't anything making LP's inferior current between pulling the
event out of the kernel and that code that handles
PTRACE_EVENT_VFORK_DONE.

Let's say that inferior 1 is waiting for a vfork done event while
inferior 2 is executing, doing things not vfork-related.  Inferior 2 is
chosen by do_target_wait and made the current one prior to calling
target_wait.  We pull the PTRACE_EVENT_VFORK_DONE event from the kernel
for inferior 1.  We check the current_inferior (inferior 2)'s
waiting_for_vfork_done flag.  It is false, so we (wrongfully) ignore the
event that was meant for inferior 1.

I thought that this would end up in inferior 1 getting stuck, waiting
for the event forever.  But we actually happen to attempt to resume
inferior 1 at various points (like in resume_stopped_resumed_lwps) after
having pulled the events), so inferior 1 resumes execution and is not
stuck.  However, the inferior::waiting_for_vfork_done flag stays set for
inferior 1, which is not a correct state.  A correct execution would
return TARGET_WAITKIND_VFORK_DONE to infrun for inferior 1, which would
clear the flag.  When processing TARGET_WAITKIND_VFORK_DONE, infrun also
clears the program_space::breakpoints_not_allowed flag.  That sounds
important, and failing to do that might lead to some other bad behavior.

To catch this bad state, add some assertions in handle_inferior_event.
The idea is that if an inferior is a vfork parent, we expect that it
can't report any events other than TARGET_WAITKIND_VFORK_DONE or
TARGET_WAITKIND_SIGNALLED (which can happen if you kill -9 it during the
vfork period).  The test program does multiple consecutive vforks.  If
the bug explained above happens and waiting_for_vfork_done stays
wrongfully set, the assertion will fail when a different event.

If I run the test without the fix in linux-nat.c, I get:

    run^M
    Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/vfork-multi-inferior/vfork-multi-inferior-vforker ^M
    [Detaching after vfork from child process 822537]^M
    /home/simark/src/binutils-gdb/gdb/infrun.c:5255: internal-error: void handle_inferior_event(execution_control_state*): Assertion `!inf->waiting_for_vfork_    done' failed.^M

At some point during my investigation, I suspected the random inferior
selection in do_target_wait to not work properly (always choosing one of
the two inferiors), so I wrote the test to try with the vforker as
inferior 1 and then as inferior 2.  In the end, I don't think that's
necessary, but I left the test as-is, as it will just test more
combinations, and the test runs quickly anyway.

Change-Id: Ie5b922d4f988d3fabf9f41fdeb83166da00af269
---
 gdb/infrun.c                                  |  13 +++
 gdb/linux-nat.c                               |   4 +-
 .../gdb.base/vfork-multi-inferior-other.c     |  12 ++
 .../gdb.base/vfork-multi-inferior-vforker.c   |  24 ++++
 .../gdb.base/vfork-multi-inferior.exp         | 107 ++++++++++++++++++
 5 files changed, 159 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/vfork-multi-inferior-other.c
 create mode 100644 gdb/testsuite/gdb.base/vfork-multi-inferior-vforker.c
 create mode 100644 gdb/testsuite/gdb.base/vfork-multi-inferior.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5ee650fa4645..87224f1e5096 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5242,6 +5242,19 @@ handle_inferior_event (struct execution_control_state *ecs)
 
   mark_non_executing_threads (ecs->target, ecs->ptid, ecs->ws);
 
+  /* If an inferior is a vfork parent, we expect it to be stopped, the only
+     two possible outcomes for it is VFORK_DONE (when the vfork child exits
+     or execs) or SIGNALLED, if is is forcefully killed (e.g. kill -9) from
+     outside GDB.  */
+  if (ecs->ws.kind != TARGET_WAITKIND_VFORK_DONE
+      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED)
+    {
+      inferior *inf = find_inferior_ptid (ecs->target, ecs->ptid);
+      gdb_assert (inf != nullptr);
+      gdb_assert (inf->vfork_child == nullptr);
+      gdb_assert (!inf->waiting_for_vfork_done);
+    }
+
   switch (ecs->ws.kind)
     {
     case TARGET_WAITKIND_LOADED:
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 211e447dc6f4..1b45f4e6c875 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2027,7 +2027,9 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
 
   if (event == PTRACE_EVENT_VFORK_DONE)
     {
-      if (current_inferior ()->waiting_for_vfork_done)
+      inferior *inf = find_inferior_ptid (linux_target, lp->ptid);
+
+      if (inf->waiting_for_vfork_done)
 	{
 	  linux_nat_debug_printf
 	    ("Got expected PTRACE_EVENT_VFORK_DONE from LWP %ld: stopping",
diff --git a/gdb/testsuite/gdb.base/vfork-multi-inferior-other.c b/gdb/testsuite/gdb.base/vfork-multi-inferior-other.c
new file mode 100644
index 000000000000..cb3f3d6abd78
--- /dev/null
+++ b/gdb/testsuite/gdb.base/vfork-multi-inferior-other.c
@@ -0,0 +1,12 @@
+#include <unistd.h>
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 30; i++)
+    sleep (1);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/vfork-multi-inferior-vforker.c b/gdb/testsuite/gdb.base/vfork-multi-inferior-vforker.c
new file mode 100644
index 000000000000..169b81d16902
--- /dev/null
+++ b/gdb/testsuite/gdb.base/vfork-multi-inferior-vforker.c
@@ -0,0 +1,24 @@
+#include <unistd.h>
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 10; i++)
+    {
+      pid_t pid = vfork ();
+
+      if (pid)
+	{
+	  /* Parent */
+	}
+      else
+	{
+	  /* Child */
+	  _exit (0);
+	}
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/vfork-multi-inferior.exp b/gdb/testsuite/gdb.base/vfork-multi-inferior.exp
new file mode 100644
index 000000000000..4ec3cf1a8635
--- /dev/null
+++ b/gdb/testsuite/gdb.base/vfork-multi-inferior.exp
@@ -0,0 +1,107 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Reproduce a case where the PTRACE_EVENT_VFORK_DONE event was wrongfully
+# ignored in the Linux native target, when debugging multiple inferiors.
+#
+# The idea of this test is:
+#
+#  - Start an inferior (named `other`) that executes in the background.
+#  - Start an inferior (named `vforker`) that repeatedly vforks.
+#
+# In a bad run, the PTRACE_EVENT_VFORK_DONE event would get ignored, the
+# waiting_for_vfork_done flag of the vforker inferior wouldn't get cleared,
+# and that would be caught by an assert later.
+#
+# For completeness, try with the vforker as first inferior and the other as
+# second inferior, and vice versa.
+
+standard_testfile -vforker.c -other.c
+
+set vforker_binfile ${binfile}-vforker
+set vforker_srcfile $srcfile
+set other_binfile ${binfile}-other
+set other_srcfile $srcfile2
+
+# We use multiple inferiors and run, which won't work on stub targets.
+if { [use_gdb_stub] } {
+    return
+}
+
+# For simplicity, we assume that everything is on the same filesystem (when
+# running with the native-extended-gdbserver board).
+if { [is_remote host] || [is_remote target] } {
+    return
+}
+
+if { [build_executable "failed to prepare" $vforker_binfile $vforker_srcfile] == -1 } {
+    return
+}
+
+if { [build_executable "failed to prepare" $other_binfile $other_srcfile] == -1 } {
+    return
+}
+
+proc do_test { first_is_vforker } {
+    save_vars { ::GDBFLAGS } {
+	set ::GDBFLAGS "$::GDBFLAGS -ex \"set non-stop\""
+	clean_restart
+    }
+
+    set remote [gdb_is_target_remote]
+
+    if { $first_is_vforker } {
+	set vforker_num 1
+	set other_num 2
+    } else {
+	set other_num 1
+	set vforker_num 2
+    }
+
+    gdb_test "add-inferior" "Added inferior 2.*"
+
+    # Add and start the other inferior (which just sleeps).
+    gdb_test "inferior $other_num" \
+	"Switching to inferior $other_num.*" \
+	"switch to other inferior"
+    gdb_test "file $::other_binfile" \
+	"Reading symbols from .*" \
+	"load other binfile"
+    if { $remote } {
+	gdb_test_no_output "set remote exec-file $::other_binfile" \
+	    "set remote exec-file for other inferior"
+    }
+    gdb_test "run &" \
+	 "Starting program: .*" \
+	 "run other inferior"
+
+    # Add and start the vforker inferior.  If everything goes right, it runs
+    # to completion.
+    gdb_test "inferior $vforker_num" \
+	"Switching to inferior $vforker_num.*" \
+	"switch to vforker inferior"
+    gdb_test "file $::vforker_binfile" \
+	"Reading symbols from .*" \
+	"load vforker binfile"
+    if { $remote } {
+	gdb_test_no_output "set remote exec-file $::vforker_binfile" \
+	    "set remote exec-file for vforker inferior"
+    }
+    gdb_test "run" "exited normally.*"
+}
+
+foreach_with_prefix first_is_vforker { 0 1 } {
+    do_test $first_is_vforker
+}
-- 
2.32.0

Re: [PATCH] gdb/linux-nat: use LP's inferior when handling vfork done in linux_handle_extended_wait

[Tom Tromey]

Simon> I spotted this bug by reading the code and subsequently wrote a test to
Simon> reproduce it.  The bug is caught by the assertions that are added.
Simon> Otherwise the bug wouldn't cause a visible problem, but GDB would still
Simon> be in a wrong state.

This explanation is really great.

Simon> the bug explained above happens and waiting_for_vfork_done stays
Simon> wrongfully set, the assertion will fail when a different event.

The clause after the "," is missing some text, I guess something like
"when a different even is received".

Simon> If I run the test without the fix in linux-nat.c, I get:

Simon>     run^M
Simon>     Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/vfork-multi-inferior/vfork-multi-inferior-vforker ^M
Simon>     [Detaching after vfork from child process 822537]^M
Simon>     /home/simark/src/binutils-gdb/gdb/infrun.c:5255: internal-error: void handle_inferior_event(execution_control_state*): Assertion `!inf->waiting_for_vfork_    done' failed.^M

Looks like some strange word wrapping in here, those spaces before the "done".

Simon>    if (event == PTRACE_EVENT_VFORK_DONE)
Simon>      {
Simon> -      if (current_inferior ()->waiting_for_vfork_done)
Simon> +      inferior *inf = find_inferior_ptid (linux_target, lp->ptid);
Simon> +
Simon> +      if (inf->waiting_for_vfork_done)

I was curious about this and looked, and saw that this is the only use
of current_inferior in this function.  A downside of our globals-based
approach is that it's hard to enforce a rule poisoning this kind of use
here.  Oh well.

Simon> index 000000000000..cb3f3d6abd78
Simon> --- /dev/null
Simon> +++ b/gdb/testsuite/gdb.base/vfork-multi-inferior-other.c
Simon> @@ -0,0 +1,12 @@
Simon> +#include <unistd.h>
Simon> +

I think we normally are sticking the GPL comment in all new files.

Otherwise this all looks reasonable to me.

Tom

Re: [PATCH] gdb/linux-nat: use LP's inferior when handling vfork done in linux_handle_extended_wait

[Simon Marchi]

On 2021-08-12 3:36 p.m., Tom Tromey wrote:
> Simon> I spotted this bug by reading the code and subsequently wrote a test to
> Simon> reproduce it.  The bug is caught by the assertions that are added.
> Simon> Otherwise the bug wouldn't cause a visible problem, but GDB would still
> Simon> be in a wrong state.
> 
> This explanation is really great.
> 
> Simon> the bug explained above happens and waiting_for_vfork_done stays
> Simon> wrongfully set, the assertion will fail when a different event.
> 
> The clause after the "," is missing some text, I guess something like
> "when a different even is received".

Fixed as suggested.

> 
> Simon> If I run the test without the fix in linux-nat.c, I get:
> 
> Simon>     run^M
> Simon>     Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/vfork-multi-inferior/vfork-multi-inferior-vforker ^M
> Simon>     [Detaching after vfork from child process 822537]^M
> Simon>     /home/simark/src/binutils-gdb/gdb/infrun.c:5255: internal-error: void handle_inferior_event(execution_control_state*): Assertion `!inf->waiting_for_vfork_    done' failed.^M
> 
> Looks like some strange word wrapping in here, those spaces before the "done".

Probably due to wrapping in my terminal or something, fixed.

> 
> Simon>    if (event == PTRACE_EVENT_VFORK_DONE)
> Simon>      {
> Simon> -      if (current_inferior ()->waiting_for_vfork_done)
> Simon> +      inferior *inf = find_inferior_ptid (linux_target, lp->ptid);
> Simon> +
> Simon> +      if (inf->waiting_for_vfork_done)
> 
> I was curious about this and looked, and saw that this is the only use
> of current_inferior in this function.  A downside of our globals-based
> approach is that it's hard to enforce a rule poisoning this kind of use
> here.  Oh well.

Yeah, I don't see any other way really than being cautious and having a
good mental model of how infrun interacts with the target's wait method
(which is hard, of course).

> Simon> index 000000000000..cb3f3d6abd78
> Simon> --- /dev/null
> Simon> +++ b/gdb/testsuite/gdb.base/vfork-multi-inferior-other.c
> Simon> @@ -0,0 +1,12 @@
> Simon> +#include <unistd.h>
> Simon> +
> 
> I think we normally are sticking the GPL comment in all new files.

Yes, I forgot.  Added.

> Otherwise this all looks reasonable to me.

Thanks, I'll leave it up for a bit in case others have some comments,
maybe Pedro if he has time.

Simon

Re: [PATCH] gdb/linux-nat: use LP's inferior when handling vfork done in linux_handle_extended_wait

[Simon Marchi]

On 2021-08-12 11:48 a.m., Simon Marchi wrote:
> I spotted this bug by reading the code and subsequently wrote a test to
> reproduce it.  The bug is caught by the assertions that are added.
> Otherwise the bug wouldn't cause a visible problem, but GDB would still
> be in a wrong state.
> 
> When receiving a PTRACE_EVENT_VFORK_DONE, we do:
> 
>   if (event == PTRACE_EVENT_VFORK_DONE)
>     {
>       if (current_inferior ()->waiting_for_vfork_done)
> 	{
> 	  linux_nat_debug_printf
> 	    ("Got expected PTRACE_EVENT_VFORK_DONE from LWP %ld: stopping",
> 	     lp->ptid.lwp ());
> 
> 	  ourstatus->kind = TARGET_WAITKIND_VFORK_DONE;
> 	  return 0;
> 	}
> 
>       linux_nat_debug_printf
> 	("Got PTRACE_EVENT_VFORK_DONE from LWP %ld: ignoring", lp->ptid.lwp ());
> 
>       return 1;
>     }
> 
> However, the current inferior may not be the inferior for which we have
> received the event, it could be another inferior of the linux-nat
> target.  The current inferior is set in do_target_wait_1 before calling
> target_wait, so that target_wait hits the target stack we want.  And
> there isn't anything making LP's inferior current between pulling the
> event out of the kernel and that code that handles
> PTRACE_EVENT_VFORK_DONE.
> 
> Let's say that inferior 1 is waiting for a vfork done event while
> inferior 2 is executing, doing things not vfork-related.  Inferior 2 is
> chosen by do_target_wait and made the current one prior to calling
> target_wait.  We pull the PTRACE_EVENT_VFORK_DONE event from the kernel
> for inferior 1.  We check the current_inferior (inferior 2)'s
> waiting_for_vfork_done flag.  It is false, so we (wrongfully) ignore the
> event that was meant for inferior 1.
> 
> I thought that this would end up in inferior 1 getting stuck, waiting
> for the event forever.  But we actually happen to attempt to resume
> inferior 1 at various points (like in resume_stopped_resumed_lwps) after
> having pulled the events), so inferior 1 resumes execution and is not
> stuck.  However, the inferior::waiting_for_vfork_done flag stays set for
> inferior 1, which is not a correct state.  A correct execution would
> return TARGET_WAITKIND_VFORK_DONE to infrun for inferior 1, which would
> clear the flag.  When processing TARGET_WAITKIND_VFORK_DONE, infrun also
> clears the program_space::breakpoints_not_allowed flag.  That sounds
> important, and failing to do that might lead to some other bad behavior.
> 
> To catch this bad state, add some assertions in handle_inferior_event.
> The idea is that if an inferior is a vfork parent, we expect that it
> can't report any events other than TARGET_WAITKIND_VFORK_DONE or
> TARGET_WAITKIND_SIGNALLED (which can happen if you kill -9 it during the
> vfork period).  The test program does multiple consecutive vforks.  If
> the bug explained above happens and waiting_for_vfork_done stays
> wrongfully set, the assertion will fail when a different event.
> 
> If I run the test without the fix in linux-nat.c, I get:
> 
>     run^M
>     Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/vfork-multi-inferior/vfork-multi-inferior-vforker ^M
>     [Detaching after vfork from child process 822537]^M
>     /home/simark/src/binutils-gdb/gdb/infrun.c:5255: internal-error: void handle_inferior_event(execution_control_state*): Assertion `!inf->waiting_for_vfork_    done' failed.^M
> 
> At some point during my investigation, I suspected the random inferior
> selection in do_target_wait to not work properly (always choosing one of
> the two inferiors), so I wrote the test to try with the vforker as
> inferior 1 and then as inferior 2.  In the end, I don't think that's
> necessary, but I left the test as-is, as it will just test more
> combinations, and the test runs quickly anyway.
> 
> Change-Id: Ie5b922d4f988d3fabf9f41fdeb83166da00af269
> ---
>  gdb/infrun.c                                  |  13 +++
>  gdb/linux-nat.c                               |   4 +-
>  .../gdb.base/vfork-multi-inferior-other.c     |  12 ++
>  .../gdb.base/vfork-multi-inferior-vforker.c   |  24 ++++
>  .../gdb.base/vfork-multi-inferior.exp         | 107 ++++++++++++++++++
>  5 files changed, 159 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.base/vfork-multi-inferior-other.c
>  create mode 100644 gdb/testsuite/gdb.base/vfork-multi-inferior-vforker.c
>  create mode 100644 gdb/testsuite/gdb.base/vfork-multi-inferior.exp
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 5ee650fa4645..87224f1e5096 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5242,6 +5242,19 @@ handle_inferior_event (struct execution_control_state *ecs)
>  
>    mark_non_executing_threads (ecs->target, ecs->ptid, ecs->ws);
>  
> +  /* If an inferior is a vfork parent, we expect it to be stopped, the only
> +     two possible outcomes for it is VFORK_DONE (when the vfork child exits
> +     or execs) or SIGNALLED, if is is forcefully killed (e.g. kill -9) from
> +     outside GDB.  */
> +  if (ecs->ws.kind != TARGET_WAITKIND_VFORK_DONE
> +      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED)
> +    {
> +      inferior *inf = find_inferior_ptid (ecs->target, ecs->ptid);
> +      gdb_assert (inf != nullptr);
> +      gdb_assert (inf->vfork_child == nullptr);
> +      gdb_assert (!inf->waiting_for_vfork_done);
> +    }

New piece of information.  Today I learned that when a multi-threaded
program vforks, only the vfork-ing thread is frozen until the vfork
child execs or exits (this is documented in the vfork man page on
Linux).  Other threads in the vfork-ing process still run.  So while a
thread is waiting for a vfork done event, other threads could in theory
report some other event.

But that leads to the question: is it really safe that the other threads
in the vfork-ing process stay running?  During the window of time where
the vfork-child and parent share an address space, we remove the
breakpoints from the parent:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/master/gdb/infrun.c#L439-449

On non-stop targets, by keeping the other threads running, don't we risk
them missing a breakpoint?

On all-stop targets, when we resume the vfork parent (expecting to
receive a vfork done event, which is the sign where it's finally safe to
re-insert the breakpoints), we do resume all threads.  That means the
other threads of the vfork parent run free without breakpoints inserted,
can't they also miss a breakpoint?

I'm thinking that when handling a vfork where we detach the child, the
safe thing to do would be:

 - non-stop target: stop all threads of the interior, remove
   breakpoints, resume only the vfork-ing thread, get the vfork done
   event, re-insert the breakpoints, resume all threads meant to be
   running

 - all-stop target: all threads are naturally stopped when the vfork
   event is reported, remove breakpoints, resume only the vfork-ing
   thread, get the vfork done event, re-insert breakpoints, resume all
   threads meant to be running

That's pretty much the same thing as when doing an in-line single-step.
Maybe that's actually what we do today, but I couldn't find signs of it.

In the mean time, I think that the fix in linux-nat.c (shown below) is
still right and would like to merge it, but I would remove the
assertions.  But that means I wouldn't know how to write a test.

> +
>    switch (ecs->ws.kind)
>      {
>      case TARGET_WAITKIND_LOADED:
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 211e447dc6f4..1b45f4e6c875 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -2027,7 +2027,9 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
>  
>    if (event == PTRACE_EVENT_VFORK_DONE)
>      {
> -      if (current_inferior ()->waiting_for_vfork_done)
> +      inferior *inf = find_inferior_ptid (linux_target, lp->ptid);
> +
> +      if (inf->waiting_for_vfork_done)

Simon