This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
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.
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