From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id YP3sENSk81/iKQAAWB0awg (envelope-from ) for ; Mon, 04 Jan 2021 18:29:24 -0500 Received: by simark.ca (Postfix, from userid 112) id 36D001F0AA; Mon, 4 Jan 2021 18:29:24 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id D05321E965 for ; Mon, 4 Jan 2021 18:29:23 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 591C13898510; Mon, 4 Jan 2021 23:29:23 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 591C13898510 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1609802963; bh=ODA/ep3Xqk/jHQFH/qdlbxAnPwUKFQqcvZPCSCbURD8=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=KmURPWc9HqzwahQL86SxFr3+ZceSRibjbDYtUqgC4lWmZowFH6Zk9A6xyaqYqKD+N ZkXiNP5whSgI1M6ORX20tVFI3t35Llio2sXkpd6EQ7ubKDtIC5z4LQO1jmgII4uKrk 4+bbIm/lUufdmATr+mdx1EmjwONuph2dYCo4r29M= Received: from beryx.lancelotsix.com (beryx.lancelotsix.com [164.132.98.193]) by sourceware.org (Postfix) with ESMTPS id A56B1389850A for ; Mon, 4 Jan 2021 23:29:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A56B1389850A Received: from [IPv6:2a02:390:8443:0:c4e:2421:fef3:4cc0] (unknown [IPv6:2a02:390:8443:0:c4e:2421:fef3:4cc0]) by beryx.lancelotsix.com (Postfix) with ESMTPSA id 5C9E52E070; Tue, 5 Jan 2021 00:29:19 +0100 (CET) Subject: Re: [PATCH][PR gdb/27133] Avoid use after free with logging and debug redirect To: Tom Tromey , Lancelot SIX via Gdb-patches References: <20210101201127.20835-1-lsix@lancelotsix.com> <87pn2ke9eb.fsf@tromey.com> Message-ID: Date: Mon, 4 Jan 2021 23:29:18 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <87pn2ke9eb.fsf@tromey.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (beryx.lancelotsix.com [0.0.0.0]); Tue, 05 Jan 2021 00:29:19 +0100 (CET) X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Lancelot SIX via Gdb-patches Reply-To: Lancelot SIX Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 04/01/2021 20:26, Tom Tromey wrote: >>>>>> "Lancelot" == Lancelot SIX via Gdb-patches writes: > Lancelot> My copyright assignment has not yet been signed by FSF, but the change > Lancelot> should be small enough not to require it. Otherwise, it will need to > Lancelot> wait a few more days I guess. > > I don't know the state of this -- could you say? Hi, I have signed my end of the document, and am waiting for it to be also signed by the FSF. I should be quite close to the end of the procedure I guess. I’ll let you know when this is validated. In the meantime, I have few patch I still need to update according to reviews. > Lancelot> gdb/ChangeLog: > > Lancelot> PR gdb/27133 > Lancelot> * cli/cli-interp.c (cli_interp_base::set_logging): Ensure the > Lancelot> unique_ptr is released when the wrapped pointer is kept for later > Lancelot> use. > > Thanks. Looking at the code I feel like there's probably some cleaner > way to do this, but this patch is ok regardless. Yes, this feels a bit hacky, but I have gone through few variations and up until now none were as straight forward as I’d like them to be (hence the bloc of comment.). Here is another approach you might prefer: --- diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c index 424264a80e..34afc94d5a 100644 --- a/gdb/cli/cli-interp.c +++ b/gdb/cli/cli-interp.c @@ -417,7 +417,7 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect, ui_file *logfile_p = nullptr; if (logging_redirect || debug_redirect) { - logfile_p = logfile.get (); + logfile_p = logfile.release (); saved_output.file_to_delete = logfile_p; } @@ -426,7 +426,13 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect, ui_file *tee = nullptr; if (!logging_redirect || !debug_redirect) { - tee = new tee_file (gdb_stdout, std::move (logfile)); + /* If logfile_p not nullptr, this is because it took ownership + of the logfile. In this case transfer the ownership from + logfile_p to tee, otherwise just transfer ownership directly + from logfile to tee. */ + tee = new tee_file (gdb_stdout, (logfile_p == nullptr ? + std::move (logfile) : + ui_file_up (logfile_p))); saved_output.file_to_delete = tee; } This option might makes it easier to follow ownership. Lancelot > Tom