* [PATCH 1/2] Thread-safety improvements for bfd_check_format_matches
2024-03-24 21:08 [PATCH 0/2] Fix some races seen by thread sanitizer Tom Tromey
@ 2024-03-24 21:08 ` Tom Tromey
2024-03-29 18:17 ` Tom Tromey
2024-04-10 3:39 ` Alan Modra
2024-03-24 21:08 ` [PATCH 2/2] Avoid cache race in bfd_check_format_matches Tom Tromey
` (2 subsequent siblings)
3 siblings, 2 replies; 9+ messages in thread
From: Tom Tromey @ 2024-03-24 21:08 UTC (permalink / raw)
To: binutils; +Cc: Tom Tromey
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. I believe it does not change
the current semantics, but I was unable to get the test file that
would let me test that.
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.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264
---
bfd/bfd.c | 68 ++++++++++++++++++++++++++++++++++++++-----------
bfd/format.c | 70 +++++++++++++++++++++++----------------------------
bfd/libbfd.h | 17 +++++++++++--
bfd/targets.c | 59 ++++++++++++++++++++++++++++++-------------
4 files changed, 142 insertions(+), 72 deletions(-)
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 54061a34240..11ad7f24456 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1534,10 +1534,17 @@ 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. */
+/* 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 bfd *error_handler_bfd;
+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. */
@@ -1554,12 +1561,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;
}
}
@@ -1597,7 +1604,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);
}
@@ -1628,18 +1642,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_warn array. Returns the previous BFD to which
+ messages are stored. Note that two sequential calls to this
+ with a non-NULL BFD 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 function 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 8f3fc7e7b96..5a5b01975ac 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -272,10 +272,33 @@ 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 non-NULL, then any
+ messages in LIST with a matching target are issued. If TARG is
+ NULL, then messages are only reissued if they are all associated
+ with a single BFD target. */
+
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 == NULL && 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;
+ }
}
/*
@@ -313,8 +336,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, NULL, NULL, NULL };
+ struct per_xvec_messages *orig_messages;
if (matching != NULL)
*matching = NULL;
@@ -345,11 +368,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))
@@ -591,15 +610,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);
/* File position has moved, BTW. */
return true;
@@ -643,27 +656,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, NULL);
return false;
}
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index f15b5f27db8..04dbe720edb 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -934,7 +934,9 @@ 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;
+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;
@@ -3699,7 +3701,18 @@ struct per_xvec_message
char message[];
};
-struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t) ATTRIBUTE_HIDDEN;
+/* A list of per_xvec_message objects. The targ field
+ indicates which xvec this list holds. 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;
+};
+
+struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t) ATTRIBUTE_HIDDEN;
#ifdef __cplusplus
}
diff --git a/bfd/targets.c b/bfd/targets.c
index f94992d031d..baf3cc5266c 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -1457,9 +1457,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];
\f
/* This array maps configuration triplets onto BFD vectors. */
@@ -1488,11 +1485,22 @@ INTERNAL
. char message[];
.};
.
+.{* A list of per_xvec_message objects. The targ field
+. indicates which xvec this list holds. 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;
+.};
+.
INTERNAL_FUNCTION
_bfd_per_xvec_warn
SYNOPSIS
- struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t);
+ struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t);
DESCRIPTION
Return a location for the given target xvec to use for
@@ -1505,19 +1513,36 @@ DESCRIPTION
pointer to a NULL pointer on allocation failure.
*/
-struct per_xvec_message **
-_bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
+struct per_xvec_message *
+_bfd_per_xvec_warn (struct per_xvec_messages *messages, 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;
+ 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 == NULL)
+ 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)
{
@@ -1531,7 +1556,7 @@ _bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
if (*m != NULL)
(*m)->next = NULL;
}
- return m;
+ return *m;
}
/* Find a target vector, given a name or configuration triplet. */
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] Avoid cache race in bfd_check_format_matches
2024-03-24 21:08 [PATCH 0/2] Fix some races seen by thread sanitizer Tom Tromey
2024-03-24 21:08 ` [PATCH 1/2] Thread-safety improvements for bfd_check_format_matches Tom Tromey
@ 2024-03-24 21:08 ` Tom Tromey
2024-04-10 3:46 ` Alan Modra
2024-04-09 16:13 ` [PATCH 0/2] Fix some races seen by thread sanitizer Tom Tromey
2024-04-12 13:03 ` Nick Clifton
3 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2024-03-24 21:08 UTC (permalink / raw)
To: binutils; +Cc: Tom Tromey
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 fa28688837c..ceb53683d21 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -2160,6 +2160,12 @@ struct bfd
that BFD is not prepared to handle for objcopy/strip. */
unsigned int read_only : 1;
+ /* 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 11ad7f24456..ca4250327d2 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -285,6 +285,12 @@ CODE_FRAGMENT
. that BFD is not prepared to handle for objcopy/strip. *}
. unsigned int read_only : 1;
.
+. {* 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 d0e7be293a5..c526dcae09f 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 <<bfd_cache_lookup>> 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)
{
@@ -657,6 +676,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 5a5b01975ac..238a76f14b9 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -86,6 +86,13 @@ DESCRIPTION
o <<bfd_error_file_ambiguously_recognized>> -
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
@@ -338,6 +345,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
bfd_cleanup cleanup = NULL;
struct per_xvec_messages messages = { abfd, NULL, NULL, NULL };
struct per_xvec_messages *orig_messages;
+ bool old_in_format_matches;
if (matching != NULL)
*matching = NULL;
@@ -362,6 +370,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;
@@ -615,7 +628,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
print_and_clear_messages (&messages, abfd->xvec);
/* File position has moved, BTW. */
- return true;
+ return bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
}
if (match_count == 0)
@@ -658,6 +671,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, NULL);
+ bfd_cache_set_uncloseable (abfd, old_in_format_matches, NULL);
return false;
}
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index 04dbe720edb..5863a658f8a 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -1017,6 +1017,8 @@ bfd_window_internal;
/* 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. */
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread