From patchwork Thu Aug 2 21:26:12 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Philippe Waroquiers X-Patchwork-Id: 28744 Received: (qmail 99235 invoked by alias); 2 Aug 2018 21:26:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 99205 invoked by uid 89); 2 Aug 2018 21:26:26 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=globally, Pedro, gdb_assert X-HELO: mailsec113.isp.belgacom.be Received: from mailsec113.isp.belgacom.be (HELO mailsec113.isp.belgacom.be) (195.238.20.109) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Aug 2018 21:26:24 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1533245184; x=1564781184; h=from:to:cc:subject:date:message-id:in-reply-to: references; bh=cz8g0hoSgqGmcCKkhF6jcKKJ/KGF2P1F2cGEJA6vgIQ=; b=IaUVz4taNqciIq7P59muL6okUqIfyqYWaaWY+GIhH7DW9iW/pv8o46op Sa3Hbh2ui+3H+NZInVr5UimcnU1kqg==; Received: from 145.43-64-87.adsl-dyn.isp.belgacom.be (HELO md.home) ([87.64.43.145]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 02 Aug 2018 23:26:22 +0200 From: Philippe Waroquiers To: gdb-patches@sourceware.org Cc: Philippe Waroquiers Subject: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing Date: Thu, 2 Aug 2018 23:26:12 +0200 Message-Id: <20180802212613.29813-2-philippe.waroquiers@skynet.be> In-Reply-To: <20180802212613.29813-1-philippe.waroquiers@skynet.be> References: <20180802212613.29813-1-philippe.waroquiers@skynet.be> X-IsSubscribed: yes When setting commands for several breakpoints (such as with 'commands 1 2'), the '1 2' is passed to commands_command_1 as const char* arg. This arg can however be freed, as this is in a just read input line that might be freed by the call to read_command_lines. This patch fixes the problem by ensuring that arg is always a locally allocated string managed via the std::string new_arg. Note that this was the logic before the regression was introduced : the use after free was introduced when (partially) undoing the patch done by Pedro in 896b6bda6904765f36692d76a37b99c0412ca9ae. Note that such problem could also (or should?) be fixed by reworking the way read_command_lines manages the memory of input lines, so as to solve this globally, and not at all places where possibly a 'being handled' line of input might be re-allocated. Tom is looking at this, but in the meantime, this patch just goes back to the previous way to avoid the error (and be able to have the regression tests for the functional regression working). Without the patch, the test fails the following way: ... commands 2 3 Type commands for breakpoint(s) 2 3, one per line. End with a line saying just "end". > print 1234321 >end No breakpoint number 4321. ... (and under valgrind, the above reports access to freed memory). breakpoint.c is also modified to fix the regression introduced when clearing commands of several breakpoint by giving an empty list of commands, by just typing "end". GDB should read an empty list of command once, but it reads it for each breakpoint, as an empty list of command is NULL. gdb/ChangeLog 2018-08-02 Philippe Waroquiers * breakpoint.c (commands_command_1): New boolean cmd_read to detect cmd was already read. Always allocate a new_arg c_str to avoid accessing arg after some calls to read_command_line as this can free arg memory. --- gdb/breakpoint.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 6b6e1f6c25..7761bdb496 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1219,9 +1219,17 @@ commands_command_1 (const char *arg, int from_tty, struct command_line *control) { counted_command_line cmd; + /* cmd_read will be true once we have read cmd. Note that cmd might still be + NULL after the call to read_command_lines if the user provides an empty + list of command by just typing "end". */ + bool cmd_read = false; std::string new_arg; + /* arg might be an input line that might be released when reading + new input lines for the list of commands. So, build a new arg + to keep the input alive during the map_breakpoint_numbers call, + even if the new arg is just a copy of arg. */ if (arg == NULL || !*arg) { if (breakpoint_count - prev_breakpoint_count > 1) @@ -1231,12 +1239,18 @@ commands_command_1 (const char *arg, int from_tty, new_arg = string_printf ("%d", breakpoint_count); arg = new_arg.c_str (); } + else + { + new_arg = arg; + arg = new_arg.c_str (); + } map_breakpoint_numbers (arg, [&] (breakpoint *b) { - if (cmd == NULL) + if (!cmd_read) { + gdb_assert (cmd == NULL); if (control != NULL) cmd = control->body_list_0; else @@ -1256,6 +1270,7 @@ commands_command_1 (const char *arg, int from_tty, cmd = read_command_lines (str.c_str (), from_tty, 1, validator); } + cmd_read = true; } /* If a breakpoint was on the list more than once, we don't need to