[RFA] Fix crash in "run" on macOS when gdb is not signed

Message ID 20180628180257.29883-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey June 28, 2018, 6:02 p.m. UTC
  On macOS, when gdb is not code-signed, it will throw an exception from
darwin_attach_pid.  However, gdb also then crashes:

thread.c:93: internal-error: struct thread_info *inferior_thread(): Assertion `tp' failed.

I think the problem here is that darwin_attach_pid does not clean up
inferior_ptid and inf->pid on failure.  This leads to a situation
where gdb tries to find a thread, but cannot.

In other cases, gdb would mourn the inferior at this point; but here
this is not possible because the target has not been pushed.  Instead
this patch works by simply updating the inferior and inferior_ptid on
failure.

Tested by building an unsigned gdb on macOS and trying to run an
inferior.

gdb/ChangeLog
2018-06-28  Tom Tromey  <tom@tromey.com>

	PR cli/23340:
	* darwin-nat.c (darwin_attach_pid): Reset inferior and
	inferior_ptid on error.
---
 gdb/ChangeLog    |   6 +++
 gdb/darwin-nat.c | 119 ++++++++++++++++++++++++++---------------------
 2 files changed, 73 insertions(+), 52 deletions(-)
  

Comments

Pedro Alves June 29, 2018, 7:31 p.m. UTC | #1
On 06/28/2018 07:02 PM, Tom Tromey wrote:
> On macOS, when gdb is not code-signed, it will throw an exception from
> darwin_attach_pid.  However, gdb also then crashes:
> 
> thread.c:93: internal-error: struct thread_info *inferior_thread(): Assertion `tp' failed.
> 
> I think the problem here is that darwin_attach_pid does not clean up
> inferior_ptid and inf->pid on failure.  This leads to a situation
> where gdb tries to find a thread, but cannot.
> 
> In other cases, gdb would mourn the inferior at this point; but here
> this is not possible because the target has not been pushed.  Instead
> this patch works by simply updating the inferior and inferior_ptid on
> failure.
> 
> Tested by building an unsigned gdb on macOS and trying to run an
> inferior.

I'd try also with "attach", see if something else needs
cleaning up / unwinding.



> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 7dccce73926..6fb108846d0 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -1583,77 +1583,92 @@ darwin_attach_pid (struct inferior *inf)
>    darwin_inferior *priv = new darwin_inferior;
>    inf->priv.reset (priv);
>  
> -  kret = task_for_pid (gdb_task, inf->pid, &priv->task);
> -  if (kret != KERN_SUCCESS)
> +  TRY
>      {

[snip reindent]

> +    }
> +  CATCH (ex, RETURN_MASK_ALL)
> +    {
> +      inf->pid = 0;

If you try this with MI as is, I think gdb will output a
-thread-group-started notification, and then an ^error.
You may want to call exit_inferior instead so that
gdb outputs a matching -thread-group-exited.

Otherwise looks fine to me.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 417c563849a..ff4f0d9acb3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-06-28  Tom Tromey  <tom@tromey.com>
+
+	PR cli/23340:
+	* darwin-nat.c (darwin_attach_pid): Reset inferior and
+	inferior_ptid on error.
+
 2018-06-28  Pedro Alves  <palves@redhat.com>
 
 	* infrun.c (handle_inferior_event_1) <TARGET_WAITKIND_EXECD>:
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 7dccce73926..6fb108846d0 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1583,77 +1583,92 @@  darwin_attach_pid (struct inferior *inf)
   darwin_inferior *priv = new darwin_inferior;
   inf->priv.reset (priv);
 
-  kret = task_for_pid (gdb_task, inf->pid, &priv->task);
-  if (kret != KERN_SUCCESS)
+  TRY
     {
-      int status;
-
-      if (!inf->attach_flag)
+      kret = task_for_pid (gdb_task, inf->pid, &priv->task);
+      if (kret != KERN_SUCCESS)
 	{
-	  kill (inf->pid, 9);
-	  waitpid (inf->pid, &status, 0);
-	}
+	  int status;
+
+	  if (!inf->attach_flag)
+	    {
+	      kill (inf->pid, 9);
+	      waitpid (inf->pid, &status, 0);
+	    }
 
-      error (_("Unable to find Mach task port for process-id %d: %s (0x%lx).\n"
+	  error
+	    (_("Unable to find Mach task port for process-id %d: %s (0x%lx).\n"
 	       " (please check gdb is codesigned - see taskgated(8))"),
-             inf->pid, mach_error_string (kret), (unsigned long) kret);
-    }
+	     inf->pid, mach_error_string (kret), (unsigned long) kret);
+	}
 
-  inferior_debug (2, _("inferior task: 0x%x, pid: %d\n"),
-		  priv->task, inf->pid);
+      inferior_debug (2, _("inferior task: 0x%x, pid: %d\n"),
+		      priv->task, inf->pid);
 
-  if (darwin_ex_port == MACH_PORT_NULL)
-    {
-      /* Create a port to get exceptions.  */
-      kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_RECEIVE,
-				 &darwin_ex_port);
-      if (kret != KERN_SUCCESS)
-	error (_("Unable to create exception port, mach_port_allocate "
-		 "returned: %d"),
-	       kret);
+      if (darwin_ex_port == MACH_PORT_NULL)
+	{
+	  /* Create a port to get exceptions.  */
+	  kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_RECEIVE,
+				     &darwin_ex_port);
+	  if (kret != KERN_SUCCESS)
+	    error (_("Unable to create exception port, mach_port_allocate "
+		     "returned: %d"),
+		   kret);
 
-      kret = mach_port_insert_right (gdb_task, darwin_ex_port, darwin_ex_port,
-				     MACH_MSG_TYPE_MAKE_SEND);
-      if (kret != KERN_SUCCESS)
-	error (_("Unable to create exception port, mach_port_insert_right "
-		 "returned: %d"),
-	       kret);
+	  kret = mach_port_insert_right (gdb_task, darwin_ex_port,
+					 darwin_ex_port,
+					 MACH_MSG_TYPE_MAKE_SEND);
+	  if (kret != KERN_SUCCESS)
+	    error (_("Unable to create exception port, mach_port_insert_right "
+		     "returned: %d"),
+		   kret);
 
-      /* Create a port set and put ex_port in it.  */
-      kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_PORT_SET,
-				 &darwin_port_set);
+	  /* Create a port set and put ex_port in it.  */
+	  kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_PORT_SET,
+				     &darwin_port_set);
+	  if (kret != KERN_SUCCESS)
+	    error (_("Unable to create port set, mach_port_allocate "
+		     "returned: %d"),
+		   kret);
+
+	  kret = mach_port_move_member (gdb_task, darwin_ex_port,
+					darwin_port_set);
+	  if (kret != KERN_SUCCESS)
+	    error (_("Unable to move exception port into new port set, "
+		     "mach_port_move_member\n"
+		     "returned: %d"),
+		   kret);
+	}
+
+      /* Create a port to be notified when the child task terminates.  */
+      kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_RECEIVE,
+				 &priv->notify_port);
       if (kret != KERN_SUCCESS)
-	error (_("Unable to create port set, mach_port_allocate "
+	error (_("Unable to create notification port, mach_port_allocate "
 		 "returned: %d"),
 	       kret);
 
-      kret = mach_port_move_member (gdb_task, darwin_ex_port, darwin_port_set);
+      kret = mach_port_move_member (gdb_task,
+				    priv->notify_port, darwin_port_set);
       if (kret != KERN_SUCCESS)
-	error (_("Unable to move exception port into new port set, "
+	error (_("Unable to move notification port into new port set, "
 		 "mach_port_move_member\n"
 		 "returned: %d"),
 	       kret);
-    }
-
-  /* Create a port to be notified when the child task terminates.  */
-  kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_RECEIVE,
-			     &priv->notify_port);
-  if (kret != KERN_SUCCESS)
-    error (_("Unable to create notification port, mach_port_allocate "
-	     "returned: %d"),
-	   kret);
 
-  kret = mach_port_move_member (gdb_task,
-				priv->notify_port, darwin_port_set);
-  if (kret != KERN_SUCCESS)
-    error (_("Unable to move notification port into new port set, "
-	     "mach_port_move_member\n"
-	     "returned: %d"),
-	   kret);
+      darwin_setup_request_notification (inf);
 
-  darwin_setup_request_notification (inf);
+      darwin_setup_exceptions (inf);
+    }
+  CATCH (ex, RETURN_MASK_ALL)
+    {
+      inf->pid = 0;
+      inf->priv.reset ();
+      inferior_ptid = null_ptid;
 
-  darwin_setup_exceptions (inf);
+      throw_exception (ex);
+    }
+  END_CATCH
 
   target_ops *darwin_ops = get_native_target ();
   if (!target_is_pushed (darwin_ops))