From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by sourceware.org (Postfix) with ESMTPS id E54463858D1E for ; Thu, 22 Feb 2024 11:43:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E54463858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E54463858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.223.130 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708602241; cv=none; b=XrnHEblHOundra2KpRmzMgeh4tDse75EuOwgFGNH+T6/EWkKFpiGJxSmYN3TbEZX0jbTYMHHyxXv3PPF+Up3mYXWYX6WL2TonIU5VfU+nXavaq3rY7HbAaO/fQ1LnzAJRZC1pZpYCGJwqKpjMd78R/4AgK/pKFMmnGn3ymZx60U= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708602241; c=relaxed/simple; bh=vI99IRja3vwh3oiK4jVm0bzdafaEjqaDDZZf2z6AEGo=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature: Message-ID:Date:MIME-Version:Subject:To:From; b=Rh022Kiqh8xbklTX8ezdaIvf94M2rpQx72BEFSCne+6g3aEZPw2TXyKKVNl6z3rqRz8mRXWTm6S6fdnxStjZOGQxKixr29SryVullcOnpP1bjhR7DXWid+ww+TEyEbFXuRLvBkt3vKWRcb3LJlJ14FQYnFruUq4FrgeZ7PJ2Ajk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap1.dmz-prg2.suse.org (imap1.dmz-prg2.suse.org [10.150.64.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id D9AC222172; Thu, 22 Feb 2024 11:43:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1708602238; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=v/18q/Hnbl4eCTi37jA/RjboBcMkp6XbYNRQo31lsXE=; b=UFbP3EIsVSPjypH7GZ1XN0v8oCdEteLijVpc0uyzlhPQEW6avI5j2/cQYF1zElBySIdUlI AqIWEa2OthSA6E5+3mZU4dn/ZMCbF6k3W63TdzOd/lYXsAQj7d6pJP1XYGBMK+zIah9+9J qa8B4yUzDpngeRBUjKw9yQbTVLHKbTI= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1708602238; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=v/18q/Hnbl4eCTi37jA/RjboBcMkp6XbYNRQo31lsXE=; b=agTfHnT851NruLOiDoKEHG1cjlf/BkP2Ly9JuYQSK7pShlHmM9W5KKEIRW3SNMD2QY4zEN wrkTkuDFlHuYBiBQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1708602237; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=v/18q/Hnbl4eCTi37jA/RjboBcMkp6XbYNRQo31lsXE=; b=cVkuJtN5+xAsaTveBJNuTzlMf4qCgbfsta3AuleNYOlb1WSOcnvBnlw/jMIElDjhZuG6j9 p9MZOUCWtps5+HY9Q+qyu0CjxMtNuxupDumOpHwjD8CGoRq+z9dErIZsyAePaLT9gbGS2g 986HeUa9bd3U5pZlV0AuIP9gXVV2Lt0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1708602237; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=v/18q/Hnbl4eCTi37jA/RjboBcMkp6XbYNRQo31lsXE=; b=gcjTIrDxNzYdYnoMnq8IOap3wWvV3fK3kmTG8+kV5ErMbLy/knAHf4LiZnHPmZtVMPsliD 036NbwvUKzreonCA== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id BF3C013A8C; Thu, 22 Feb 2024 11:43:57 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id wO5uLX0z12WNIAAAD6G6ig (envelope-from ); Thu, 22 Feb 2024 11:43:57 +0000 Message-ID: Date: Thu, 22 Feb 2024 12:43:55 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] [gdb] Fix heap-use-after-free in select_event_lwp Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org Cc: Simon Marchi References: <20240123114830.20253-1-tdevries@suse.de> <830ab71f-8968-4ab0-b8e7-8a2884169d4c@palves.net> <17059615-cba2-4ff2-a8dc-c6799930406f@palves.net> From: Tom de Vries In-Reply-To: <17059615-cba2-4ff2-a8dc-c6799930406f@palves.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Authentication-Results: smtp-out1.suse.de; none X-Spam-Level: X-Spam-Score: -4.19 X-Spamd-Result: default: False [-4.19 / 50.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; XM_UA_NO_VERSION(0.01)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[3]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; NEURAL_HAM_LONG(-1.00)[-1.000]; BAYES_HAM(-3.00)[100.00%]; RCVD_COUNT_THREE(0.00)[3]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; NEURAL_HAM_SHORT(-0.10)[-0.504]; FUZZY_BLOCKED(0.00)[rspamd.com]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_TLS_ALL(0.00)[]; MID_RHS_MATCH_FROM(0.00)[] X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2/21/24 18:42, Pedro Alves wrote: > On 2024-02-19 15:04, Tom de Vries wrote: >> On 2/9/24 16:46, Pedro Alves wrote: >>> On 2024-01-23 11:48, Tom de Vries wrote: >>> It sounds like we were about to report a stop for a thread that isn't marked as stopped? >>> Now it looks to me that _that_ would be the bug to fix. >> >> This patch adds an assert to catch the bug you mention, and a fix in wait_lwp: >> ... >> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c >> index e91c57ba239..5022da9abd2 100644 >> --- a/gdb/linux-nat.c >> +++ b/gdb/linux-nat.c >> @@ -2210,6 +2210,7 @@ wait_lwp (struct lwp_info *lp) >>           core.  Store it in lp->waitstatus, because lp->status >>           would be ambiguous (W_EXITCODE(0,0) == 0).  */ >>            lp->waitstatus = host_status_to_waitstatus (status); >> +          lp->stopped = 1; >>            return 0; >>          } >> >> @@ -3368,6 +3369,7 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus, >>      } >> >>    gdb_assert (lp); >> +  gdb_assert (lp->stopped); >> >>    status = lp->status; >>    lp->status = 0; >> ... >> >> This fixes the problem observed in the PR, and passes testing on x86_64-linux and aarch64-linux. >> >> WDYT? > > Ah, thanks, so it was an exiting lwp that we're going to report the exit for. > > Yes, this is the right thing to do. We do the same in linux_nat_filter_event: > > /* This LWP is stopped now. (And if dead, this prevents it from > ever being continued.) */ > lp->stopped = 1; > > > gdbserver has a mark_lwp_dead function to centralize setting up various lwp flags such that the > rest of the code doesn't mishandle them, and it seems like a good idea to do a similar thing in > gdb as well. Like below. I ran the testsuite and saw no regressions. > I agree that it's a good idea, and also ran the test-suite and saw no regressions. > I tried to reproduce the gdb.base/vfork-follow-parent.exp issue described in the bug, but when I > run that with a gdb built with -O0 -fsanitize=thread, the testcase just seemingly hangs starting > up until I ctrl-c: > > Running /home/pedro/rocm/gdb/build-master-sanitize-thread/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/vfork-follow-parent.exp ... > WARNING: Couldn't set the height to 0 > WARNING: Couldn't set the width to 0. > ^Cgot a INT signal, interrupted by user > > and when I look at gdb.log, I see it is full of the same report over and over: > > ... > WARNING: ThreadSanitizer: signal-unsafe call inside of a signal (pid=3086741) > #0 free ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:711 (libtsan.so.0+0x37ab8) > #1 __gconv_close_transform iconv/gconv_db.c:800 (libc.so.6+0x2be3a) > #2 ext_lang_initialization() ../../src/gdb/extension.c:339 (gdb+0x5c6a3f) > #3 captured_main_1 ../../src/gdb/main.c:1058 (gdb+0x7c94da) > #4 captured_main ../../src/gdb/main.c:1329 (gdb+0x7ca7be) > #5 gdb_main(captured_main_args*) ../../src/gdb/main.c:1358 (gdb+0x7ca886) > #6 main ../../src/gdb/gdb.c:39 (gdb+0xf98b3) > > SUMMARY: ThreadSanitizer: signal-unsafe call inside of a signal iconv/gconv_db.c:800 in __gconv_close_transform > ... > > Hmm, I can't reproduce that, not on aarch64-linux f39, nor on Leap 15.4 x86_64-linux. At first glance this could be a tsan problem, given that the stack trace doesn't show entry into a signal handler. > Does this patch work for you? > It does. As mentioned in the PR, on aarch64-linux this reproduces for me 5/10 times, and with your patch 0/10 times. Given that pretty much the entire patch is yours, do you want to proceed with this, or do you want me to integrate this into my patch with a Co-Authored-By tag? Thanks, - Tom > From 8cbc62be0dab8bb4b09fe05a52a348ddbfb2e33c Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Wed, 21 Feb 2024 16:23:55 +0000 > Subject: [PATCH] mark_lwp_dead > > Change-Id: I4a6169976f89bf714c478cbb2b7d4c32365e62a9 > --- > gdb/linux-nat.c | 34 +++++++++++++++++++++++++--------- > 1 file changed, 25 insertions(+), 9 deletions(-) > > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c > index e91c57ba239..b3d319e0a82 100644 > --- a/gdb/linux-nat.c > +++ b/gdb/linux-nat.c > @@ -2123,6 +2123,27 @@ wait_for_signal () > } > } > > +/* Mark LWP dead, with STATUS as exit status pending to report > + later. */ > + > +static void > +mark_lwp_dead (lwp_info *lp, int status) > +{ > + /* Store the exit status lp->waitstatus, because lp->status would be > + ambiguous (W_EXITCODE(0,0) == 0). */ > + lp->waitstatus = host_status_to_waitstatus (status); > + > + /* If we're processing LP's status, there should be no other event > + already recorded as pending. */ > + gdb_assert (lp->status == 0); > + > + /* Dead LWP's aren't expected to reported a pending sigstop. */ > + lp->signalled = 0; > + > + /* Prevent trying to stop it. */ > + lp->stopped = 1; > +} > + > /* Wait for LP to stop. Returns the wait status, or 0 if the LWP has > exited. */ > > @@ -2207,9 +2228,8 @@ wait_lwp (struct lwp_info *lp) > > /* If this is the leader exiting, it means the whole > process is gone. Store the status to report to the > - core. Store it in lp->waitstatus, because lp->status > - would be ambiguous (W_EXITCODE(0,0) == 0). */ > - lp->waitstatus = host_status_to_waitstatus (status); > + core. */ > + mark_lwp_dead (lp, status); > return 0; > } > > @@ -3013,12 +3033,7 @@ linux_nat_filter_event (int lwpid, int status) > linux_nat_debug_printf ("LWP %ld exited (resumed=%d)", > lp->ptid.lwp (), lp->resumed); > > - /* Dead LWP's aren't expected to reported a pending sigstop. */ > - lp->signalled = 0; > - > - /* Store the pending event in the waitstatus, because > - W_EXITCODE(0,0) == 0. */ > - lp->waitstatus = host_status_to_waitstatus (status); > + mark_lwp_dead (lp, status); > return; > } > > @@ -3368,6 +3383,7 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus, > } > > gdb_assert (lp); > + gdb_assert (lp->stopped); > > status = lp->status; > lp->status = 0; > > base-commit: 99eeecc8d276e5af745e48825d66efff693a7678