From patchwork Fri Feb 23 16:20:31 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 86287 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 809443858435 for ; Fri, 23 Feb 2024 16:21:06 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-io1-xd36.google.com (mail-io1-xd36.google.com [IPv6:2607:f8b0:4864:20::d36]) by sourceware.org (Postfix) with ESMTPS id 25F4C3858CDA for ; Fri, 23 Feb 2024 16:20:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 25F4C3858CDA Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 25F4C3858CDA Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::d36 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708705245; cv=none; b=SvwUisYGc4zMO3nqfoJYZY+Ri6dQ3Wu7AX3HuFYalzXvv8k9iAeAiVy3OK3xAiUpsL9JZrA2+2PN1LpRQjd16FKPxt09R7jTF6Vt+dVyDU2pCya8iRbbp8TGnYI6Aa3Ub06a0NoFmTzdW/t0iHgrzH6raSRstmpTKLquyR2NXr0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708705245; c=relaxed/simple; bh=sWEbnGgeIoUmH7o6vqV6+dNIeU97nf2YfibRn1H+9QA=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=jQ34Z/J21tX2uJ/4v0uWmS2EjEuuk70/rZ+Nc0Ldx1wyFIfYbV7MCBIoEech447fRGch4zs//yx2eL6xDl7vhr+q86IXSJcDu+vWJKbwvE4wsgYn7MiSwFqSeH0MOfxNbzu4xBq+gLgvm4RviYk59mCqCIMEhRgPwE1l8egX9Bs= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-io1-xd36.google.com with SMTP id ca18e2360f4ac-7c72290deb6so41373539f.3 for ; Fri, 23 Feb 2024 08:20:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1708705242; x=1709310042; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=+wl8BP5NNjMSdTs3XISnQmq3ob2o/6y6M8yn33AD53s=; b=WlJbJEsIw6+ZTgijmUB3nBMvKe5iSLL+ugAYUMgKXl0eDxNLl5+xcyQAnUAXXO1COO aOR44yeFpSz2uE9KkrRsTVaVVEaKlxk5CpIDP5u9LnGbi0kk8wWiBEruUlBkapOwRkpI 1d5crfkaNBtvAcxUpT4t8LU/SQIPy4xMjUFK+ILFIq1OOtM0/C4oAzgLrY+VoMtVz+kK 9fO+6aUBhAJH19O6AhK4ZOc/mvijDQDLMvRv71xmHQw2bVN3VARUf6ouVTJcePTcKKqc 9Sdma61W8I8sx+SU80TSc4VaIowgPGgGCLE19070Vh/ZF0TNrkf22ZFt8gqYFKzXhceR oAYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708705242; x=1709310042; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=+wl8BP5NNjMSdTs3XISnQmq3ob2o/6y6M8yn33AD53s=; b=sBQmPtH9LFd3CJhCE+eMxzCXR0Z2Arxy9HtsvbrknH8oMkuZhV8NxMozYxyW1RFHNI Cca0fJxY5dLjg8IcibdChiT8mzkMdWycFrlDynaty3Yx56KaYfmajdA2ig4VPhmur1vS QFYF5knSOA8xjQ3zEQYxiQZQROEiGj/2BAhNWPlZn/l1W4HxFQh/TOGMdo4m2DsVrPWR SvWO6Z/pTr7cU49xSrC/SqxMUcnQ6+5YH7kBhd1Q9aDaUwdgjpRgtBI5M+lxPVOWTbY5 5VO4V0T/xTS2w6IuwwMN97/21MGOnAinpIBb5rxH65ly4D/bxCfMnIgLggOzEVpvAnfB sVIw== X-Gm-Message-State: AOJu0Yz7zA7LdfjIWIaFxK4pX9wlZUdW1/WAPJ1fUzmO6R1glk/AOnsH BRTwH4LUZHKIbbF5S9CblnN1VTLgHf4o0bxhYz94g2yIJgTyyWkIMhJnjWXHX8lzZrk/9njyo6c = X-Google-Smtp-Source: AGHT+IHCgj0GWZBxzGSfqH+FbcOOH7JJR79jBwwuvOeeCk5gS/xjN8FHw2k9FDmJ+CYVFj00seWfVg== X-Received: by 2002:a05:6602:1642:b0:7c7:2099:8bc0 with SMTP id y2-20020a056602164200b007c720998bc0mr440495iow.2.1708705242165; Fri, 23 Feb 2024 08:20:42 -0800 (PST) Received: from localhost.localdomain (71-211-170-195.hlrn.qwest.net. [71.211.170.195]) by smtp.gmail.com with ESMTPSA id gu2-20020a0566382e0200b00474341aa2bbsm2418590jab.141.2024.02.23.08.20.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 23 Feb 2024 08:20:41 -0800 (PST) From: Tom Tromey To: gdb-patches@sourceware.org Cc: Tom Tromey Subject: [PATCH] Fix gdb.interrupt race Date: Fri, 23 Feb 2024 09:20:31 -0700 Message-ID: <20240223162031.3568167-1-tromey@adacore.com> X-Mailer: git-send-email 2.43.0 MIME-Version: 1.0 X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org gdb.interrupt was introduced to implement DAP request cancellation. However, because it can be run from another thread, and because I didn't look deeply enough at the implementation, it turns out to be racy. The fix here is to lock accesses to certain globals in extension.c. Note that this won't work in the case where configure detects that the C++ compiler doesn't provide thread support. In this patch I chose to simply let this race, but another choice would be to disable DAP entirely in this situation. Regression tested on x86-64 Fedora 38. I also ran gdb.dap/pause.exp in a thread-sanitizer build tree to make sure the reported race is gone. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31263 Tested-By: Tom de Vries --- gdb/extension.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/gdb/extension.c b/gdb/extension.c index 42e05199d2c..3a77d355a78 100644 --- a/gdb/extension.c +++ b/gdb/extension.c @@ -634,6 +634,21 @@ breakpoint_ext_lang_cond_says_stop (struct breakpoint *b) This requires cooperation with the extension languages so the support is defined here. */ +#if CXX_STD_THREAD + +#include + +/* DAP needs a way to interrupt the main thread, so we added + gdb.interrupt. However, as this can run from any thread, we need + locking for the current extension language. If threading is not + available, we simply let this race. + + This lock is held for accesses to quit_flag, active_ext_lang, and + cooperative_sigint_handling_disabled. */ +static std::recursive_mutex ext_lang_mutex; + +#endif /* CXX_STD_THREAD */ + /* This flag tracks quit requests when we haven't called out to an extension language. it also holds quit requests when we transition to an extension language that doesn't have cooperative SIGINT handling. */ @@ -689,6 +704,10 @@ static bool cooperative_sigint_handling_disabled = false; scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_handling () { +#if CXX_STD_THREAD + std::lock_guard guard (ext_lang_mutex); +#endif /* CXX_STD_THREAD */ + /* Force the active extension language to the GDB scripting language. This ensures that a previously saved SIGINT is moved to the quit_flag global, as well as ensures that future SIGINTs @@ -706,6 +725,10 @@ scoped_disable_cooperative_sigint_handling::scoped_disable_cooperative_sigint_ha scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_handling () { +#if CXX_STD_THREAD + std::lock_guard guard (ext_lang_mutex); +#endif /* CXX_STD_THREAD */ + cooperative_sigint_handling_disabled = m_prev_cooperative_sigint_handling_disabled; restore_active_ext_lang (m_prev_active_ext_lang_state); } @@ -744,6 +767,10 @@ scoped_disable_cooperative_sigint_handling::~scoped_disable_cooperative_sigint_h struct active_ext_lang_state * set_active_ext_lang (const struct extension_language_defn *now_active) { +#if CXX_STD_THREAD + std::lock_guard guard (ext_lang_mutex); +#endif /* CXX_STD_THREAD */ + #if GDB_SELF_TEST if (selftests::hook_set_active_ext_lang) selftests::hook_set_active_ext_lang (); @@ -796,6 +823,10 @@ set_active_ext_lang (const struct extension_language_defn *now_active) void restore_active_ext_lang (struct active_ext_lang_state *previous) { +#if CXX_STD_THREAD + std::lock_guard guard (ext_lang_mutex); +#endif /* CXX_STD_THREAD */ + if (cooperative_sigint_handling_disabled) { /* See set_active_ext_lang. */ @@ -832,6 +863,10 @@ restore_active_ext_lang (struct active_ext_lang_state *previous) void set_quit_flag (void) { +#if CXX_STD_THREAD + std::lock_guard guard (ext_lang_mutex); +#endif /* CXX_STD_THREAD */ + if (active_ext_lang->ops != NULL && active_ext_lang->ops->set_quit_flag != NULL) active_ext_lang->ops->set_quit_flag (active_ext_lang); @@ -856,6 +891,10 @@ set_quit_flag (void) int check_quit_flag (void) { +#if CXX_STD_THREAD + std::lock_guard guard (ext_lang_mutex); +#endif /* CXX_STD_THREAD */ + int result = 0; for (const struct extension_language_defn *extlang : extension_languages)