Hi Heather, On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote: > From: Heather McIntyre <h...@rice.edu> > > * libelf/elf_readall.c (__libelf_readall): Move rwlock_unlock > before libelf_acquire_all. > > Signed-off-by: Heather S. McIntyre <h...@rice.edu> > Signed-off-by: Mark Wielaard <m...@klomp.org> > --- > libelf/elf_readall.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/libelf/elf_readall.c b/libelf/elf_readall.c > index d0f9a28c..2d62d447 100644 > --- a/libelf/elf_readall.c > +++ b/libelf/elf_readall.c > @@ -84,6 +84,7 @@ __libelf_readall (Elf *elf) > > /* If this is an archive and we have derived descriptors get the > locks for all of them. */ > + rwlock_unlock(elf->lock); // lock will be reacquired next line > libelf_acquire_all (elf); > > if (elf->maximum_size == ~((size_t) 0)) > @@ -141,10 +142,8 @@ __libelf_readall (Elf *elf) > __libelf_seterrno (ELF_E_NOMEM); > > /* Free the locks on the children. */ > - libelf_release_all (elf); > + libelf_release_all (elf); // lock is released > } > > - rwlock_unlock (elf->lock); > - > return (char *) elf->map_address; > }
I think this is wrong when this if statement, at the start of the block, fails: /* If the file is not mmap'ed and not previously loaded, do it now. */ if (elf->map_address == NULL) So if elf->map_address != NULL we now never call rwlock_unlock (elf->lock). One way to simplify this locking might be to rewrite libelf_acquire_all and libelf_release_all to libelf_acquire_all_children and libelf_release_all_children (which would only be called with the elf- >lock already acquired). __libelf_readall is the only caller of these functions. Cheers, Mark
