From 1185b5b79a12ba67eb60bee3f75babf7a222fde0 Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Tue, 5 Sep 2023 19:05:40 -0600 Subject: [PATCH] Add minimal thread-safety to BFD This patch provides some minimal thread-safety to BFD. The BFD client can request thread-safety by providing a lock and unlock function. The globals used during BFD creation (e.g., bfd_id_counter) are then locked, and the file descriptor cache is also locked. A function to clean up any thread-local data is now provided for BFD clients. * bfd-in2.h: Regenerate. * bfd.c (lock_fn, unlock_fn): New globals. (bfd_thread_init, bfd_thread_cleanup, bfd_lock, bfd_unlock): New functions. * cache.c (bfd_cache_lookup_worker): Use _bfd_open_file_unlocked. (cache_btell, cache_bseek, cache_bread, cache_bwrite): Lock and unlock. (cache_bclose): Add comment. (cache_bflush, cache_bstat, cache_bmmap): Lock and unlock. (_bfd_cache_init_unlocked): New function. (bfd_cache_init): Use it. Lock and unlock. (_bfd_cache_close_unlocked): New function. (bfd_cache_close, bfd_cache_close_all): Use it. Lock and unlock. (_bfd_open_file_unlocked): New function. (bfd_open_file): Use it. Lock and unlock. * doc/bfd.texi (BFD front end): Add Threading menu item. * libbfd.h: Regenerate. * opncls.c (_bfd_new_bfd): Lock and unlock. * po/bfd.pot: Regenerate. --- bfd/bfd-in2.h | 8 ++ bfd/bfd.c | 131 +++++++++++++++++++++++++++++++- bfd/cache.c | 193 +++++++++++++++++++++++++++++++++++------------ bfd/doc/bfd.texi | 1 + bfd/libbfd.h | 4 + bfd/opncls.c | 7 ++ bfd/po/bfd.pot | 10 +-- 7 files changed, 299 insertions(+), 55 deletions(-) diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h index 96eef92fdc7..79f7f24436c 100644 --- a/bfd/bfd-in2.h +++ b/bfd/bfd-in2.h @@ -2572,6 +2572,14 @@ unsigned int bfd_init (void); /* Value returned by bfd_init. */ #define BFD_INIT_MAGIC (sizeof (struct bfd_section)) +typedef bool (*bfd_lock_unlock_fn_type) (void *); +bool bfd_thread_init + (bfd_lock_unlock_fn_type lock, + bfd_lock_unlock_fn_type unlock, + void *data); + +void bfd_thread_cleanup (void); + long bfd_get_reloc_upper_bound (bfd *abfd, asection *sect); long bfd_canonicalize_reloc diff --git a/bfd/bfd.c b/bfd/bfd.c index 2cf8361caa2..99189e0b5ac 100644 --- a/bfd/bfd.c +++ b/bfd/bfd.c @@ -1716,7 +1716,7 @@ bfd_set_assert_handler (bfd_assert_handler_type pnew) /* INODE -Initialization, Miscellaneous, Error reporting, BFD front end +Initialization, Threading, Error reporting, BFD front end FUNCTION bfd_init @@ -1749,9 +1749,136 @@ bfd_init (void) return BFD_INIT_MAGIC; } + +/* +INODE +Threading, Miscellaneous, Initialization, BFD front end + +SECTION + Threading + + BFD has limited support for thread-safety. Most BFD globals + are protected by locks, while the error-related globals are + thread-local. A given BFD cannot safely be used from two + threads at the same time; it is up to the application to do + any needed locking. However, it is ok for different threads + to work on different BFD objects at the same time. + +SUBSECTION + Thread functions. + +CODE_FRAGMENT +.typedef bool (*bfd_lock_unlock_fn_type) (void *); +*/ + +/* The lock and unlock functions, if set. */ +static bfd_lock_unlock_fn_type lock_fn; +static bfd_lock_unlock_fn_type unlock_fn; +static void *lock_data; + +/* +FUNCTION + bfd_thread_init + +SYNOPSIS + bool bfd_thread_init + (bfd_lock_unlock_fn_type lock, + bfd_lock_unlock_fn_type unlock, + void *data); + +DESCRIPTION + + Initialize BFD threading. The functions passed in will be + used to lock and unlock global data structures. This may only + be called a single time in a given process. Returns true on + success and false on error. DATA is passed verbatim to the + lock and unlock functions. The lock and unlock functions + should return true on success, or set the BFD error and return + false on failure. +*/ + +bool +bfd_thread_init (bfd_lock_unlock_fn_type lock, bfd_lock_unlock_fn_type unlock, + void *data) +{ + /* Both functions must be set, and this cannot have been called + before. */ + if (lock == NULL || unlock == NULL || unlock_fn != NULL) + { + bfd_set_error (bfd_error_invalid_operation); + return false; + } + + lock_fn = lock; + unlock_fn = unlock; + lock_data = data; + return true; +} + +/* +FUNCTION + bfd_thread_cleanup + +SYNOPSIS + void bfd_thread_cleanup (void); + +DESCRIPTION + Clean up any thread-local state. This should be called by a + thread that uses any BFD functions, before the thread exits. + It is fine to call this multiple times, or to call it and then + later call BFD functions on the same thread again. +*/ + +void +bfd_thread_cleanup (void) +{ + _bfd_clear_error_data (); +} + +/* +INTERNAL_FUNCTION + bfd_lock + +SYNOPSIS + bool bfd_lock (void); + +DESCRIPTION + Acquire the global BFD lock, if needed. Returns true on + success, false on error. +*/ + +bool +bfd_lock (void) +{ + if (lock_fn != NULL) + return lock_fn (lock_data); + return true; +} + +/* +INTERNAL_FUNCTION + bfd_unlock + +SYNOPSIS + bool bfd_unlock (void); + +DESCRIPTION + Release the global BFD lock, if needed. Returns true on + success, false on error. +*/ + +bool +bfd_unlock (void) +{ + if (unlock_fn != NULL) + return unlock_fn (lock_data); + return true; +} + + /* INODE -Miscellaneous, Memory Usage, Initialization, BFD front end +Miscellaneous, Memory Usage, Threading, BFD front end SECTION Miscellaneous diff --git a/bfd/cache.c b/bfd/cache.c index 3d26bee0773..a392bd28e18 100644 --- a/bfd/cache.c +++ b/bfd/cache.c @@ -49,6 +49,8 @@ SUBSECTION #include #endif +static FILE *_bfd_open_file_unlocked (bfd *abfd); + /* In some cases we can optimize cache operation when reopening files. For instance, a flush is entirely unnecessary if the file is already closed, so a flush would use CACHE_NO_OPEN. Similarly, a seek using @@ -259,7 +261,7 @@ bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag) if (flag & CACHE_NO_OPEN) return NULL; - if (bfd_open_file (abfd) == NULL) + if (_bfd_open_file_unlocked (abfd) == NULL) ; else if (!(flag & CACHE_NO_SEEK) && _bfd_real_fseek ((FILE *) abfd->iostream, @@ -278,19 +280,36 @@ bfd_cache_lookup_worker (bfd *abfd, enum cache_flag flag) static file_ptr cache_btell (struct bfd *abfd) { + if (!bfd_lock ()) + return -1; FILE *f = bfd_cache_lookup (abfd, CACHE_NO_OPEN); if (f == NULL) - return abfd->where; - return _bfd_real_ftell (f); + { + if (!bfd_unlock ()) + return -1; + return abfd->where; + } + file_ptr result = _bfd_real_ftell (f); + if (!bfd_unlock ()) + return -1; + return result; } static int cache_bseek (struct bfd *abfd, file_ptr offset, int whence) { + if (!bfd_lock ()) + return -1; FILE *f = bfd_cache_lookup (abfd, whence != SEEK_CUR ? CACHE_NO_SEEK : CACHE_NORMAL); if (f == NULL) + { + bfd_unlock (); + return -1; + } + int result = _bfd_real_fseek (f, offset, whence); + if (!bfd_unlock ()) return -1; - return _bfd_real_fseek (f, offset, whence); + return result; } /* Note that archive entries don't have streams; they share their parent's. @@ -338,12 +357,17 @@ cache_bread_1 (FILE *f, void *buf, file_ptr nbytes) static file_ptr cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes) { + if (!bfd_lock ()) + return -1; file_ptr nread = 0; FILE *f; f = bfd_cache_lookup (abfd, CACHE_NORMAL); if (f == NULL) - return -1; + { + bfd_unlock (); + return -1; + } /* Some filesystems are unable to handle reads that are too large (for instance, NetApp shares with oplocks turned off). To avoid @@ -374,57 +398,84 @@ cache_bread (struct bfd *abfd, void *buf, file_ptr nbytes) break; } + if (!bfd_unlock ()) + return -1; return nread; } static file_ptr cache_bwrite (struct bfd *abfd, const void *from, file_ptr nbytes) { + if (!bfd_lock ()) + return -1; file_ptr nwrite; FILE *f = bfd_cache_lookup (abfd, CACHE_NORMAL); if (f == NULL) - return 0; + { + if (!bfd_unlock ()) + return -1; + return 0; + } nwrite = fwrite (from, 1, nbytes, f); if (nwrite < nbytes && ferror (f)) { bfd_set_error (bfd_error_system_call); + bfd_unlock (); return -1; } + if (!bfd_unlock ()) + return -1; return nwrite; } static int cache_bclose (struct bfd *abfd) { + /* No locking needed here, it's handled by the callee. */ return bfd_cache_close (abfd) - 1; } static int cache_bflush (struct bfd *abfd) { + if (!bfd_lock ()) + return -1; int sts; FILE *f = bfd_cache_lookup (abfd, CACHE_NO_OPEN); if (f == NULL) - return 0; + { + if (!bfd_unlock ()) + return -1; + return 0; + } sts = fflush (f); if (sts < 0) bfd_set_error (bfd_error_system_call); + if (!bfd_unlock ()) + return -1; return sts; } static int cache_bstat (struct bfd *abfd, struct stat *sb) { + if (!bfd_lock ()) + return -1; int sts; FILE *f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR); if (f == NULL) - return -1; + { + bfd_unlock (); + return -1; + } sts = fstat (fileno (f), sb); if (sts < 0) bfd_set_error (bfd_error_system_call); + if (!bfd_unlock ()) + return -1; return sts; } @@ -440,6 +491,8 @@ cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED, { void *ret = (void *) -1; + if (!bfd_lock ()) + return ret; if ((abfd->flags & BFD_IN_MEMORY) != 0) abort (); #ifdef HAVE_MMAP @@ -452,7 +505,10 @@ cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED, f = bfd_cache_lookup (abfd, CACHE_NO_SEEK_ERROR); if (f == NULL) - return ret; + { + bfd_unlock (); + return ret; + } if (pagesize_m1 == 0) pagesize_m1 = getpagesize () - 1; @@ -473,6 +529,8 @@ cache_bmmap (struct bfd *abfd ATTRIBUTE_UNUSED, } #endif + if (!bfd_unlock ()) + return (void *) -1; return ret; } @@ -482,6 +540,22 @@ static const struct bfd_iovec cache_iovec = &cache_bclose, &cache_bflush, &cache_bstat, &cache_bmmap }; +static bool +_bfd_cache_init_unlocked (bfd *abfd) +{ + BFD_ASSERT (abfd->iostream != NULL); + if (open_files >= bfd_cache_max_open ()) + { + if (! close_one ()) + return false; + } + abfd->iovec = &cache_iovec; + insert (abfd); + abfd->flags &= ~BFD_CLOSED_BY_CACHE; + ++open_files; + return true; +} + /* INTERNAL_FUNCTION bfd_cache_init @@ -496,17 +570,28 @@ DESCRIPTION bool bfd_cache_init (bfd *abfd) { - BFD_ASSERT (abfd->iostream != NULL); - if (open_files >= bfd_cache_max_open ()) - { - if (! close_one ()) - return false; - } - abfd->iovec = &cache_iovec; - insert (abfd); - abfd->flags &= ~BFD_CLOSED_BY_CACHE; - ++open_files; - return true; + if (!bfd_lock ()) + return false; + bool result = _bfd_cache_init_unlocked (abfd); + if (!bfd_unlock ()) + return false; + return result; +} + +static bool +_bfd_cache_close_unlocked (bfd *abfd) +{ + /* Don't remove this test. bfd_reinit depends on it. */ + if (abfd->iovec != &cache_iovec) + return true; + + if (abfd->iostream == NULL) + /* Previously closed. */ + return true; + + /* Note: no locking needed in this function, as it is handled by + bfd_cache_delete. */ + return bfd_cache_delete (abfd); } /* @@ -527,15 +612,12 @@ DESCRIPTION bool bfd_cache_close (bfd *abfd) { - /* Don't remove this test. bfd_reinit depends on it. */ - if (abfd->iovec != &cache_iovec) - return true; - - if (abfd->iostream == NULL) - /* Previously closed. */ - return true; - - return bfd_cache_delete (abfd); + if (!bfd_lock ()) + return false; + bool result = _bfd_cache_close_unlocked (abfd); + if (!bfd_unlock ()) + return false; + return result; } /* @@ -560,11 +642,13 @@ bfd_cache_close_all (void) { bool ret = true; + if (!bfd_lock ()) + return false; while (bfd_last_cache != NULL) { bfd *prev_bfd_last_cache = bfd_last_cache; - ret &= bfd_cache_close (bfd_last_cache); + ret &= _bfd_cache_close_unlocked (bfd_last_cache); /* Stop a potential infinite loop should bfd_cache_close() not update bfd_last_cache. */ @@ -572,6 +656,8 @@ bfd_cache_close_all (void) break; } + if (!bfd_unlock ()) + return false; return ret; } @@ -592,23 +678,8 @@ bfd_cache_size (void) return open_files; } -/* -INTERNAL_FUNCTION - bfd_open_file - -SYNOPSIS - FILE* bfd_open_file (bfd *abfd); - -DESCRIPTION - Call the OS to open a file for @var{abfd}. Return the <> - (possibly <>) that results from this operation. Set up the - BFD so that future accesses know the file is open. If the <> - returned is <>, then it won't have been put in the - cache, so it won't have to be removed from it. -*/ - -FILE * -bfd_open_file (bfd *abfd) +static FILE * +_bfd_open_file_unlocked (bfd *abfd) { abfd->cacheable = true; /* Allow it to be closed later. */ @@ -673,9 +744,35 @@ bfd_open_file (bfd *abfd) bfd_set_error (bfd_error_system_call); else { - if (! bfd_cache_init (abfd)) + if (! _bfd_cache_init_unlocked (abfd)) return NULL; } return (FILE *) abfd->iostream; } + +/* +INTERNAL_FUNCTION + bfd_open_file + +SYNOPSIS + FILE* bfd_open_file (bfd *abfd); + +DESCRIPTION + Call the OS to open a file for @var{abfd}. Return the <> + (possibly <>) that results from this operation. Set up the + BFD so that future accesses know the file is open. If the <> + returned is <>, then it won't have been put in the + cache, so it won't have to be removed from it. +*/ + +FILE * +bfd_open_file (bfd *abfd) +{ + if (!bfd_lock ()) + return NULL; + FILE *result = _bfd_open_file_unlocked (abfd); + if (!bfd_unlock ()) + return NULL; + return result; +} diff --git a/bfd/doc/bfd.texi b/bfd/doc/bfd.texi index f348710845f..3f70cb73a1d 100644 --- a/bfd/doc/bfd.texi +++ b/bfd/doc/bfd.texi @@ -199,6 +199,7 @@ IEEE-695. * typedef bfd:: * Error reporting:: * Initialization:: +* Threading:: * Miscellaneous:: * Memory Usage:: * Sections:: diff --git a/bfd/libbfd.h b/bfd/libbfd.h index 498ce8d95b5..cc432677a81 100644 --- a/bfd/libbfd.h +++ b/bfd/libbfd.h @@ -937,6 +937,10 @@ bfd_error_handler_type _bfd_set_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN; const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN; +bool bfd_lock (void) ATTRIBUTE_HIDDEN; + +bool bfd_unlock (void) ATTRIBUTE_HIDDEN; + /* Extracted from bfdio.c. */ struct bfd_iovec { diff --git a/bfd/opncls.c b/bfd/opncls.c index cddfd7ec1fb..5a77562744c 100644 --- a/bfd/opncls.c +++ b/bfd/opncls.c @@ -81,6 +81,8 @@ _bfd_new_bfd (void) if (nbfd == NULL) return NULL; + if (!bfd_lock ()) + return NULL; if (bfd_use_reserved_id) { nbfd->id = --bfd_reserved_id_counter; @@ -88,6 +90,11 @@ _bfd_new_bfd (void) } else nbfd->id = bfd_id_counter++; + if (!bfd_unlock ()) + { + free (nbfd); + return NULL; + } nbfd->memory = objalloc_create (); if (nbfd->memory == NULL) diff --git a/bfd/po/bfd.pot b/bfd/po/bfd.pot index 6c979791ef7..bc428dad3b2 100644 --- a/bfd/po/bfd.pot +++ b/bfd/po/bfd.pot @@ -231,22 +231,22 @@ msgstr "" msgid "#" msgstr "" -#: bfd.c:1892 +#: bfd.c:2019 #, c-format msgid "BFD %s assertion fail %s:%d" msgstr "" -#: bfd.c:1905 +#: bfd.c:2032 #, c-format msgid "BFD %s internal error, aborting at %s:%d in %s\n" msgstr "" -#: bfd.c:1910 +#: bfd.c:2037 #, c-format msgid "BFD %s internal error, aborting at %s:%d\n" msgstr "" -#: bfd.c:1912 +#: bfd.c:2039 msgid "Please report this bug.\n" msgstr "" @@ -265,7 +265,7 @@ msgstr "" msgid "warning: writing section `%pA' at huge (ie negative) file offset" msgstr "" -#: cache.c:273 +#: cache.c:275 #, c-format msgid "reopening %pB: %s" msgstr "" -- 2.30.2