[PATCH 1/2] Thread-safety improvements for bfd_check_format_matches

[Alan Modra]

On Sun, Mar 24, 2024 at 03:08:05PM -0600, Tom Tromey wrote:
> 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.

I suspect you have changed things a little.  The curent per_xvec_warn
array has an extra element to cache messages emitted when xvec is
NULL.  I added that just in case such a thing ever happened.  I don't
think it does, so I'm not at all concerned about a change in behaviour
when xvec is NULL.

However, the new _bfd_per_xvec_warn does seem to be a little weird
should messages->abfd-xvec ever be NULL.  It seems it will create a
new entry in the list for a NULL targ that then might be reused for a
later non-NULL targ message.  Also, the description for the new
_bfd_per_xvec_warn needs updating and maybe the function should be
made static and moved from targets.c to bfd.c.

> 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(-)
> 

>  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.
>  */

non-NULL BFD?

-- 
Alan Modra
Australia Development Lab, IBM