[commit] remote: make the whole connection setup exception safe

[commit] remote: make the whole connection setup exception safe

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 2320 bytes --]

 (gdb) tar rem | asdfasdfa
 Remote debugging using | asdfasdfa
 sh: asdfasdfa: not found
 Remote communication error: Resource temporarily unavailable.
 (gdb) p a
 ../../src/gdb/thread.c:529: internal-error: is_thread_state: Assertion `tp' failed.
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.
 Quit this debugging session? (y or n) 

Ok, doesn't look good, and when I wrote this patch, the symptoms
weren't so bad, but this is really undefined behaviour ...

The big picture: We got ourselves a half-backed target pushed on
the stack.  Not very digestible.

The issue is that we have this remote_start_remote function
that is wrapped in catch_exceptions, where the initial comunication
with the target should be done.  At its call site, we have this
comment:

remote_open_1:

"  /* Start the remote connection.  If error() or QUIT, discard this
     target (we'd otherwise be in an inconsistent state) and then
     propogate the error on up the exception chain.  This ensures that
     the caller doesn't stumble along blindly assuming that the
     function succeeded.
"

But, over time, several possible calls to putpkt/getpkt/error made
its way to remote_open_1, making it possible to leave a
half-backed target pushed when an exception is thrown.

So, the patch looks bigger than it really is.  I'm just mostly
moving things around (but keeping the same sequences).

(The initial acknowleding of any pending ack, which should be the
first thing sent to the stub used to be in remote_start_remote, until
I moved it out, with the noack mode changes, because I had noticed
that it was no longer the first thing being sent.)

Daniel then found out that this made an existing race easier to
trigger.  If the connection is closed while inside
remote_start_remote, readchar calls target_mourn_inferior followed
by throwing an error.  This exception is then caught in the safe
net of remote_open_1 mentioned about, that pops the target.  Problem is,
remote_mourn_1 had already unpushed the target.  This meant that
the second pop could pop the dummy target --- not something that we
should be doing every day.  The following commands would crash, because
the target_stack would be borked.

Tested against a local x86-64-unknown-linux-gnu gdbserver and
checked in.

-- 
Pedro Alves

[-- Attachment #2: remote_setup_excp_safe.diff --]
[-- Type: text/x-diff, Size: 7224 bytes --]

2008-10-09  Pedro Alves  <pedro@codesourcery.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>

	* remote.c (remote_open_1): Move acknowledging any pending ack,
	querying supported features, activating noack mode, finding the
	target description, enabling extended remote, and checking remote
	symbols from here ...
	(remote_start_remote): ... to here.
	(remote_open_1): Don't pop the target if it is already gone.
	* target.c (unpush_target): Check for the dummy target.

---
 gdb/remote.c |  115 +++++++++++++++++++++++++++++++----------------------------
 gdb/target.c |    4 ++
 2 files changed, 65 insertions(+), 54 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-10-09 03:20:08.000000000 +0100
+++ src/gdb/remote.c	2008-10-09 03:26:18.000000000 +0100
@@ -211,6 +211,10 @@ static void show_remote_protocol_packet_
 static char *write_ptid (char *buf, const char *endbuf, ptid_t ptid);
 static ptid_t read_ptid (char *buf, char **obuf);
 
+static void remote_query_supported (void);
+
+static void remote_check_symbols (struct objfile *objfile);
+
 void _initialize_remote (void);
 
 /* For "remote".  */
@@ -2375,12 +2379,50 @@ struct start_remote_args
 static void
 remote_start_remote (struct ui_out *uiout, void *opaque)
 {
-  struct remote_state *rs = get_remote_state ();
   struct start_remote_args *args = opaque;
+  struct remote_state *rs = get_remote_state ();
+  struct packet_config *noack_config;
   char *wait_status = NULL;
 
   immediate_quit++;		/* Allow user to interrupt it.  */
 
+  /* Ack any packet which the remote side has already sent.  */
+  serial_write (remote_desc, "+", 1);
+
+  /* The first packet we send to the target is the optional "supported
+     packets" request.  If the target can answer this, it will tell us
+     which later probes to skip.  */
+  remote_query_supported ();
+
+  /* Next, we possibly activate noack mode.
+
+     If the QStartNoAckMode packet configuration is set to AUTO,
+     enable noack mode if the stub reported a wish for it with
+     qSupported.
+
+     If set to TRUE, then enable noack mode even if the stub didn't
+     report it in qSupported.  If the stub doesn't reply OK, the
+     session ends with an error.
+
+     If FALSE, then don't activate noack mode, regardless of what the
+     stub claimed should be the default with qSupported.  */
+
+  noack_config = &remote_protocol_packets[PACKET_QStartNoAckMode];
+
+  if (noack_config->detect == AUTO_BOOLEAN_TRUE
+      || (noack_config->detect == AUTO_BOOLEAN_AUTO
+	  && noack_config->support == PACKET_ENABLE))
+    {
+      putpkt ("QStartNoAckMode");
+      getpkt (&rs->buf, &rs->buf_size, 0);
+      if (packet_ok (rs->buf, noack_config) == PACKET_OK)
+	rs->noack_mode = 1;
+    }
+
+  /* Next, if the target can specify a description, read it.  We do
+     this before anything involving memory or registers.  */
+  target_find_description ();
+
   /* Check whether the target is running now.  */
   putpkt ("?");
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -2439,6 +2481,20 @@ remote_start_remote (struct ui_out *uiou
 
   immediate_quit--;
   start_remote (args->from_tty); /* Initialize gdb process mechanisms.  */
+
+  if (args->extended_p)
+    {
+      /* Tell the remote that we are using the extended protocol.  */
+      putpkt ("!");
+      getpkt (&rs->buf, &rs->buf_size, 0);
+    }
+
+  /* If we connected to a live target, do some additional setup.  */
+  if (target_has_execution)
+    {
+      if (exec_bfd) 	/* No use without an exec file.  */
+	remote_check_symbols (symfile_objfile);
+    }
 }
 
 /* Open a connection to a remote debugger.
@@ -2788,7 +2844,6 @@ static void
 remote_open_1 (char *name, int from_tty, struct target_ops *target, int extended_p)
 {
   struct remote_state *rs = get_remote_state ();
-  struct packet_config *noack_config;
 
   if (name == 0)
     error (_("To open a remote debug connection, you need to specify what\n"
@@ -2883,43 +2938,6 @@ remote_open_1 (char *name, int from_tty,
   use_threadinfo_query = 1;
   use_threadextra_query = 1;
 
-  /* Ack any packet which the remote side has already sent.  */
-  serial_write (remote_desc, "+", 1);
-
-  /* The first packet we send to the target is the optional "supported
-     packets" request.  If the target can answer this, it will tell us
-     which later probes to skip.  */
-  remote_query_supported ();
-
-  /* Next, we possibly activate noack mode.
-
-     If the QStartNoAckMode packet configuration is set to AUTO,
-     enable noack mode if the stub reported a wish for it with
-     qSupported.
-
-     If set to TRUE, then enable noack mode even if the stub didn't
-     report it in qSupported.  If the stub doesn't reply OK, the
-     session ends with an error.
-
-     If FALSE, then don't activate noack mode, regardless of what the
-     stub claimed should be the default with qSupported.  */
-
-  noack_config = &remote_protocol_packets[PACKET_QStartNoAckMode];
-
-  if (noack_config->detect == AUTO_BOOLEAN_TRUE
-      || (noack_config->detect == AUTO_BOOLEAN_AUTO
-	  && noack_config->support == PACKET_ENABLE))
-    {
-      putpkt ("QStartNoAckMode");
-      getpkt (&rs->buf, &rs->buf_size, 0);
-      if (packet_ok (rs->buf, noack_config) == PACKET_OK)
-	rs->noack_mode = 1;
-    }
-
-  /* Next, if the target can specify a description, read it.  We do
-     this before anything involving memory or registers.  */
-  target_find_description ();
-
   if (target_async_permitted)
     {
       /* With this target we start out by owning the terminal.  */
@@ -2964,7 +2982,10 @@ remote_open_1 (char *name, int from_tty,
     ex = catch_exception (uiout, remote_start_remote, &args, RETURN_MASK_ALL);
     if (ex.reason < 0)
       {
-	pop_target ();
+	/* Pop the partially set up target - unless something else did
+	   already before throwing the exception.  */
+	if (remote_desc != NULL)
+	  pop_target ();
 	if (target_async_permitted)
 	  wait_forever_enabled_p = 1;
 	throw_exception (ex);
@@ -2973,20 +2994,6 @@ remote_open_1 (char *name, int from_tty,
 
   if (target_async_permitted)
     wait_forever_enabled_p = 1;
-
-  if (extended_p)
-    {
-      /* Tell the remote that we are using the extended protocol.  */
-      putpkt ("!");
-      getpkt (&rs->buf, &rs->buf_size, 0);
-    }
-
-  /* If we connected to a live target, do some additional setup.  */
-  if (target_has_execution)
-    {
-      if (exec_bfd) 	/* No use without an exec file.  */
-	remote_check_symbols (symfile_objfile);
-    }
 }
 
 /* This takes a program previously attached to and detaches it.  After
Index: src/gdb/target.c
===================================================================
--- src.orig/gdb/target.c	2008-10-09 03:21:50.000000000 +0100
+++ src/gdb/target.c	2008-10-09 03:26:18.000000000 +0100
@@ -746,6 +746,10 @@ unpush_target (struct target_ops *t)
   struct target_ops **cur;
   struct target_ops *tmp;
 
+  if (t->to_stratum == dummy_stratum)
+    internal_error (__FILE__, __LINE__,
+		    "Attempt to unpush the dummy target");
+
   /* Look for the specified target.  Note that we assume that a target
      can only occur once in the target stack. */
 

Re: [commit] remote: make the whole connection setup exception safe

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Sorry, this broke `target extended-remote' if the stub reports that
it isn't controlling any process yet (gdbserver --multi).  I missed
that there's a return statement in the middle of remote_start_remote,
which makes us not send '!' in that case.

remote_start_remote:
  if (rs->buf[0] == 'W' || rs->buf[0] == 'X')
    {
      if (args->extended_p)
	{
	  /* We're connected, but not running.  Drop out before we
	     call start_remote.  */
	  target_mark_exited (args->target);
	  return;
	}

Fixed by just sending '!' earlier, like below.  Checked in.

We should have a test for extended/persistent connections.  Let me
cook something up.

-- 
Pedro Alves

[-- Attachment #2: extended.diff --]
[-- Type: text/x-diff, Size: 1325 bytes --]

2008-10-10  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* remote.c (remote_start_remote): Always tell the stub if we're in
	extended-remote.

---
 gdb/remote.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-10-09 15:57:48.000000000 +0100
+++ src/gdb/remote.c	2008-10-10 14:58:30.000000000 +0100
@@ -2419,6 +2419,13 @@ remote_start_remote (struct ui_out *uiou
 	rs->noack_mode = 1;
     }
 
+  if (args->extended_p)
+    {
+      /* Tell the remote that we are using the extended protocol.  */
+      putpkt ("!");
+      getpkt (&rs->buf, &rs->buf_size, 0);
+    }
+
   /* Next, if the target can specify a description, read it.  We do
      this before anything involving memory or registers.  */
   target_find_description ();
@@ -2482,13 +2489,6 @@ remote_start_remote (struct ui_out *uiou
   immediate_quit--;
   start_remote (args->from_tty); /* Initialize gdb process mechanisms.  */
 
-  if (args->extended_p)
-    {
-      /* Tell the remote that we are using the extended protocol.  */
-      putpkt ("!");
-      getpkt (&rs->buf, &rs->buf_size, 0);
-    }
-
   /* If we connected to a live target, do some additional setup.  */
   if (target_has_execution)
     {