[MI] Segfault using 'interpreter-exec mi'

[MI] Segfault using 'interpreter-exec mi'

[Marc Khouzam]

Hi,

I got a segfault when using 'interpreter-exec mi' and getting an error result.
I believe I tracked it down to mi_parse().  From what I can see, we cannot
call error() from mi_parse() because it does not catch exceptions.

Note that the segfault does not happen in full MI mode, I think because
we are in the correct interpreter for output, however, the MI command
does not get the proper ^error and requires the user to enter a new line
to get the ^done.

Session, backtrace, and patch below.

> ./gdb
GNU gdb (GDB) 7.2.50.20101125-cvs
(gdb) interpreter-exec mi "-break-insert --thread a"
&"Invalid value for the '--thread' option\n"

&"\n"
Segmentation fault

(top-gdb) backtrace 
#0  0x080a002a in ui_file_put (file=0xa9, write=0x8113503 <do_write>, dest=0x85547e8) at ../../src/gdb/ui-file.c:195
#1  0x08113555 in mi_out_put (uiout=0x8555560, stream=0x85547e8) at ../../src/gdb/mi/mi-out.c:390
During symbol reading, unsupported tag: 'DW_TAG_const_type'.
#2  0x0811b290 in captured_mi_execute_command (uiout=0x8555560, data=0x85573a0) at ../../src/gdb/mi/mi-main.c:1850
#3  0x081ae4ae in catch_exception (uiout=0x8555560, func=0x811b054 <captured_mi_execute_command>, func_args=0x85573a0, mask=6)
    at ../../src/gdb/exceptions.c:468
#4  0x0811b390 in mi_execute_command (cmd=0x8554bf0 "", from_tty=1) at ../../src/gdb/mi/mi-main.c:1895
#5  0x081171c3 in mi_execute_command_wrapper (cmd=0x8554bf0 "") at ../../src/gdb/mi/mi-interp.c:262
#6  0x081b5c24 in gdb_readline2 (client_data=0x0) at ../../src/gdb/event-top.c:783
#7  0x081b5463 in stdin_event_handler (error=0, client_data=0x0) at ../../src/gdb/event-top.c:433
#8  0x081b414e in handle_file_event (data=...) at ../../src/gdb/event-loop.c:817
#9  0x081b375c in process_event () at ../../src/gdb/event-loop.c:399
#10 0x081b3820 in gdb_do_one_event (data=0x0) at ../../src/gdb/event-loop.c:464
#11 0x081ae66c in catch_errors (func=0x81b376a <gdb_do_one_event>, func_args=0x0, errstring=0x8386077 "", mask=6) at ../../src/gdb/exceptions.c:518
#12 0x0811f319 in tui_command_loop (data=0x0) at ../../src/gdb/tui/tui-interp.c:171
#13 0x081aed60 in current_interp_command_loop () at ../../src/gdb/interps.c:291
#14 0x0809042e in captured_command_loop (data=0x0) at ../../src/gdb/main.c:228
#15 0x081ae66c in catch_errors (func=0x8090423 <captured_command_loop>, func_args=0x0, errstring=0x8366fbf "", mask=6) at ../../src/gdb/exceptions.c:518
#16 0x0809130d in captured_main (data=0xbffff750) at ../../src/gdb/main.c:912
#17 0x081ae66c in catch_errors (func=0x8090464 <captured_main>, func_args=0xbffff750, errstring=0x8366fbf "", mask=6) at ../../src/gdb/exceptions.c:518
#18 0x08091343 in gdb_main (args=0xbffff750) at ../../src/gdb/main.c:921
#19 0x080901b3 in main (argc=1, argv=0xbffff814) at ../../src/gdb/gdb.c:34


The below patch removes the calls to error() and uses fprintf_unfiltered.
Because of the comment
/* FIXME: This should be a function call. */
I took the opportunity to make a method mi_parse_error().

No regressions.

Comments?

Thanks

Marc


2010-11-25  Marc Khouzam  <marc.khouzam@ericsson.com>

	* mi/mi-parse.c (vmi_parse_error, mi_parse_error): Added.
	(mi_parse): Call mi_parse_error instead of error.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-parse.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.c,v
retrieving revision 1.21
diff -u -r1.21 mi-parse.c
--- gdb/mi/mi-parse.c   17 May 2010 20:49:39 -0000      1.21
+++ gdb/mi/mi-parse.c   25 Nov 2010 20:14:40 -0000
@@ -223,6 +223,22 @@
   xfree (parse);
 }
 
+void
+vmi_parse_error (struct mi_parse *parse, const char *format, va_list args)
+{
+  vfprintf_unfiltered (raw_stdout, format, args);
+  mi_parse_free (parse);
+}
+
+void
+mi_parse_error (struct mi_parse *parse, const char *format, ...) 
+{
+  va_list args;
+
+  va_start (args, format);
+  vmi_parse_error (parse, format, args);
+  va_end (args);
+}
 
 struct mi_parse *
 mi_parse (char *cmd)
@@ -272,12 +288,10 @@
   parse->cmd = mi_lookup (parse->command);
   if (parse->cmd == NULL)
     {
-      /* FIXME: This should be a function call. */
-      fprintf_unfiltered
-       (raw_stdout,
+      mi_parse_error 
+       (parse,
         "%s^error,msg=\"Undefined MI command: %s\"\n",
         parse->token, parse->command);
-      mi_parse_free (parse);
       return NULL;
     }
 
@@ -312,24 +326,48 @@
       if (strncmp (chp, "--thread-group ", tgs) == 0)
        {
          if (parse->thread_group != -1)
-           error (_("Duplicate '--thread-group' option"));
+           {
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--thread-group' option\"\n",
+                parse->token);
+             return NULL;
+           }
          chp += tgs;
          if (*chp != 'i')
-           error (_("Invalid thread group id"));
+           {
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Invalid thread group id\"\n",
+                parse->token);
+             return NULL;
+           }
          chp += 1;
          parse->thread_group = strtol (chp, &chp, 10);
        }
       if (strncmp (chp, "--thread ", ts) == 0)
        {
          if (parse->thread != -1)
-           error (_("Duplicate '--thread' option"));
+           {
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--thread' option\"\n",
+                parse->token);
+             return NULL;
+           }
          chp += ts;
          parse->thread = strtol (chp, &chp, 10);
        }
       else if (strncmp (chp, "--frame ", fs) == 0)
        {
          if (parse->frame != -1)
-           error (_("Duplicate '--frame' option"));
+           {
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--frame' option\"\n",
+                parse->token);
+             return NULL;
+           }
          chp += fs;
          parse->frame = strtol (chp, &chp, 10);
        }
@@ -337,8 +375,13 @@
        break;
 
       if (*chp != '\0' && !isspace (*chp))
-       error (_("Invalid value for the '%s' option"),
-              start[2] == 't' ? "--thread" : "--frame");
+           {
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Invalid value for the '%s' option\"\n",
+                parse->token, start[2] == 't' ? "--thread" : "--frame");
+             return NULL;
+           }
       while (isspace (*chp))
        chp++;
     }
@@ -350,12 +393,10 @@
       mi_parse_argv (chp, parse);
       if (parse->argv == NULL)
        {
-         /* FIXME: This should be a function call. */
-         fprintf_unfiltered
-           (raw_stdout,
+         mi_parse_error
+           (parse,
             "%s^error,msg=\"Problem parsing arguments: %s %s\"\n",
             parse->token, parse->command, chp);
-         mi_parse_free (parse);
          return NULL;
        }
     }

Re: [MI] Segfault using 'interpreter-exec mi'

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> I got a segfault when using 'interpreter-exec mi' and getting an
Marc> error result.  I believe I tracked it down to mi_parse().  From
Marc> what I can see, we cannot call error() from mi_parse() because it
Marc> does not catch exceptions.

Marc> Note that the segfault does not happen in full MI mode, I think because
Marc> we are in the correct interpreter for output, however, the MI command
Marc> does not get the proper ^error and requires the user to enter a new line
Marc> to get the ^done.

Thanks for the patch.

Marc> The below patch removes the calls to error() and uses fprintf_unfiltered.
Marc> Because of the comment
Marc> /* FIXME: This should be a function call. */
Marc> I took the opportunity to make a method mi_parse_error().

I don't mind this approach, but I think it is probably better to just
change mi_parse to use exceptions like the rest of gdb.  Then the caller
can handle them, just like it does for exceptions occurring in the
actual MI command.

The reason I think this is better is that a rule like "this code cannot
call error" is reasonably difficult to enforce in gdb.

What do you think of that?

A quick nit about the patch itself.

Marc> +void
Marc> +vmi_parse_error (struct mi_parse *parse, const char *format, va_list args)

The new functions need introductory comments.

I would have made them both static.

Tom

RE: [MI] Segfault using 'interpreter-exec mi'

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Thursday, December 02, 2010 11:35 AM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [MI] Segfault using 'interpreter-exec mi'
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> I got a segfault when using 'interpreter-exec mi' and getting an
> Marc> error result.  I believe I tracked it down to mi_parse().  From
> Marc> what I can see, we cannot call error() from mi_parse() 
> because it
> Marc> does not catch exceptions.
> 
> Marc> Note that the segfault does not happen in full MI mode, 
> I think because
> Marc> we are in the correct interpreter for output, however, 
> the MI command
> Marc> does not get the proper ^error and requires the user to 
> enter a new line
> Marc> to get the ^done.
> 
> Thanks for the patch.
> 
> Marc> The below patch removes the calls to error() and uses 
> fprintf_unfiltered.
> Marc> Because of the comment
> Marc> /* FIXME: This should be a function call. */
> Marc> I took the opportunity to make a method mi_parse_error().
> 
> I don't mind this approach, but I think it is probably better to just
> change mi_parse to use exceptions like the rest of gdb.  Then 
> the caller
> can handle them, just like it does for exceptions occurring in the
> actual MI command.
> 
> The reason I think this is better is that a rule like "this 
> code cannot
> call error" is reasonably difficult to enforce in gdb.
> 
> What do you think of that?

I agree with you and I started coding such a patch.  Things
got a bit more complex than I expected though.  Mostly because
the error output of mi_parse() has to use some of the data that 
was actually parsed in mi_parse(), which lead me to have to 
play around with return values while handling exceptions, or
having to free some memory _after_ calling error(), which 
I didn't know how to do.

After a while, it started to feel a bit complicated of a fix for 
something that I'm hoping to push to the 7_2 branch.  Currently, 
not getting the proper ^error message from MI could lead the 
frontend into a pretty bad situation, which is why I think this
should be fixed in 7_2.

Could we have the brute force fix now, and leave the improvement
for later?

> A quick nit about the patch itself.
> 
> Marc> +void
> Marc> +vmi_parse_error (struct mi_parse *parse, const char 
> *format, va_list args)
> 
> The new functions need introductory comments.
> 
> I would have made them both static.

Here is the same patch incorporating your two comments.

Thanks

Marc


2010-12-06  Marc Khouzam  <marc.khouzam@ericsson.com>

	* mi/mi-parse.c (vmi_parse_error, mi_parse_error): Added.
	(mi_parse): Call mi_parse_error instead of error.


### Eclipse Workspace Patch 1.0
#P src
Index: gdb/mi/mi-parse.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.c,v
retrieving revision 1.22
diff -u -r1.22 mi-parse.c
--- gdb/mi/mi-parse.c   6 Dec 2010 14:16:43 -0000       1.22
+++ gdb/mi/mi-parse.c   6 Dec 2010 21:33:39 -0000
@@ -223,6 +223,26 @@
   xfree (parse);
 }
 
+/* Output the error message from the mi_parse method using
+   a variable parameter list, and deallocate the struct parse */
+static void
+vmi_parse_error (struct mi_parse *parse, const char *format, va_list args)
+{
+  vfprintf_unfiltered (raw_stdout, format, args);
+  mi_parse_free (parse);
+}
+
+/* Output an error message for the mi_parse method,
+   and deallocate the struct parse */
+static void
+mi_parse_error (struct mi_parse *parse, const char *format, ...) 
+{
+  va_list args;
+
+  va_start (args, format);
+  vmi_parse_error (parse, format, args);
+  va_end (args);
+}
 
 struct mi_parse *
 mi_parse (char *cmd)
@@ -272,12 +292,10 @@
   parse->cmd = mi_lookup (parse->command);
   if (parse->cmd == NULL)
     {
-      /* FIXME: This should be a function call. */
-      fprintf_unfiltered
-       (raw_stdout,
+      mi_parse_error 
+       (parse,
         "%s^error,msg=\"Undefined MI command: %s\"\n",
         parse->token, parse->command);
-      mi_parse_free (parse);
       return NULL;
     }
 
@@ -312,24 +330,51 @@
       if (strncmp (chp, "--thread-group ", tgs) == 0)
        {
          if (parse->thread_group != -1)
-           error (_("Duplicate '--thread-group' option"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--thread-group' option\"\n",
+                parse->token);
+             return NULL;
+           }   
+
          chp += tgs;
          if (*chp != 'i')
-           error (_("Invalid thread group id"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Invalid thread group id\"\n",
+                parse->token);
+             return NULL;
+           }
          chp += 1;
          parse->thread_group = strtol (chp, &chp, 10);
        }
       else if (strncmp (chp, "--thread ", ts) == 0)
        {
          if (parse->thread != -1)
-           error (_("Duplicate '--thread' option"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--thread' option\"\n",
+                parse->token);
+             return NULL;
+           }   
+
          chp += ts;
          parse->thread = strtol (chp, &chp, 10);
        }
       else if (strncmp (chp, "--frame ", fs) == 0)
        {
          if (parse->frame != -1)
-           error (_("Duplicate '--frame' option"));
+           {   
+             mi_parse_error
+               (parse,
+                "%s^error,msg=\"Duplicate '--frame' option\"\n",
+                parse->token);
+             return NULL;
+           }
+
          chp += fs;
          parse->frame = strtol (chp, &chp, 10);
        }
@@ -337,8 +382,14 @@
        break;
 
       if (*chp != '\0' && !isspace (*chp))
-       error (_("Invalid value for the '%s' option"),
-              start[2] == 't' ? "--thread" : "--frame");
+       {
+         mi_parse_error
+           (parse,
+            "%s^error,msg=\"Invalid value for the '%s' option\"\n",
+            parse->token, start[2] == 't' ? "--thread" : "--frame");
+         return NULL;
+       }
+
       while (isspace (*chp))
        chp++;
     }
@@ -350,12 +401,10 @@
       mi_parse_argv (chp, parse);
       if (parse->argv == NULL)
        {
-         /* FIXME: This should be a function call. */
-         fprintf_unfiltered
-           (raw_stdout,
+         mi_parse_error
+           (parse,
             "%s^error,msg=\"Problem parsing arguments: %s %s\"\n",
             parse->token, parse->command, chp);
-         mi_parse_free (parse);
          return NULL;
        }
     }

Re: [MI] Segfault using 'interpreter-exec mi'

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> Mostly because the error output of mi_parse() has to use some of
Marc> the data that was actually parsed in mi_parse(), which lead me to
Marc> have to play around with return values while handling exceptions,
Marc> or having to free some memory _after_ calling error(), which I
Marc> didn't know how to do.

You can install an 'xfree' cleanup before calling error.

Marc> Could we have the brute force fix now, and leave the improvement
Marc> for later?

I will look at it tomorrow.

Tom

Re: [MI] Segfault using 'interpreter-exec mi'

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> I agree with you and I started coding such a patch.  Things
Marc> got a bit more complex than I expected though.  Mostly because
Marc> the error output of mi_parse() has to use some of the data that 
Marc> was actually parsed in mi_parse(), which lead me to have to 
Marc> play around with return values while handling exceptions, or
Marc> having to free some memory _after_ calling error(), which 
Marc> I didn't know how to do.

Ok -- I wrote it myself and now I actually understand.  mi_parse
extracts the token from the MI command line, and you need this token to
report the error.  But, if you write the patch the obvious way, the
token will be freed too soon.

Here is the patch I came up with.  Let me know what you think.

I am running it through the regression tester.

Marc> Could we have the brute force fix now, and leave the improvement
Marc> for later?

As a general rule I think it is perfectly ok for one patch to go on a
branch and a different one to go in mainline.

In this particular case, I think the appended is probably safe enough
for 7.2 as well, but I'd appreciate a sanity check on that.

Tom

2010-12-07  Tom Tromey  <tromey@redhat.com>

	* mi/mi-parse.h (mi_parse): Update.
	* mi/mi-parse.c (mi_parse_cleanup): New function.
	(mi_parse): Add 'token' argument.  Throw exception on error.
	* mi/mi-main.c (mi_print_exception): New function.
	(mi_execute_command): Use mi_print_exception.  Catch exceptions
	from mi_parse.

2010-12-07  Tom Tromey  <tromey@redhat.com>

	* gdb.base/interp.exp: Add regression test.

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 3343c03..48e907f 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1865,11 +1865,26 @@ captured_mi_execute_command (struct ui_out *uiout, void *data)
   return;
 }
 
+/* Print a gdb exception to the MI output stream.  */
+
+static void
+mi_print_exception (const char *token, struct gdb_exception exception)
+{
+  fputs_unfiltered (token, raw_stdout);
+  fputs_unfiltered ("^error,msg=\"", raw_stdout);
+  if (exception.message == NULL)
+    fputs_unfiltered ("unknown error", raw_stdout);
+  else
+    fputstr_unfiltered (exception.message, '"', raw_stdout);
+  fputs_unfiltered ("\"\n", raw_stdout);
+}
 
 void
 mi_execute_command (char *cmd, int from_tty)
 {
-  struct mi_parse *command;
+  char *token;
+  struct mi_parse *command = NULL;
+  volatile struct gdb_exception exception;
 
   /* This is to handle EOF (^D). We just quit gdb.  */
   /* FIXME: we should call some API function here.  */
@@ -1878,13 +1893,22 @@ mi_execute_command (char *cmd, int from_tty)
 
   target_log_command (cmd);
 
-  command = mi_parse (cmd);
-
-  if (command != NULL)
+  TRY_CATCH (exception, RETURN_MASK_ALL)
+    {
+      command = mi_parse (cmd, &token);
+    }
+  if (exception.reason < 0)
+    {
+      mi_print_exception (token, exception);
+      xfree (token);
+    }
+  else
     {
       struct gdb_exception result;
       ptid_t previous_ptid = inferior_ptid;
 
+      command->token = token;
+
       if (do_timings)
 	{
 	  command->cmd_start = (struct mi_timestamp *)
@@ -1898,13 +1922,7 @@ mi_execute_command (char *cmd, int from_tty)
 	{
 	  /* The command execution failed and error() was called
 	     somewhere.  */
-	  fputs_unfiltered (command->token, raw_stdout);
-	  fputs_unfiltered ("^error,msg=\"", raw_stdout);
-	  if (result.message == NULL)
-	    fputs_unfiltered ("unknown error", raw_stdout);
-	  else
-	    fputstr_unfiltered (result.message, '"', raw_stdout);
-	  fputs_unfiltered ("\"\n", raw_stdout);
+	  mi_print_exception (command->token, result);
 	  mi_out_rewind (uiout);
 	}
 
diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
index 4541b66..774d368 100644
--- a/gdb/mi/mi-parse.c
+++ b/gdb/mi/mi-parse.c
@@ -223,12 +223,20 @@ mi_parse_free (struct mi_parse *parse)
   xfree (parse);
 }
 
+/* A cleanup that calls mi_parse_free.  */
+
+static void
+mi_parse_cleanup (void *arg)
+{
+  mi_parse_free (arg);
+}
 
 struct mi_parse *
-mi_parse (char *cmd)
+mi_parse (char *cmd, char **token)
 {
   char *chp;
   struct mi_parse *parse = XMALLOC (struct mi_parse);
+  struct cleanup *cleanup;
 
   memset (parse, 0, sizeof (*parse));
   parse->all = 0;
@@ -236,6 +244,8 @@ mi_parse (char *cmd)
   parse->thread = -1;
   parse->frame = -1;
 
+  cleanup = make_cleanup (mi_parse_cleanup, parse);
+
   /* Before starting, skip leading white space. */
   while (isspace (*cmd))
     cmd++;
@@ -243,9 +253,9 @@ mi_parse (char *cmd)
   /* Find/skip any token and then extract it. */
   for (chp = cmd; *chp >= '0' && *chp <= '9'; chp++)
     ;
-  parse->token = xmalloc ((chp - cmd + 1) * sizeof (char *));
-  memcpy (parse->token, cmd, (chp - cmd));
-  parse->token[chp - cmd] = '\0';
+  *token = xmalloc ((chp - cmd + 1) * sizeof (char *));
+  memcpy (*token, cmd, (chp - cmd));
+  (*token)[chp - cmd] = '\0';
 
   /* This wasn't a real MI command.  Return it as a CLI_COMMAND. */
   if (*chp != '-')
@@ -254,6 +264,9 @@ mi_parse (char *cmd)
 	chp++;
       parse->command = xstrdup (chp);
       parse->op = CLI_COMMAND;
+
+      discard_cleanups (cleanup);
+
       return parse;
     }
 
@@ -271,15 +284,7 @@ mi_parse (char *cmd)
   /* Find the command in the MI table. */
   parse->cmd = mi_lookup (parse->command);
   if (parse->cmd == NULL)
-    {
-      /* FIXME: This should be a function call. */
-      fprintf_unfiltered
-	(raw_stdout,
-	 "%s^error,msg=\"Undefined MI command: %s\"\n",
-	 parse->token, parse->command);
-      mi_parse_free (parse);
-      return NULL;
-    }
+    error (_("Undefined MI command: %s"), parse->command);
 
   /* Skip white space following the command. */
   while (isspace (*chp))
@@ -349,15 +354,7 @@ mi_parse (char *cmd)
     {
       mi_parse_argv (chp, parse);
       if (parse->argv == NULL)
-	{
-	  /* FIXME: This should be a function call. */
-	  fprintf_unfiltered
-	    (raw_stdout,
-	     "%s^error,msg=\"Problem parsing arguments: %s %s\"\n",
-	     parse->token, parse->command, chp);
-	  mi_parse_free (parse);
-	  return NULL;
-	}
+	error (_("Problem parsing arguments: %s %s"), parse->command, chp);
     }
 
   /* FIXME: DELETE THIS */
@@ -366,6 +363,8 @@ mi_parse (char *cmd)
   if (parse->cmd->cli.cmd != NULL)
     parse->args = xstrdup (chp);
 
+  discard_cleanups (cleanup);
+
   /* Fully parsed. */
   parse->op = MI_COMMAND;
   return parse;
diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
index 3c6cd9a..6d0f53c 100644
--- a/gdb/mi/mi-parse.h
+++ b/gdb/mi/mi-parse.h
@@ -52,13 +52,15 @@ struct mi_parse
     int frame;
   };
 
-/* Attempts to parse CMD returning a ``struct mi_command''.  If CMD is
-   invalid, an error mesage is reported (MI format) and NULL is
-   returned. For a CLI_COMMAND, COMMAND, TOKEN and OP are initialized.
-   For an MI_COMMAND COMMAND, TOKEN, ARGS and OP are
-   initialized. Un-initialized fields are zero. */
+/* Attempts to parse CMD returning a ``struct mi_parse''.  If CMD is
+   invalid, an exception is thrown.  For an MI_COMMAND COMMAND, ARGS
+   and OP are initialized.  Un-initialized fields are zero.  *TOKEN is
+   set to the token, even if an exception is thrown.  It is allocated
+   with xmalloc; it must either be freed with xfree, or assigned to
+   the TOKEN field of the resultant mi_parse object, to be freed by
+   mi_parse_free.  */
 
-extern struct mi_parse *mi_parse (char *cmd);
+extern struct mi_parse *mi_parse (char *cmd, char **token);
 
 /* Free a command returned by mi_parse_command. */
 
diff --git a/gdb/testsuite/gdb.base/interp.exp b/gdb/testsuite/gdb.base/interp.exp
index ece2552..a923f1a 100644
--- a/gdb/testsuite/gdb.base/interp.exp
+++ b/gdb/testsuite/gdb.base/interp.exp
@@ -33,4 +33,15 @@ gdb_test_multiple $cmd $cmd {
 }
 gdb_test "interpreter-exec console \"show version\"" "GNU gdb .*"
 
+# Regression test for crash when an exception occurs in mi_parse.
+gdb_test_multiple "interpreter-exec mi \"-break-insert --thread a\"" \
+    "regression test for mi_parse crash" {
+	-re ".error,msg=.Invalid value for the '--thread' option.\r\n$gdb_prompt " {
+	    pass "$cmd"
+	    gdb_expect 1 {
+		-re "\r\n$gdb_prompt $" { }
+	    }
+	}
+    }
+
 gdb_exit

Re: [MI] Segfault using 'interpreter-exec mi'

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> Here is the patch I came up with.  Let me know what you think.

Tom> I am running it through the regression tester.

It passed.

Tom

RE: [MI] Segfault using 'interpreter-exec mi'

[Marc Khouzam]

 

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Tuesday, December 07, 2010 3:33 PM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [MI] Segfault using 'interpreter-exec mi'
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> I agree with you and I started coding such a patch.  Things
> Marc> got a bit more complex than I expected though.  Mostly because
> Marc> the error output of mi_parse() has to use some of the data that 
> Marc> was actually parsed in mi_parse(), which lead me to have to 
> Marc> play around with return values while handling exceptions, or
> Marc> having to free some memory _after_ calling error(), which 
> Marc> I didn't know how to do.
> 
> Ok -- I wrote it myself and now I actually understand.  mi_parse
> extracts the token from the MI command line, and you need 
> this token to
> report the error.  But, if you write the patch the obvious way, the
> token will be freed too soon.
> 
> Here is the patch I came up with.  Let me know what you think.

Thanks!
I was stuck trying to use catch_exception(), which was not working
because I needed the return value of mi_parse(), but I see that
using TRY_CATCH was the way to go.

> I am running it through the regression tester.
> 
> Marc> Could we have the brute force fix now, and leave the improvement
> Marc> for later?
> 
> As a general rule I think it is perfectly ok for one patch to go on a
> branch and a different one to go in mainline.
> 
> In this particular case, I think the appended is probably safe enough
> for 7.2 as well, but I'd appreciate a sanity check on that.

I haven't actually run it yet, I'll do that tonight, but I have
a single question about the patch, please see below.

Thanks a lot for taking the time for this!

Marc

> 
> Tom
> 
> 2010-12-07  Tom Tromey  <tromey@redhat.com>
> 
> 	* mi/mi-parse.h (mi_parse): Update.
> 	* mi/mi-parse.c (mi_parse_cleanup): New function.
> 	(mi_parse): Add 'token' argument.  Throw exception on error.
> 	* mi/mi-main.c (mi_print_exception): New function.
> 	(mi_execute_command): Use mi_print_exception.  Catch exceptions
> 	from mi_parse.
> 
> 2010-12-07  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.base/interp.exp: Add regression test.
> 
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 3343c03..48e907f 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1865,11 +1865,26 @@ captured_mi_execute_command (struct 
> ui_out *uiout, void *data)
>    return;
>  }
>  
> +/* Print a gdb exception to the MI output stream.  */
> +
> +static void
> +mi_print_exception (const char *token, struct gdb_exception 
> exception)
> +{
> +  fputs_unfiltered (token, raw_stdout);
> +  fputs_unfiltered ("^error,msg=\"", raw_stdout);
> +  if (exception.message == NULL)
> +    fputs_unfiltered ("unknown error", raw_stdout);
> +  else
> +    fputstr_unfiltered (exception.message, '"', raw_stdout);
> +  fputs_unfiltered ("\"\n", raw_stdout);
> +}
>  
>  void
>  mi_execute_command (char *cmd, int from_tty)
>  {
> -  struct mi_parse *command;
> +  char *token;
> +  struct mi_parse *command = NULL;
> +  volatile struct gdb_exception exception;
>  
>    /* This is to handle EOF (^D). We just quit gdb.  */
>    /* FIXME: we should call some API function here.  */
> @@ -1878,13 +1893,22 @@ mi_execute_command (char *cmd, int from_tty)
>  
>    target_log_command (cmd);
>  
> -  command = mi_parse (cmd);
> -
> -  if (command != NULL)
> +  TRY_CATCH (exception, RETURN_MASK_ALL)
> +    {
> +      command = mi_parse (cmd, &token);
> +    }
> +  if (exception.reason < 0)
> +    {
> +      mi_print_exception (token, exception);
> +      xfree (token);

Here, do we need an call to
mi_out_rewind (uiout)?
I don't know what it does, but I noticed it was being
called below after mi_print_exception()

> +    }
> +  else
>      {
>        struct gdb_exception result;
>        ptid_t previous_ptid = inferior_ptid;
>  
> +      command->token = token;
> +
>        if (do_timings)
>  	{
>  	  command->cmd_start = (struct mi_timestamp *)
> @@ -1898,13 +1922,7 @@ mi_execute_command (char *cmd, int from_tty)
>  	{
>  	  /* The command execution failed and error() was called
>  	     somewhere.  */
> -	  fputs_unfiltered (command->token, raw_stdout);
> -	  fputs_unfiltered ("^error,msg=\"", raw_stdout);
> -	  if (result.message == NULL)
> -	    fputs_unfiltered ("unknown error", raw_stdout);
> -	  else
> -	    fputstr_unfiltered (result.message, '"', raw_stdout);
> -	  fputs_unfiltered ("\"\n", raw_stdout);
> +	  mi_print_exception (command->token, result);
>  	  mi_out_rewind (uiout);
>  	}
>  
> diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
> index 4541b66..774d368 100644
> --- a/gdb/mi/mi-parse.c
> +++ b/gdb/mi/mi-parse.c
> @@ -223,12 +223,20 @@ mi_parse_free (struct mi_parse *parse)
>    xfree (parse);
>  }
>  
> +/* A cleanup that calls mi_parse_free.  */
> +
> +static void
> +mi_parse_cleanup (void *arg)
> +{
> +  mi_parse_free (arg);
> +}
>  
>  struct mi_parse *
> -mi_parse (char *cmd)
> +mi_parse (char *cmd, char **token)
>  {
>    char *chp;
>    struct mi_parse *parse = XMALLOC (struct mi_parse);
> +  struct cleanup *cleanup;
>  
>    memset (parse, 0, sizeof (*parse));
>    parse->all = 0;
> @@ -236,6 +244,8 @@ mi_parse (char *cmd)
>    parse->thread = -1;
>    parse->frame = -1;
>  
> +  cleanup = make_cleanup (mi_parse_cleanup, parse);
> +
>    /* Before starting, skip leading white space. */
>    while (isspace (*cmd))
>      cmd++;
> @@ -243,9 +253,9 @@ mi_parse (char *cmd)
>    /* Find/skip any token and then extract it. */
>    for (chp = cmd; *chp >= '0' && *chp <= '9'; chp++)
>      ;
> -  parse->token = xmalloc ((chp - cmd + 1) * sizeof (char *));
> -  memcpy (parse->token, cmd, (chp - cmd));
> -  parse->token[chp - cmd] = '\0';
> +  *token = xmalloc ((chp - cmd + 1) * sizeof (char *));
> +  memcpy (*token, cmd, (chp - cmd));
> +  (*token)[chp - cmd] = '\0';
>  
>    /* This wasn't a real MI command.  Return it as a CLI_COMMAND. */
>    if (*chp != '-')
> @@ -254,6 +264,9 @@ mi_parse (char *cmd)
>  	chp++;
>        parse->command = xstrdup (chp);
>        parse->op = CLI_COMMAND;
> +
> +      discard_cleanups (cleanup);
> +
>        return parse;
>      }
>  
> @@ -271,15 +284,7 @@ mi_parse (char *cmd)
>    /* Find the command in the MI table. */
>    parse->cmd = mi_lookup (parse->command);
>    if (parse->cmd == NULL)
> -    {
> -      /* FIXME: This should be a function call. */
> -      fprintf_unfiltered
> -	(raw_stdout,
> -	 "%s^error,msg=\"Undefined MI command: %s\"\n",
> -	 parse->token, parse->command);
> -      mi_parse_free (parse);
> -      return NULL;
> -    }
> +    error (_("Undefined MI command: %s"), parse->command);
>  
>    /* Skip white space following the command. */
>    while (isspace (*chp))
> @@ -349,15 +354,7 @@ mi_parse (char *cmd)
>      {
>        mi_parse_argv (chp, parse);
>        if (parse->argv == NULL)
> -	{
> -	  /* FIXME: This should be a function call. */
> -	  fprintf_unfiltered
> -	    (raw_stdout,
> -	     "%s^error,msg=\"Problem parsing arguments: %s %s\"\n",
> -	     parse->token, parse->command, chp);
> -	  mi_parse_free (parse);
> -	  return NULL;
> -	}
> +	error (_("Problem parsing arguments: %s %s"), 
> parse->command, chp);
>      }
>  
>    /* FIXME: DELETE THIS */
> @@ -366,6 +363,8 @@ mi_parse (char *cmd)
>    if (parse->cmd->cli.cmd != NULL)
>      parse->args = xstrdup (chp);
>  
> +  discard_cleanups (cleanup);
> +
>    /* Fully parsed. */
>    parse->op = MI_COMMAND;
>    return parse;
> diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
> index 3c6cd9a..6d0f53c 100644
> --- a/gdb/mi/mi-parse.h
> +++ b/gdb/mi/mi-parse.h
> @@ -52,13 +52,15 @@ struct mi_parse
>      int frame;
>    };
>  
> -/* Attempts to parse CMD returning a ``struct mi_command''.  
> If CMD is
> -   invalid, an error mesage is reported (MI format) and NULL is
> -   returned. For a CLI_COMMAND, COMMAND, TOKEN and OP are 
> initialized.
> -   For an MI_COMMAND COMMAND, TOKEN, ARGS and OP are
> -   initialized. Un-initialized fields are zero. */
> +/* Attempts to parse CMD returning a ``struct mi_parse''.  If CMD is
> +   invalid, an exception is thrown.  For an MI_COMMAND COMMAND, ARGS
> +   and OP are initialized.  Un-initialized fields are zero.  
> *TOKEN is
> +   set to the token, even if an exception is thrown.  It is allocated
> +   with xmalloc; it must either be freed with xfree, or assigned to
> +   the TOKEN field of the resultant mi_parse object, to be freed by
> +   mi_parse_free.  */
>  
> -extern struct mi_parse *mi_parse (char *cmd);
> +extern struct mi_parse *mi_parse (char *cmd, char **token);
>  
>  /* Free a command returned by mi_parse_command. */
>  
> diff --git a/gdb/testsuite/gdb.base/interp.exp 
> b/gdb/testsuite/gdb.base/interp.exp
> index ece2552..a923f1a 100644
> --- a/gdb/testsuite/gdb.base/interp.exp
> +++ b/gdb/testsuite/gdb.base/interp.exp
> @@ -33,4 +33,15 @@ gdb_test_multiple $cmd $cmd {
>  }
>  gdb_test "interpreter-exec console \"show version\"" "GNU gdb .*"
>  
> +# Regression test for crash when an exception occurs in mi_parse.
> +gdb_test_multiple "interpreter-exec mi \"-break-insert 
> --thread a\"" \
> +    "regression test for mi_parse crash" {
> +	-re ".error,msg=.Invalid value for the '--thread' 
> option.\r\n$gdb_prompt " {
> +	    pass "$cmd"
> +	    gdb_expect 1 {
> +		-re "\r\n$gdb_prompt $" { }
> +	    }
> +	}
> +    }
> +
>  gdb_exit
> 

Re: [MI] Segfault using 'interpreter-exec mi'

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> Thanks!
Marc> I was stuck trying to use catch_exception(), which was not working
Marc> because I needed the return value of mi_parse(), but I see that
Marc> using TRY_CATCH was the way to go.

Yeah.  For future reference, you can do that by having a trampoline
function that has different arguments.  I find TRY_CATCH a lot simpler
though.

>> +  if (exception.reason < 0)
>> +    {
>> +      mi_print_exception (token, exception);
>> +      xfree (token);

Marc> Here, do we need an call to
Marc> mi_out_rewind (uiout)?
Marc> I don't know what it does, but I noticed it was being
Marc> called below after mi_print_exception()

It is unclear to me what mi_out_rewind does.
There don't seem to be any comments describing this function.

So, I chose to implement this in a way that is compatible with the
earlier code, which did not call mi_out_rewind on this path.

Tom

RE: [MI] Segfault using 'interpreter-exec mi'

[Marc Khouzam]

From: Tom Tromey [tromey@redhat.com]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> Thanks!
Marc> I was stuck trying to use catch_exception(), which was not working
Marc> because I needed the return value of mi_parse(), but I see that
Marc> using TRY_CATCH was the way to go.

> Yeah.  For future reference, you can do that by having a trampoline
> function that has different arguments.  I find TRY_CATCH a lot simpler
> though.

>> +  if (exception.reason < 0)
>> +    {
>> +      mi_print_exception (token, exception);
>> +      xfree (token);

Marc> Here, do we need an call to
Marc> mi_out_rewind (uiout)?
Marc> I don't know what it does, but I noticed it was being
Marc> called below after mi_print_exception()

> It is unclear to me what mi_out_rewind does.
> There don't seem to be any comments describing this function.
> 
> So, I chose to implement this in a way that is compatible with the
> earlier code, which did not call mi_out_rewind on this path.

Thanks Tom, I tried the patch and it does exactly what I was hoping for.

I did notice one issue but it is not with your patch, but something
I missed in the fix from
http://sourceware.org/ml/gdb-patches/2010-12/msg00049.html

I will post a fix for that in 2 minutes.

Thanks again

Marc

Re: [MI] Segfault using 'interpreter-exec mi'

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> Thanks Tom, I tried the patch and it does exactly what I was hoping for.

Thanks.  I am going to check this in on the trunk and the 7.2 branch.

Tom