From patchwork Mon Apr 15 18:22:18 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 88509 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 860F73844772 for ; Mon, 15 Apr 2024 18:24:16 +0000 (GMT) X-Original-To: binutils@sourceware.org Delivered-To: binutils@sourceware.org Received: from omta034.useast.a.cloudfilter.net (omta034.useast.a.cloudfilter.net [44.202.169.33]) by sourceware.org (Postfix) with ESMTPS id E63453858408 for ; Mon, 15 Apr 2024 18:23:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E63453858408 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E63453858408 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=44.202.169.33 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713205419; cv=none; b=BI65xPcJYh0YVR8bdy2kAEp6Eck2Bsj9uwX8A0+AEjdudlbkq3zb+xBB/BnLbKvaVvWF72WTb9pvq+qVqRwdTtbsn6D1G5TnZMGyX8aOVGxZS1vF9EdubOU2PkVhNfVrwQNpXf3rpSMuOUUdZ2hqovgLZ+K4xsT7JDqQAO5CNM8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713205419; c=relaxed/simple; bh=fyODr8TpnfyPKhpbcdjPZYIawgojuSJaqqxq9u59n6k=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=G3po7uJ86oKS8bJT+oGvySMSIVcCaGiOVDD+2DsLcNlOtSZpGFOOXRZHxdtZJ0LahKegD4BRiNQquuFGHBLglTzim1CBLIyu97AyRX11mQMeRwNWnn0lP4P2pUrjyLhXR8hgXrk3ovBkgLpUiLBwv3ulAAvYI/VR3jN3A76O8Fk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from eig-obgw-5010a.ext.cloudfilter.net ([10.0.29.199]) by cmsmtp with ESMTPS id wDI8rGIDhs4yTwQzQrEscb; Mon, 15 Apr 2024 18:23:36 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id wQzPrisoVo82kwQzQr1WjG; Mon, 15 Apr 2024 18:23:36 +0000 X-Authority-Analysis: v=2.4 cv=WOB5XGsR c=1 sm=1 tr=0 ts=661d70a8 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=raytVjVEu-sA:10 a=Qbun_eYptAEA:10 a=CCpqsmhAAAAA:8 a=YMXtYI3SjDaeBidk_ZwA:9 a=3ZKOabzyN94A:10 a=ul9cdbp4aOFLsgKbc677:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Sender:Reply-To:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=mD1hpw0HPyBYBtEigbxJsHkNU4ssp9cWoiUly5cBdlQ=; b=SKgJGfQ9A8eRqG9pUpaU1hmLMu 6yTF8lc+oC17rEN0W3Dc4n5sSXtWLNiwcbCuheOFLx3cezTYt+t0gW1rcRAusWqt+3bwlrWWM0ltY rbK3I36OjJ3iQSzZEs4ozFZqX; Received: from 97-122-82-115.hlrn.qwest.net ([97.122.82.115]:42076 helo=localhost.localdomain) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96.2) (envelope-from ) id 1rwQzP-000gh0-23; Mon, 15 Apr 2024 12:23:35 -0600 From: Tom Tromey To: binutils@sourceware.org Cc: Tom Tromey Subject: [PATCH v2 1/2] Thread-safety improvements for bfd_check_format_matches Date: Mon, 15 Apr 2024 12:22:18 -0600 Message-ID: <20240415182328.2754398-2-tom@tromey.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240415182328.2754398-1-tom@tromey.com> References: <20240415182328.2754398-1-tom@tromey.com> MIME-Version: 1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 97.122.82.115 X-Source-L: No X-Exim-ID: 1rwQzP-000gh0-23 X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 97-122-82-115.hlrn.qwest.net (localhost.localdomain) [97.122.82.115]:42076 X-Source-Auth: tom+tromey.com X-Email-Count: 5 X-Org: HG=bhshared;ORG=bluehost; X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfNupqqqk7gr17FbsttWwGqLtCweV3qeTMmAhuNWUuzc+pcS5aEyr5OoxjhfxiRDOuM0N5LRP7DMYqyW/APp8DHjXDcDnGHn9qDqwHRlzOw7q+4VU255C LAJUUNUug2uc+/p0b+n2jEpmtM0p24vQn/aNrqtvHkAfsnolszpxuHMLlDkByYaJQuADLj5ov9u8BXKc59REXvVgpPJtzaaqO2A= X-Spam-Status: No, score=-3021.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, JMQ_SPF_NEUTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP 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: binutils@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: binutils-bounces+patchwork=sourceware.org@sourceware.org A gdb bug found that bfd_check_format_matches has some data races when called from multiple threads. In particular, it changes the BFD error handler, which is a global. It also has a local static variable ("in_check_format") that is used for recursion detection. And, finally, it may emit warnings to the per-xvec warning array, which is a global. This patch removes all the races here. The first part of patch is to change _bfd_error_handler to directly handle the needs of bfd_check_format_matches. This way, the error handler does not need to be changed. This change lets us use the new per-thread global (error_handler_messages, replacing error_handler_bfd) to also remove the need for in_check_format -- a single variable suffices. Finally, the global per-xvec array is replaced with a new type that holds the error messages. The outermost such type is stack-allocated in bfd_check_format_matches. I tested this using the binutils test suite. I also built gdb with thread sanitizer and ran the test case that was noted as failing. Finally, Alan sent me the test file that caused the addition of the xvec warning code in the first place, and I confirmed that "nm-new" has the same behavior on this file both before and after this patch. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264 --- bfd/bfd.c | 141 ++++++++++++++++++++++++++++++++++++++++++++------ bfd/format.c | 71 ++++++++++++------------- bfd/libbfd.h | 33 ++++++++---- bfd/targets.c | 58 --------------------- 4 files changed, 182 insertions(+), 121 deletions(-) diff --git a/bfd/bfd.c b/bfd/bfd.c index 8fd86f68e6e..80ce859d874 100644 --- a/bfd/bfd.c +++ b/bfd/bfd.c @@ -1565,10 +1565,90 @@ err_sprintf (void *stream, const char *fmt, ...) return total; } -/* Communicate the bfd processed by bfd_check_format_matches to the - error handling function error_handler_sprintf. */ +/* +INTERNAL +.{* Cached _bfd_check_format messages are put in this. *} +.struct per_xvec_message +.{ +. struct per_xvec_message *next; +. char message[]; +.}; +. +.{* A list of per_xvec_message objects. The targ field +. indicates which xvec this list holds; PER_XVEC_NO_TARGET +. indicates that this entry isn't yet used. The abfd field is +. only needed in the root entry of the list. *} +.struct per_xvec_messages +.{ +. bfd *abfd; +. const bfd_target *targ; +. struct per_xvec_message *messages; +. struct per_xvec_messages *next; +.}; +. +.#define PER_XVEC_NO_TARGET ((const bfd_target *) -1) +*/ + +/* Helper function to find or allocate the correct per-xvec object + when emitting a message. */ + +static struct per_xvec_message * +_bfd_per_xvec_warn (struct per_xvec_messages *messages, size_t alloc) +{ + const bfd_target *targ = messages->abfd->xvec; + + struct per_xvec_messages *prev = NULL; + struct per_xvec_messages *iter = messages; + + for (iter = messages; iter != NULL; iter = iter->next) + { + if (iter->targ == targ || iter->targ == PER_XVEC_NO_TARGET) + break; + prev = iter; + } + + if (iter == NULL) + { + iter = bfd_malloc (sizeof (*iter)); + if (iter == NULL) + return NULL; + iter->abfd = messages->abfd; + iter->targ = targ; + iter->messages = NULL; + iter->next = NULL; + prev->next = iter; + } + else if (iter->targ == NULL) + iter->targ = targ; + + struct per_xvec_message **m = &iter->messages; + int count = 0; + while (*m) + { + m = &(*m)->next; + count++; + } + /* Anti-fuzzer measure. Don't cache more than 5 messages. */ + if (count < 5) + { + *m = bfd_malloc (sizeof (**m) + alloc); + if (*m != NULL) + (*m)->next = NULL; + } + return *m; +} -static bfd *error_handler_bfd; +/* Communicate the error-message container processed by + bfd_check_format_matches to the error handling function + error_handler_sprintf. When non-NULL, _bfd_error_handler will call + error_handler_sprintf; when NULL, _bfd_error_internal will be used + instead. */ + +static TLS struct per_xvec_messages *error_handler_messages; + +/* A special value for error_handler_messages that indicates that the + error should simply be ignored. */ +#define IGNORE_ERROR_MESSAGES ((struct per_xvec_messages *) -1) /* An error handler that prints to a string, then dups that string to a per-xvec cache. */ @@ -1585,12 +1665,12 @@ error_handler_sprintf (const char *fmt, va_list ap) _bfd_print (err_sprintf, &error_stream, fmt, ap); size_t len = error_stream.ptr - error_buf; - struct per_xvec_message **warn - = _bfd_per_xvec_warn (error_handler_bfd->xvec, len + 1); - if (*warn) + struct per_xvec_message *warn + = _bfd_per_xvec_warn (error_handler_messages, len + 1); + if (warn) { - memcpy ((*warn)->message, error_buf, len); - (*warn)->message[len] = 0; + memcpy (warn->message, error_buf, len); + warn->message[len] = 0; } } @@ -1628,7 +1708,14 @@ _bfd_error_handler (const char *fmt, ...) va_list ap; va_start (ap, fmt); - _bfd_error_internal (fmt, ap); + if (error_handler_messages == IGNORE_ERROR_MESSAGES) + { + /* Nothing. */ + } + else if (error_handler_messages != NULL) + error_handler_sprintf (fmt, ap); + else + _bfd_error_internal (fmt, ap); va_end (ap); } @@ -1659,18 +1746,42 @@ INTERNAL_FUNCTION _bfd_set_error_handler_caching SYNOPSIS - bfd_error_handler_type _bfd_set_error_handler_caching (bfd *); + struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *); DESCRIPTION Set the BFD error handler function to one that stores messages - to the per_xvec_warn array. Returns the previous function. + to the per_xvec_messages object. Returns the previous object + to which messages are stored. Note that two sequential calls + to this with a non-NULL argument will cause output to be + dropped, rather than gathered. */ -bfd_error_handler_type -_bfd_set_error_handler_caching (bfd *abfd) +struct per_xvec_messages * +_bfd_set_error_handler_caching (struct per_xvec_messages *messages) +{ + struct per_xvec_messages *old = error_handler_messages; + if (old == NULL) + error_handler_messages = messages; + else + error_handler_messages = IGNORE_ERROR_MESSAGES; + return old; +} + +/* +INTERNAL_FUNCTION + _bfd_restore_error_handler_caching + +SYNOPSIS + void _bfd_restore_error_handler_caching (struct per_xvec_messages *); + +DESCRIPTION + Reset the BFD error handler object to an earlier value. +*/ + +void +_bfd_restore_error_handler_caching (struct per_xvec_messages *old) { - error_handler_bfd = abfd; - return bfd_set_error_handler (error_handler_sprintf); + error_handler_messages = old; } /* diff --git a/bfd/format.c b/bfd/format.c index 6d95683acb5..2a700bab557 100644 --- a/bfd/format.c +++ b/bfd/format.c @@ -272,10 +272,34 @@ clear_warnmsg (struct per_xvec_message **list) *list = NULL; } +/* Free all the storage in LIST. Note that the first element of LIST + is special and is assumed to be stack-allocated. TARG is used for + re-issuing warning messages. If TARG is PER_XVEC_NO_TARGET, then + it acts like a sort of wildcard -- messages are only reissued if + they are all associated with a single BFD target, regardless of + which one it is. If TARG is anything else, then only messages + associated with TARG are emitted. */ + static void -null_error_handler (const char *fmt ATTRIBUTE_UNUSED, - va_list ap ATTRIBUTE_UNUSED) +print_and_clear_messages (struct per_xvec_messages *list, + const bfd_target *targ) { + struct per_xvec_messages *iter = list; + + if (targ == PER_XVEC_NO_TARGET && list->next == NULL) + print_warnmsg (&list->messages); + + while (iter != NULL) + { + struct per_xvec_messages *next = iter->next; + + if (iter->targ == targ) + print_warnmsg (&iter->messages); + clear_warnmsg (&iter->messages); + if (iter != list) + free (iter); + iter = next; + } } /* This a copy of lto_section defined in GCC (lto-streamer.h). */ @@ -357,8 +381,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) unsigned int initial_section_id = _bfd_section_id; struct bfd_preserve preserve, preserve_match; bfd_cleanup cleanup = NULL; - bfd_error_handler_type orig_error_handler; - static int in_check_format; + struct per_xvec_messages messages = { abfd, PER_XVEC_NO_TARGET, NULL, NULL }; + struct per_xvec_messages *orig_messages; if (matching != NULL) *matching = NULL; @@ -392,11 +416,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) /* Don't report errors on recursive calls checking the first element of an archive. */ - if (in_check_format) - orig_error_handler = bfd_set_error_handler (null_error_handler); - else - orig_error_handler = _bfd_set_error_handler_caching (abfd); - ++in_check_format; + orig_messages = _bfd_set_error_handler_caching (&messages); preserve_match.marker = NULL; if (!bfd_preserve_save (abfd, &preserve, NULL)) @@ -638,15 +658,9 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) if (preserve_match.marker != NULL) bfd_preserve_finish (abfd, &preserve_match); bfd_preserve_finish (abfd, &preserve); - bfd_set_error_handler (orig_error_handler); + _bfd_restore_error_handler_caching (orig_messages); - struct per_xvec_message **list = _bfd_per_xvec_warn (abfd->xvec, 0); - if (*list) - print_warnmsg (list); - list = _bfd_per_xvec_warn (NULL, 0); - for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++) - clear_warnmsg (list++); - --in_check_format; + print_and_clear_messages (&messages, abfd->xvec); bfd_set_lto_type (abfd); @@ -692,27 +706,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) if (preserve_match.marker != NULL) bfd_preserve_finish (abfd, &preserve_match); bfd_preserve_restore (abfd, &preserve); - bfd_set_error_handler (orig_error_handler); - struct per_xvec_message **list = _bfd_per_xvec_warn (NULL, 0); - struct per_xvec_message **one = NULL; - for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++) - { - if (list[i]) - { - if (!one) - one = list + i; - else - { - one = NULL; - break; - } - } - } - if (one) - print_warnmsg (one); - for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++) - clear_warnmsg (list++); - --in_check_format; + _bfd_restore_error_handler_caching (orig_messages); + print_and_clear_messages (&messages, PER_XVEC_NO_TARGET); return false; } diff --git a/bfd/libbfd.h b/bfd/libbfd.h index 5f5ad2daace..4f0acc48b49 100644 --- a/bfd/libbfd.h +++ b/bfd/libbfd.h @@ -963,7 +963,29 @@ void _bfd_clear_error_data (void) ATTRIBUTE_HIDDEN; char *bfd_asprintf (const char *fmt, ...) ATTRIBUTE_HIDDEN; -bfd_error_handler_type _bfd_set_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN; +/* Cached _bfd_check_format messages are put in this. */ +struct per_xvec_message +{ + struct per_xvec_message *next; + char message[]; +}; + +/* A list of per_xvec_message objects. The targ field + indicates which xvec this list holds; PER_XVEC_NO_TARGET + indicates that this entry isn't yet used. The abfd field is + only needed in the root entry of the list. */ +struct per_xvec_messages +{ + bfd *abfd; + const bfd_target *targ; + struct per_xvec_message *messages; + struct per_xvec_messages *next; +}; + +#define PER_XVEC_NO_TARGET ((const bfd_target *) -1) +struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN; + +void _bfd_restore_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN; const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN; @@ -3708,15 +3730,6 @@ bool _bfd_write_stab_strings (bfd *, struct stab_info *) ATTRIBUTE_HIDDEN; bfd_vma _bfd_stab_section_offset (asection *, void *, bfd_vma) ATTRIBUTE_HIDDEN; /* Extracted from targets.c. */ -/* Cached _bfd_check_format messages are put in this. */ -struct per_xvec_message -{ - struct per_xvec_message *next; - char message[]; -}; - -struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t) ATTRIBUTE_HIDDEN; - #ifdef __cplusplus } #endif diff --git a/bfd/targets.c b/bfd/targets.c index 532b6465a61..0d5d73ba462 100644 --- a/bfd/targets.c +++ b/bfd/targets.c @@ -1454,9 +1454,6 @@ const bfd_target *const *const bfd_associated_vector = _bfd_associated_vector; number of entries that the array could possibly need. */ const size_t _bfd_target_vector_entries = ARRAY_SIZE (_bfd_target_vector); -/* A place to stash a warning from _bfd_check_format. */ -static struct per_xvec_message *per_xvec_warn[ARRAY_SIZE (_bfd_target_vector) - + 1]; /* This array maps configuration triplets onto BFD vectors. */ @@ -1476,61 +1473,6 @@ static const struct targmatch bfd_target_match[] = { { NULL, NULL } }; -/* -INTERNAL -.{* Cached _bfd_check_format messages are put in this. *} -.struct per_xvec_message -.{ -. struct per_xvec_message *next; -. char message[]; -.}; -. -INTERNAL_FUNCTION - _bfd_per_xvec_warn - -SYNOPSIS - struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t); - -DESCRIPTION - Return a location for the given target xvec to use for - warnings specific to that target. If TARG is NULL, returns - the array of per_xvec_message pointers, otherwise if ALLOC is - zero, returns a pointer to a pointer to the list of messages - for TARG, otherwise (both TARG and ALLOC non-zero), allocates - a new per_xvec_message with space for a string of ALLOC - bytes and returns a pointer to a pointer to it. May return a - pointer to a NULL pointer on allocation failure. -*/ - -struct per_xvec_message ** -_bfd_per_xvec_warn (const bfd_target *targ, size_t alloc) -{ - size_t idx; - - if (!targ) - return per_xvec_warn; - for (idx = 0; idx < ARRAY_SIZE (_bfd_target_vector); idx++) - if (_bfd_target_vector[idx] == targ) - break; - struct per_xvec_message **m = per_xvec_warn + idx; - if (!alloc) - return m; - int count = 0; - while (*m) - { - m = &(*m)->next; - count++; - } - /* Anti-fuzzer measure. Don't cache more than 5 messages. */ - if (count < 5) - { - *m = bfd_malloc (sizeof (**m) + alloc); - if (*m != NULL) - (*m)->next = NULL; - } - return m; -} - /* Find a target vector, given a name or configuration triplet. */ static const bfd_target * From patchwork Mon Apr 15 18:22:19 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 88510 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 201443844748 for ; Mon, 15 Apr 2024 18:25:22 +0000 (GMT) X-Original-To: binutils@sourceware.org Delivered-To: binutils@sourceware.org Received: from omta34.uswest2.a.cloudfilter.net (omta34.uswest2.a.cloudfilter.net [35.89.44.33]) by sourceware.org (Postfix) with ESMTPS id A41A8385840E for ; Mon, 15 Apr 2024 18:23:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A41A8385840E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org A41A8385840E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=35.89.44.33 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713205420; cv=none; b=ZAs2Iezc2NG2Oa16S67IMsqTVyLhX4ijDmAPbJPD79UJXhxwVeuUvcDFzU7Vu0A2qCs/HGUhcQo2mo5hzQJ1eCnQoucFVITJ9DQ6ee9UyOsoQkVkJNFaQkgXiPuw6/V7TauTXnXqxk6EJfRu1LZHn8iXT+O1uN9Benq+4XwAHiY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713205420; c=relaxed/simple; bh=yskwG8IWa9sDt/XlPiMcVEfJH+tCcS8x2DS80cxKzzc=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=KLfHfKcZL7JOxKUHFij0RgSCOqsawgqADVd8joG+Cogyen3LxHCX8q8yebNRbXugidzQHap/lsjPhogAygq4CmXh5us+Et0gpt/0kUuzSv9yrWvoj/C9jd2BRPBwYy7oh8htItMvj+w7hCUJtmFdC4n1ec9meOEG29W/K7DJzpU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from eig-obgw-6009a.ext.cloudfilter.net ([10.0.30.184]) by cmsmtp with ESMTPS id wOmDrWramHXmAwQzQrz6Sm; Mon, 15 Apr 2024 18:23:36 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id wQzQrw2Y1nNCOwQzQrZJQF; Mon, 15 Apr 2024 18:23:36 +0000 X-Authority-Analysis: v=2.4 cv=DKCJ4TNb c=1 sm=1 tr=0 ts=661d70a8 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=raytVjVEu-sA:10 a=Qbun_eYptAEA:10 a=CCpqsmhAAAAA:8 a=-X-rwvkNgfjKfmDDk8AA:9 a=ul9cdbp4aOFLsgKbc677:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Sender:Reply-To:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=THuiNqGbSRUe1aarUZ0RVzmBV0HLkcCddE9kCu3VFiU=; b=uGlyJ9KAhUK5mziXldp1YbK22b 7YHeQ8hBALs+SXGqU1xdrbhGaiYIo7kXpgt+1X6BmdaIMjIkAaaB/rOT1TOWDmHRE2EkK6r13Ubq9 wLLYnsuXMTliQ72yVq4/2+K8b; Received: from 97-122-82-115.hlrn.qwest.net ([97.122.82.115]:42076 helo=localhost.localdomain) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96.2) (envelope-from ) id 1rwQzP-000gh0-2l; Mon, 15 Apr 2024 12:23:35 -0600 From: Tom Tromey To: binutils@sourceware.org Cc: Tom Tromey Subject: [PATCH v2 2/2] Avoid cache race in bfd_check_format_matches Date: Mon, 15 Apr 2024 12:22:19 -0600 Message-ID: <20240415182328.2754398-3-tom@tromey.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240415182328.2754398-1-tom@tromey.com> References: <20240415182328.2754398-1-tom@tromey.com> MIME-Version: 1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 97.122.82.115 X-Source-L: No X-Exim-ID: 1rwQzP-000gh0-2l X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 97-122-82-115.hlrn.qwest.net (localhost.localdomain) [97.122.82.115]:42076 X-Source-Auth: tom+tromey.com X-Email-Count: 6 X-Org: HG=bhshared;ORG=bluehost; X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfJHzxm88HJNxGYiqux0B8guFyHPwEGHZKEKNTSo3gqjpTw2S3MR+bPVfIAoIKCLvMMQ72bpITF0Cg9DVJM6sXU0ATc2fno7+j8+HU2aVBYiWW3uLEsM0 kTExJ7ePcPmqBntNzRWE2desgQ7MtUBNw9y8Tp/OtBBp+L9N44ulDrhDsTDgqtvx1OK2KDBzurcTO6Rf9etsYfDD7hiuUg3PyEU= X-Spam-Status: No, score=-3021.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, JMQ_SPF_NEUTRAL, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP 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: binutils@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: binutils-bounces+patchwork=sourceware.org@sourceware.org Running the gdb test suite with the thread sanitizer enabled shows a race when bfd_check_format_matches and bfd_cache_close_all are called simultaneously on different threads. This patch fixes this race by having bfd_check_format_matches temporarily remove the BFD from the file descriptor cache -- leaving it open while format-checking proceeds. In this setup, the BFD client is responsible for closing the BFD again on the "checking" thread, should that be desired. gdb does this by calling bfd_cache_close in the relevant worker thread. An earlier version of this patch omitted the "possibly_cached" helper function. However, this ran into crashes in the binutils test suite involving the archive-checking abort in bfd_cache_lookup_worker. I do not understand the purpose of this check, so I've simply had the new function work around it. I couldn't find any comments explaining this situation, either. I suspect that there may still be races related to this case, but I don't think I have access to the platforms where gdb deals with archives. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264 --- bfd/bfd-in2.h | 6 ++++ bfd/bfd.c | 6 ++++ bfd/cache.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++--- bfd/format.c | 16 +++++++++- bfd/libbfd.h | 2 ++ 5 files changed, 109 insertions(+), 5 deletions(-) diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h index 109de767a39..e3b5a8b8522 100644 --- a/bfd/bfd-in2.h +++ b/bfd/bfd-in2.h @@ -2186,6 +2186,12 @@ struct bfd /* LTO object type. */ ENUM_BITFIELD (bfd_lto_object_type) lto_type : 2; + /* Set if this BFD is currently being processed by + bfd_check_format_matches. This is checked by the cache to + avoid closing the BFD in this case. This should only be + examined or modified while the BFD lock is held. */ + unsigned int in_format_matches : 1; + /* Set to dummy BFD created when claimed by a compiler plug-in library. */ bfd *plugin_dummy_bfd; diff --git a/bfd/bfd.c b/bfd/bfd.c index 80ce859d874..1d171274628 100644 --- a/bfd/bfd.c +++ b/bfd/bfd.c @@ -307,6 +307,12 @@ CODE_FRAGMENT . {* LTO object type. *} . ENUM_BITFIELD (bfd_lto_object_type) lto_type : 2; . +. {* Set if this BFD is currently being processed by +. bfd_check_format_matches. This is checked by the cache to +. avoid closing the BFD in this case. This should only be +. examined or modified while the BFD lock is held. *} +. unsigned int in_format_matches : 1; +. . {* Set to dummy BFD created when claimed by a compiler plug-in . library. *} . bfd *plugin_dummy_bfd; diff --git a/bfd/cache.c b/bfd/cache.c index 0f994c74239..5c825433b62 100644 --- a/bfd/cache.c +++ b/bfd/cache.c @@ -226,6 +226,20 @@ close_one (void) ? (FILE *) (bfd_last_cache->iostream) \ : bfd_cache_lookup_worker (x, flag)) +/* A helper function that returns true if ABFD can possibly be cached + -- that is, whether bfd_cache_lookup_worker will accept it. */ + +static bool +possibly_cached (bfd *abfd) +{ + if ((abfd->flags & BFD_IN_MEMORY) != 0) + return false; + if (abfd->my_archive != NULL + && !bfd_is_thin_archive (abfd->my_archive)) + return false; + return true; +} + /* Called when the macro <> fails to find a quick answer. Find a file descriptor for @var{abfd}. If necessary, it open it. If there are already more than @@ -236,12 +250,17 @@ close_one (void) static FILE * bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag) { - if ((abfd->flags & BFD_IN_MEMORY) != 0) + if (!possibly_cached (abfd)) abort (); - if (abfd->my_archive != NULL - && !bfd_is_thin_archive (abfd->my_archive)) - abort (); + /* If the BFD is being processed by bfd_check_format_matches, it + must already be open and won't be on the list. */ + if (abfd->in_format_matches) + { + if (abfd->iostream == NULL) + abort (); + return (FILE *) abfd->iostream; + } if (abfd->iostream != NULL) { @@ -654,6 +673,63 @@ bfd_cache_close_all (void) return ret; } +/* +INTERNAL_FUNCTION + bfd_cache_set_uncloseable + +SYNOPSIS + bool bfd_cache_set_uncloseable (bfd *abfd, bool value, bool *old); + +DESCRIPTION + Internal function to mark ABFD as either closeable or not. + This is used by bfd_check_format_matches to avoid races + where bfd_cache_close_all is called in another thread. + VALUE is true to mark the BFD as temporarily uncloseable + by the cache; false to mark it as closeable once again. + OLD, if non-NULL, is set to the previous value of the flag. + Returns false on error, true on success. +*/ + +bool +bfd_cache_set_uncloseable (bfd *abfd, bool value, bool *old) +{ + bool result = true; + + if (!bfd_lock ()) + return false; + if (old != NULL) + *old = abfd->in_format_matches; + + /* Only perform any action when the state changes,and only when this + BFD is actually using the cache. */ + if (value != abfd->in_format_matches + && abfd->iovec == &cache_iovec + && possibly_cached (abfd)) + { + if (value) + { + /* Marking as uncloseable for the first time. Ensure the + file is open, and remove from the cache list. */ + FILE *f = bfd_cache_lookup (abfd, CACHE_NORMAL); + if (f == NULL) + result = false; + else + snip (abfd); + } + else + { + /* Mark as closeable again. */ + insert (abfd); + } + + abfd->in_format_matches = value; + } + + if (!bfd_unlock ()) + return false; + return result; +} + /* FUNCTION bfd_cache_size diff --git a/bfd/format.c b/bfd/format.c index 2a700bab557..443fc6dbde0 100644 --- a/bfd/format.c +++ b/bfd/format.c @@ -86,6 +86,13 @@ DESCRIPTION o <> - more than one backend recognised the file format. + + When calling bfd_check_format (or bfd_check_format_matches), + any underlying file descriptor will be kept open for the + duration of the call. This is done to avoid races when + another thread calls bfd_cache_close_all. In this scenario, + the thread calling bfd_check_format must call bfd_cache_close + itself. */ bool @@ -383,6 +390,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) bfd_cleanup cleanup = NULL; struct per_xvec_messages messages = { abfd, PER_XVEC_NO_TARGET, NULL, NULL }; struct per_xvec_messages *orig_messages; + bool old_in_format_matches; if (matching != NULL) *matching = NULL; @@ -410,6 +418,11 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) return false; } + /* Avoid clashes with bfd_cache_close_all running in another + thread. */ + if (!bfd_cache_set_uncloseable (abfd, true, &old_in_format_matches)) + return false; + /* Presume the answer is yes. */ abfd->format = format; save_targ = abfd->xvec; @@ -665,7 +678,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) bfd_set_lto_type (abfd); /* File position has moved, BTW. */ - return true; + return bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL); } if (match_count == 0) @@ -708,6 +721,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching) bfd_preserve_restore (abfd, &preserve); _bfd_restore_error_handler_caching (orig_messages); print_and_clear_messages (&messages, PER_XVEC_NO_TARGET); + bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL); return false; } diff --git a/bfd/libbfd.h b/bfd/libbfd.h index 4f0acc48b49..da55528128c 100644 --- a/bfd/libbfd.h +++ b/bfd/libbfd.h @@ -1055,6 +1055,8 @@ void *bfd_arch_default_fill (bfd_size_type count, /* Extracted from cache.c. */ bool bfd_cache_init (bfd *abfd) ATTRIBUTE_HIDDEN; +bool bfd_cache_set_uncloseable (bfd *abfd, bool value, bool *old) ATTRIBUTE_HIDDEN; + FILE* bfd_open_file (bfd *abfd) ATTRIBUTE_HIDDEN; /* Extracted from hash.c. */