This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [MI] Segfault using 'interpreter-exec mi'
> -----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
>