From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3150 invoked by alias); 7 Dec 2010 20:32:51 -0000 Received: (qmail 3128 invoked by uid 22791); 7 Dec 2010 20:32:50 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_CP,TW_RG,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 07 Dec 2010 20:32:44 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oB7KWelJ016186 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 Dec 2010 15:32:41 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id oB7KWeRq022428; Tue, 7 Dec 2010 15:32:40 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id oB7KWdRg031571; Tue, 7 Dec 2010 15:32:39 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 3557C378071; Tue, 7 Dec 2010 13:32:39 -0700 (MST) From: Tom Tromey To: Marc Khouzam Cc: "'gdb-patches\@sourceware.org'" Subject: Re: [MI] Segfault using 'interpreter-exec mi' References: Date: Tue, 07 Dec 2010 20:32:00 -0000 In-Reply-To: (Marc Khouzam's message of "Mon, 6 Dec 2010 16:34:32 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-12/txt/msg00075.txt.bz2 >>>>> "Marc" == Marc Khouzam 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 * 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 * 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