From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57482 invoked by alias); 10 Aug 2018 00:35:34 -0000 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 Received: (qmail 57456 invoked by uid 89); 10 Aug 2018 00:35:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=philippe, Philippe, noticing X-HELO: gateway34.websitewelcome.com Received: from gateway34.websitewelcome.com (HELO gateway34.websitewelcome.com) (192.185.148.109) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 10 Aug 2018 00:35:31 +0000 Received: from cm15.websitewelcome.com (cm15.websitewelcome.com [100.42.49.9]) by gateway34.websitewelcome.com (Postfix) with ESMTP id 5AC961A043 for ; Thu, 9 Aug 2018 19:35:30 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id nvOhfR3E6bXuJnvOnfpkqs; Thu, 09 Aug 2018 19:35:30 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=tTQ4h9c9NXBS15PX0cnVf0u2BClij7Ru77m8eQWeTXE=; b=CTo7Ukbj0k+AJsWBxw4httay0J rJWXBKIyliHqy4pj9H1iu6RX4xMaq2zJbw+oLpPyR1wggAkAEtGPoRC2ZEPKW5O0Y62uj8rNK7j8g ugFn+x5jzSZ+faheKKsAwveyj; Received: from 75-166-85-72.hlrn.qwest.net ([75.166.85.72]:39548 helo=bapiya) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1fnvOh-003cU6-FA; Thu, 09 Aug 2018 19:35:19 -0500 From: Tom Tromey To: Philippe Waroquiers Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing References: <20180802212613.29813-1-philippe.waroquiers@skynet.be> <20180802212613.29813-2-philippe.waroquiers@skynet.be> <87sh3v1ezc.fsf@tromey.com> <87lg9gi1c4.fsf@tromey.com> <1533845999.1860.1.camel@skynet.be> Date: Fri, 10 Aug 2018 00:35:00 -0000 In-Reply-To: <1533845999.1860.1.camel@skynet.be> (Philippe Waroquiers's message of "Thu, 09 Aug 2018 22:19:59 +0200") Message-ID: <878t5fhxdl.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2018-08/txt/msg00241.txt.bz2 >>>>> "Philippe" == Philippe Waroquiers writes: Philippe> There was also a second regression which was fixed by the patch Philippe> suggested in the RFA 1/2 (and the test in RFA 2/2 was checking Philippe> the regression). Philippe> Will you add the fix for the second regression (and the test) Philippe> in another commit ? Yeah, I can rebase your patches on top of this one. >> if (repeat && *p1 == '\0') >> - return saved_command_line; >> + { >> + xfree (buffer_finish (cmd_line_buffer)); >> + buffer_grow_str0 (cmd_line_buffer, saved_command_line); >> + cmd_line_buffer->used_size = 0; Philippe> I was somewhat surprised by used_size being set to 0. Philippe> I am not sure to understand in which case it is supposed to be Philippe> set to 0 : it is set to 0 in the first few lines of handle_line_of_input Philippe> with a comment explaining why. Philippe> I however do not understand why it is set to the string length in Philippe> the 'Do history expansion' case, and not to 0 : as far as I can see, Philippe> cmd will be returned as a full line in case of expansion ? Thanks for noticing this. I suspect the history case (which doesn't seem to be tested...) is just wrong and should also set the used size to 0. I believe the idea being clear the used size is that the buffer still owns the string, but if the buffer is reused then the string is considered ok to overwrite. Which, admittedly, seems odd to me, but this patch seemed like the least intrusive way to avoid the use-after-free... I'll see if I can come up with a test case here. Philippe> IIUC, the returned value will stay valid as long as the Philippe> cmd_line_buffer is not destroyed but also only if the buffer is Philippe> not touched (e.g. no data can be added to it, as otherwise Philippe> the previously returned value might become dangling). Philippe> So, for example, the below (pseudo) code would be incorrect: Philippe> arg = command_line_input (..., cmd_buffer); Philippe> while parsing arg not finished Philippe> // here it is forbidden to touch cmd_buffer Philippe> // e.g. the below would be bad: Philippe> some_other_arg = command_line_input (..., cmd_buffer); Philippe> So, I am wondering what kind of usage of this function Philippe> will make the buffer bigger. Maybe worth pointing at the Philippe> above risk then ? I can add a comment somewhere. I don't really understand why this code works the way it does. It seems unusually convoluted. At the same time, I didn't want to completely tear it apart, since it's not clear that's a good use of one's time. Tom