[03/16] libelf: Fix deadlock in __libelf_readall

  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(-)
  

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
  

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
>
  

Hi Heather,

On Tue, 2023-10-17 at 14:11 -0500, Heather McIntyre wrote:
> I will push the changes after I am done testing other fixes
> since I want to ensure that everything works together cohesively. 

Please let us know if we can help update your patches. I realize we
have been patching some of the code you are also working on. So if
things don't apply anymore and you need help to figure out how to
adjust your patches please just ask.

Also please don't feel everything needs to be perfect and work together
completely. If you have parts ready that are independently useful
please just submit those parts and we can integrate them early.

Thanks,

Mark