From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by sourceware.org (Postfix) with ESMTPS id 9FC223858434 for ; Wed, 21 Feb 2024 17:42:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9FC223858434 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9FC223858434 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.221.52 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708537358; cv=none; b=PXXL+AJddReeOgoy7U0lsSivOiHL4gpPAfPw0hBOzNgSJbL1k3+K4prQ6sQWKWd4ygx+dobtKJxfsnYBpHfw6SSTAvwWMOjTZbiAkTk+Vn3EYWNoQGn4dpg7dNDoVUnxmgV9/yQstz2tDkEjaaBxSVxTQYQpJQliqk0/fIVwkI4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708537358; c=relaxed/simple; bh=EfpcL93yGmXf4/lWMv+bkgc/Br5gN3Wyix2Rt3UIgCM=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=Ke3QdGgzCu0jg2vttxQRZRafduM/pXJlEeLPwfY3zGabwF6pnTbU1JvczTtRIwyWbLs7MhHvKJsoj5k+bqll2PLTi+fARi2CZWH9Ac3pXVBSoHH18CXrgrXB0W28dHx+TySSL8RUjGzx1snnpRB0jlFXZ/TbosmWVKRkUGOR9sk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wr1-f52.google.com with SMTP id ffacd0b85a97d-33d568fbf62so1996348f8f.3 for ; Wed, 21 Feb 2024 09:42:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708537351; x=1709142151; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MUq6dhqDPpv3YVp8JXSbVMsjiaN8gQDxEjMkELchBkk=; b=GZNbPmWodt3nnhiYrQ/dq82PjzTt/jNtbnvt2L5q9C2tG/KJw7Q6mYAtnz5oUHvxIc pyhtQIYVgsmj1lIfhchkNcMX/XKzOD3G7BjkrJUqbSnG4oE+UNjvDvAy2wn0vKqYHwku WzhnQ4OTX9m6W2XPYZQNeH1CHEfw8JgNwtIsW13qCof0MCD6MdZXAQjIIMryk2tFSBSk icEW1aSXdNJalhKL//j3vKOCqKejN4A/tqrWeME/I484oS3i3IYhOFkBbHFY7xKlVaeS v3DHO6VltpHAxu1U8TTF+nw3HYVHR/l4kZtaOiODg1Y09SNyDR6IUIxSZz+LtB39d+TP fv6A== X-Forwarded-Encrypted: i=1; AJvYcCX4Jf3d+AAUGcivijvCM8995LZaZg1tqloQzz16PEjCo5Opf8aG9scdZjlCPj3Xmi0AjJ83DC5apqksWd6k/X38FyjWsYFJ0XtgiQ== X-Gm-Message-State: AOJu0Yy1S75sIB8sQByrTfyrT2YPRXhErLf0b0rYaybCIXgYmoEdKphq Hr5dIpWvP08xYq71bHp9iKj9h7VYrMoGFc1+to1vA+HyceO0SKik X-Google-Smtp-Source: AGHT+IFuOYhEKjPSghxCmDo6SJjhSaexRbvg/vgJp2GNdVSH3Y6511ljY2pPx6O5KKDpVUSVtfLijA== X-Received: by 2002:adf:ed81:0:b0:33d:374f:83e6 with SMTP id c1-20020adfed81000000b0033d374f83e6mr8590075wro.4.1708537351235; Wed, 21 Feb 2024 09:42:31 -0800 (PST) Received: from ?IPV6:2001:8a0:f918:ab00:e0e8:3620:43e9:e37a? ([2001:8a0:f918:ab00:e0e8:3620:43e9:e37a]) by smtp.gmail.com with ESMTPSA id b7-20020a5d6347000000b0033d14455c99sm17430507wrw.101.2024.02.21.09.42.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 21 Feb 2024 09:42:30 -0800 (PST) Message-ID: <17059615-cba2-4ff2-a8dc-c6799930406f@palves.net> Date: Wed, 21 Feb 2024 17:42:26 +0000 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: Tom de Vries , gdb-patches@sourceware.org Cc: Simon Marchi References: <20240123114830.20253-1-tdevries@suse.de> <830ab71f-8968-4ab0-b8e7-8a2884169d4c@palves.net> From: Pedro Alves In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00,BODY_8BITS,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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 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 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 ... Does this patch work for you? >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 -- 2.43.2