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

[Mark Wielaard <mark@klomp.org>]

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