Remove the rdlock held throughout elf_getscn and replace it with a wrlock that covers only the one-time initialization of section zero. This reduces rwlock overhead when on the hotpath and also fixes a race condition where a flag (runp->cnt) could be updated while the rdlock was being held, causing leaks and racing stores.
Atomic store/acquire is now used when accessing section zero outside of when a lock is being held. VALGRIND_HG_DISABLE_CHECKING is used on runp->cnt before its atomic_store_release call to prevent helgrind false positives since there is no definite order to whether lock-free loads of runp->cnt happen before or after this store. Signed-off-by: Aaron Merey <[email protected]> --- v2: move elf32_offscn.c changes into separate commit, add valgrind annotation libelf/elf_getscn.c | 58 ++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/libelf/elf_getscn.c b/libelf/elf_getscn.c index be9c76f0..599191f4 100644 --- a/libelf/elf_getscn.c +++ b/libelf/elf_getscn.c @@ -50,8 +50,6 @@ elf_getscn (Elf *elf, size_t idx) return NULL; } - rwlock_rdlock (elf->lock); - Elf_Scn *result = NULL; /* Find the section in the list. */ @@ -63,39 +61,52 @@ elf_getscn (Elf *elf, size_t idx) /* Section zero is special. It always exists even if there is no "first" section. And it is needed to store "overflow" values from the Elf header. */ - if (idx == 0 && runp->cnt == 0 && runp->max > 0) + if (idx == 0 && atomic_load_acquire (&runp->cnt) == 0 && runp->max > 0) { - Elf_Scn *scn0 = &runp->data[0]; - if (elf->class == ELFCLASS32) + rwlock_wrlock (elf->lock); + + /* Check whether section zero was set up before this thread acquired + the wrlock. */ + if (runp->cnt == 0) { - scn0->shdr.e32 = calloc (1, sizeof (Elf32_Shdr)); - if (scn0->shdr.e32 == NULL) + Elf_Scn *scn0 = &runp->data[0]; + if (elf->class == ELFCLASS32) { - __libelf_seterrno (ELF_E_NOMEM); - goto out; + scn0->shdr.e32 = calloc (1, sizeof (Elf32_Shdr)); + if (scn0->shdr.e32 == NULL) + { + __libelf_seterrno (ELF_E_NOMEM); + rwlock_unlock (elf->lock); + return NULL; + } } - } - else - { - scn0->shdr.e64 = calloc (1, sizeof (Elf64_Shdr)); - if (scn0->shdr.e64 == NULL) + else { - __libelf_seterrno (ELF_E_NOMEM); - goto out; + scn0->shdr.e64 = calloc (1, sizeof (Elf64_Shdr)); + if (scn0->shdr.e64 == NULL) + { + __libelf_seterrno (ELF_E_NOMEM); + rwlock_unlock (elf->lock); + return NULL; + } } + + scn0->elf = elf; + scn0->shdr_flags = ELF_F_DIRTY | ELF_F_MALLOCED; + scn0->list = elf->state.elf.scns_last; + scn0->data_read = 1; + VALGRIND_HG_DISABLE_CHECKING (&runp->cnt, sizeof (runp->cnt)); + atomic_store_release (&runp->cnt, 1); } - scn0->elf = elf; - scn0->shdr_flags = ELF_F_DIRTY | ELF_F_MALLOCED; - scn0->list = elf->state.elf.scns_last; - scn0->data_read = 1; - runp->cnt = 1; + + rwlock_unlock (elf->lock); } while (1) { if (idx < runp->max) { - if (idx < runp->cnt) + if (idx < atomic_load_acquire (&runp->cnt)) result = &runp->data[idx]; else __libelf_seterrno (ELF_E_INVALID_INDEX); @@ -112,9 +123,6 @@ elf_getscn (Elf *elf, size_t idx) } } - out: - rwlock_unlock (elf->lock); - return result; } INTDEF(elf_getscn) -- 2.55.0
