From patchwork Fri May 4 15:19:18 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 10380853 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 7B2BB6037D for ; Fri, 4 May 2018 15:20:36 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4C40A294FF for ; Fri, 4 May 2018 15:20:36 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4775429573; Fri, 4 May 2018 15:20:36 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id B620F29575 for ; Fri, 4 May 2018 15:20:21 +0000 (UTC) Received: from localhost ([::1]:34818 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fEcVN-0003In-74 for patchwork-qemu-devel@patchwork.kernel.org; Fri, 04 May 2018 11:20:17 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fEcUd-0002wK-U2 for qemu-devel@nongnu.org; Fri, 04 May 2018 11:19:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fEcUZ-0004Ys-JO for qemu-devel@nongnu.org; Fri, 04 May 2018 11:19:31 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:51848 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fEcUZ-0004Yc-Ay for qemu-devel@nongnu.org; Fri, 04 May 2018 11:19:27 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2544F11E11F; Fri, 4 May 2018 15:19:23 +0000 (UTC) Received: from [10.10.120.174] (ovpn-120-174.rdu2.redhat.com [10.10.120.174]) by smtp.corp.redhat.com (Postfix) with ESMTP id CA15F1C701; Fri, 4 May 2018 15:19:18 +0000 (UTC) To: Collin Walling , qemu-devel@nongnu.org References: <1525445354-16233-1-git-send-email-walling@linux.ibm.com> From: Eric Blake Organization: Red Hat, Inc. Message-ID: <7e9e9dfd-795b-47f2-453a-59bf65f28229@redhat.com> Date: Fri, 4 May 2018 10:19:18 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1525445354-16233-1-git-send-email-walling@linux.ibm.com> Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 04 May 2018 15:19:23 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 04 May 2018 15:19:23 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'eblake@redhat.com' RCPT:'' X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.187.233.73 Subject: Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 05/04/2018 09:49 AM, Collin Walling wrote: > When a user incorrectly provides an hmp command, an error response will be > printed that prompts the user to try "help ". However, when > the command contains multiple parts e.g. "info skeys", only the last > whitespace delimited string will be reported (in this example "info" will > be dropped and the message will read "Try "help skeys" for more information", > which is incorrect). What's the exact formula for reproducing this? I tried: $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic --monitor stdio QEMU 2.12.50 monitor - type 'help' for more information (qemu) info skeys unknown command: 'info skeys' Oh, I see now: (qemu) info uuid blah uuid: extraneous characters at the end of line Try "help uuid" for more information (qemu) help uuid (qemu) You'll want to update your commit message to document something that is reproducible (you may be adding an 'info skeys', but until that is in, it doesn't make a good example). > > Let's correct this by capturing the full name of the command as we recurse > through the function monitor_parse_command. > > Reported-by: Mikhail Fokin > Signed-off-by: Collin Walling > --- > monitor.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 39f8ee1..d4844b4 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2964,7 +2964,8 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, > static const mon_cmd_t *monitor_parse_command(Monitor *mon, > const char *cmdp_start, > const char **cmdp, > - mon_cmd_t *table) > + mon_cmd_t *table, > + char *fullname) Umm, how is fullname any better than cmdp_start that we already have? > { > const char *p; > const mon_cmd_t *cmd; > @@ -2987,10 +2988,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > p++; > } > > + strncat(fullname, cmdname, strlen(cmdname)); gcc 8 is pickier about using strncat() [perhaps too picky - see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85602], but it is generally NOT the function you want to be using. > + > *cmdp = p; > /* search sub command */ > if (cmd->sub_table != NULL && *p != '\0') { > - return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table); > + strncat(fullname, " ", 1); > + return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table, > + fullname); See, you're reconstructing a command into fullname, which already matches the original command in cmdp_start, so I see no reason to change the signature. > } > > return cmd; > @@ -3371,10 +3376,12 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline) > { > QDict *qdict; > const mon_cmd_t *cmd; > + char fullname[256]; EWWW. Don't do that. You are just ASKING for a buffer overflow exploit that prints the wrong thing or causes a security hole, when I intentionally type a super-long garbage command into HMP. > > trace_handle_hmp_command(mon, cmdline); > > - cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table); > + cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table, > + fullname); Note that even without your patch, this call updates 'cmdline' to point to the position within the original string (although that position has already skipped spaces). > if (!cmd) { > return; > } > @@ -3382,7 +3389,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline) > qdict = monitor_parse_arguments(mon, &cmdline, cmd); > if (!qdict) { > monitor_printf(mon, "Try \"help %s\" for more information\n", > - cmd->name); > + fullname); So rather than trying to reconstruct a string, you could reuse what you already have. This is a shorter patch that I think accomplishes the same goal: @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline) qdict = monitor_parse_arguments(mon, &cmdline, cmd); if (!qdict) { - monitor_printf(mon, "Try \"help %s\" for more information\n", - cmd->name); + while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) { + cmdline--; + } + monitor_printf(mon, "Try \"help %.*s\" for more information\n", + (int)(cmdline - cmd_start), cmd_start); return; } diff --git i/monitor.c w/monitor.c index 39f8ee17ba7..38736b3a20d 100644 --- i/monitor.c +++ w/monitor.c @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline) { QDict *qdict; const mon_cmd_t *cmd; + const char *cmd_start = cmdline; trace_handle_hmp_command(mon, cmdline);