You are right that if elf->map_address != NULL then the acquired wrlock is not unlocked. The rwlock_unlock that was there initially was removed due to deadlocking when returning from inside the if statement, but this was not correct. However, adding ‘else rwlock_unlock (elf->lock)’ at the end of the if statement fixes this issue.
I rewrote libelf_acquire_all and libelf_release_all as per your suggestion. Now, libelf_acquire_all_children does not acquire the lock again for the current elf object, but it does acquire locks for all children. Similarly, libelf_release_all_children releases the locks for all children under the acquired elf->lock. In libelf_readall, the elf->lock for the current elf object is released after the call to libelf_release_all_children before returning from the function. All tests are still passing after I made these changes. I will push the changes after I am done testing other fixes since I want to ensure that everything works together cohesively. On Tue, Oct 10, 2023 at 10:06 AM Mark Wielaard <m...@klomp.org> wrote: > 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 >
