From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id tEdsNxqC718kSwAAWB0awg (envelope-from ) for ; Fri, 01 Jan 2021 15:12:10 -0500 Received: by simark.ca (Postfix, from userid 112) id D57CA1F0AA; Fri, 1 Jan 2021 15:12:10 -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 86AC41E590 for ; Fri, 1 Jan 2021 15:12:10 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C2C9B3846403; Fri, 1 Jan 2021 20:12:09 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C2C9B3846403 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1609531929; bh=z869MNNc3LFvevwZHi9RDjyZTenssZuHQQ3Cg3KGTNI=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=bMr8kLHiL1fgxouPY2mI1NhKjCIpvqmBFoaTUMWTf25fSgu9NK2pE9ZJcH+BPFpVT kUhOvXCk6sFw5DZdWL7eZEflOroxm51s/ysXzBKxZXlQwPlqnV/dAPWoxMOrLQixLZ 9ykVRerTxLIzlmu4fG73QfQGrvl03EkaI/h+qruo= Received: from beryx.lancelotsix.com (beryx.lancelotsix.com [IPv6:2001:41d0:401:3000::1ab3]) by sourceware.org (Postfix) with ESMTPS id 344B03846403 for ; Fri, 1 Jan 2021 20:12:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 344B03846403 Received: from gwenhwyvar.lan (unknown [IPv6:2a02:390:8443:0:21b0:40c4:6e60:baf5]) by beryx.lancelotsix.com (Postfix) with ESMTPSA id 848182E070; Fri, 1 Jan 2021 21:12:03 +0100 (CET) To: gdb-patches@sourceware.org Subject: [PATCH][PR gdb/27133] Avoid use after free with logging and debug redirect Date: Fri, 1 Jan 2021 20:11:28 +0000 Message-Id: <20210101201127.20835-1-lsix@lancelotsix.com> X-Mailer: git-send-email 2.29.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (beryx.lancelotsix.com [0.0.0.0]); Fri, 01 Jan 2021 21:12:03 +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 Cc: Lancelot SIX Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" This patch addresses PR gdb/27133. Before it, the following succession of commands would cause gdb to crash: set logging redirect on set logging debugredirect on set logging on The problem eventually comes down to a use after free. The function cli_interp_base::set_logging is called with a unique_ptr argument that holds a pointer to the redirection file. In the problematic use case, no-one ever took ownership of that pointer (as far as unique_ptr is concerned), so the call to its dtor at the end of the function causes the file object to be deleted. Any later use of the pointer to the redirection file is therefore an error. This patch ensures that the unique_ptr is released when required (so it does not assume ownership anymore). The internal logic of cli_interp_base::set_logging takes care of freeing the ui_file when it is not necessary anymore using the saved_output.file_to_delete field. My copyright assignment has not yet been signed by FSF, but the change should be small enough not to require it. Otherwise, it will need to wait a few more days I guess. gdb/ChangeLog: PR gdb/27133 * cli/cli-interp.c (cli_interp_base::set_logging): Ensure the unique_ptr is released when the wrapped pointer is kept for later use. gdb/testsuite/ChangeLog: PR gdb/27133 * gdb.base/ui-redirect.exp: Add test case that ensures that redirecting both logging and debug does not cause gdb to crash. --- gdb/cli/cli-interp.c | 8 ++++++++ gdb/testsuite/gdb.base/ui-redirect.exp | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c index 424264a80e..882384e9a9 100644 --- a/gdb/cli/cli-interp.c +++ b/gdb/cli/cli-interp.c @@ -430,6 +430,14 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect, saved_output.file_to_delete = tee; } + /* Make sure that the call to logfile's dtor does not delete the + underlying pointer if we still keep a reference to it. If + logfile_p is not referenced as the file_to_delete, then either + the logfile is not used (no redirection) and it should be + deleted, or a tee took ownership of the pointer. */ + if (logfile_p != nullptr && saved_output.file_to_delete == logfile_p) + logfile.release (); + gdb_stdout = logging_redirect ? logfile_p : tee; gdb_stdlog = debug_redirect ? logfile_p : tee; gdb_stderr = logging_redirect ? logfile_p : tee; diff --git a/gdb/testsuite/gdb.base/ui-redirect.exp b/gdb/testsuite/gdb.base/ui-redirect.exp index 21fbb58aff..e6855d2378 100644 --- a/gdb/testsuite/gdb.base/ui-redirect.exp +++ b/gdb/testsuite/gdb.base/ui-redirect.exp @@ -133,3 +133,11 @@ with_test_prefix "redirect debugging" { gdb_test "set logging off" "Done logging to /dev/null\\." gdb_test "help" "List of classes of commands:.*" } + +with_test_prefix "redirect logging and debuging" { + gdb_test_no_output "set logging redirect on" + gdb_test_no_output "set logging debugredirect on" + gdb_test "set logging on" \ + "Redirecting output to /dev/null.*Redirecting debug output to /dev/null\\." + gdb_test "set logging off" "Done logging to /dev/null\\." +} -- 2.29.2