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

login
register
mail settings
Submitter Tom Tromey
Date July 2, 2018, 3:33 p.m.
Message ID <87o9fpodos.fsf@tromey.com>
Download mbox | patch
Permalink /patch/28192/
State New
Headers show

Comments

Tom Tromey - July 2, 2018, 3:33 p.m.
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

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

Thanks.  This seemed to work fine.

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

Indeed; I've changed it to use exit_inferior.

Pedro> Otherwise looks fine to me.

How's this?

Tom

commit 2697c65bdfb0078c6cd60b87f16d73b25399a32c
Author: Tom Tromey <tom@tromey.com>
Date:   Thu Jun 28 11:57:39 2018 -0600

    Fix crash in "run" on macOS when gdb is not signed
    
    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 calling exit_inferior and then updating
    inferior_ptid on failure.
    
    Tested by building an unsigned gdb on macOS and trying to run an
    inferior.  I also tried this with MI, and by attaching; as suggested
    by Pedro.
    
    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.
Pedro Alves - July 3, 2018, 2:16 p.m.
On 07/02/2018 04:33 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I'd try also with "attach", see if something else needs
> Pedro> cleaning up / unwinding.
> 
> Thanks.  This seemed to work fine.
> 
> Pedro> If you try this with MI as is, I think gdb will output a
> Pedro> -thread-group-started notification, and then an ^error.
> Pedro> You may want to call exit_inferior instead so that
> Pedro> gdb outputs a matching -thread-group-exited.
> 
> Indeed; I've changed it to use exit_inferior.
> 
> Pedro> Otherwise looks fine to me.
> 
> How's this?

Looks good.

Thanks,
Pedro Alves
Pedro Alves - July 4, 2018, 11:46 a.m.
On 07/03/2018 03:16 PM, Pedro Alves wrote:
> On 07/02/2018 04:33 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> I'd try also with "attach", see if something else needs
>> Pedro> cleaning up / unwinding.
>>
>> Thanks.  This seemed to work fine.
>>
>> Pedro> If you try this with MI as is, I think gdb will output a
>> Pedro> -thread-group-started notification, and then an ^error.
>> Pedro> You may want to call exit_inferior instead so that
>> Pedro> gdb outputs a matching -thread-group-exited.
>>
>> Indeed; I've changed it to use exit_inferior.
>>
>> Pedro> Otherwise looks fine to me.
>>
>> How's this?
> 
> Looks good.

Hi Tom, I happened to notice that you pushed the wrong version of
the patch.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0f601bdbf07..e61d042aa63 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-07-02  Tom Tromey  <tom@tromey.com>
+
+	PR cli/23340:
+	* darwin-nat.c (darwin_attach_pid): Call exit_inferior and reset
+	inferior_ptid on error.
+
 2018-07-02  Sebastian Huber  <sebastian.huber@embedded-brains.de>
 
 	* riscv-tdep.c (riscv_register_aliases): Swap "fp" and "s0" entries.
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 7dccce73926..38b3b765159 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1583,77 +1583,91 @@  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)
+    {
+      exit_inferior (inf);
+      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))