Re: [PATCH 03/16] libelf: Fix deadlock in __libelf_readall

[Heather McIntyre <hsm2@rice.edu>]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

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 <mark@klomp.org> wrote:

> Hi Heather,
>
> On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> > From: Heather McIntyre <hsm2@rice.edu>
> >
> >       * libelf/elf_readall.c (__libelf_readall): Move rwlock_unlock
> >       before libelf_acquire_all.
> >
> > Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
> > Signed-off-by: Mark Wielaard <mark@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
>