[PATCH] elf_getdata_rawchunk.c: Fix dummy chunk insertion race condition

[Mark Wielaard]

Hi Aaron,

I did a quick check for this unlock read lock, aqcuire write lock
pattern in the code which we should double check/document/fix if
necessary: https://sourceware.org/bugzilla/show_bug.cgi?id=33382

On Thu, 2025-09-04 at 14:24 -0400, Aaron Merey wrote:
> When elf_getdata_rawchunk aquires a new chunk for the first time, it
> inserts a stack-allocated dummy chunk into a search_tree with an rdlock
> held.  When the real chunk is prepared to replace the dummy chunk, the
> rdlock is released and a wrlock is then held while replacing the
> dummy with the real chunk.
> 
> Before the wrlock is held, other threads could incorrectly acquire the
> dummy chunk as if it were a real chunk.
> 
> Fix this by holding a wrlock throughout elf_getdata_rawchunk.

This looks correct to me.

> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
>  libelf/elf_getdata_rawchunk.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/libelf/elf_getdata_rawchunk.c b/libelf/elf_getdata_rawchunk.c
> index 010fac90..87da912a 100644
> --- a/libelf/elf_getdata_rawchunk.c
> +++ b/libelf/elf_getdata_rawchunk.c
> @@ -87,7 +87,7 @@ elf_getdata_rawchunk (Elf *elf, int64_t offset, size_t size, Elf_Type type)
>    int flags = 0;
>    Elf_Data *result = NULL;
>  
> -  rwlock_rdlock (elf->lock);
> +  rwlock_wrlock (elf->lock);
>  
>    /* Maybe we already got this chunk?  */
>    Elf_Data_Chunk key;

OK, take write lock from the start.

> @@ -95,7 +95,7 @@ elf_getdata_rawchunk (Elf *elf, int64_t offset, size_t size, Elf_Type type)
>    key.data.d.d_size = size;
>    key.data.d.d_type = type;
>    Elf_Data_Chunk **found
> -    = eu_tsearch (&key, &elf->state.elf.rawchunk_tree, &chunk_compare);
> +    = eu_tsearch_nolock (&key, &elf->state.elf.rawchunk_tree, &chunk_compare);
>  
>    if (found == NULL)
>      goto nomem;

OK, this function is the only one using the rawchunk_tree (except for
elf_begin and elf_end of course). So using it unlocked while the elf
lock is held here is fine.

> @@ -136,7 +136,8 @@ elf_getdata_rawchunk (Elf *elf, int64_t offset, size_t size, Elf_Type type)
>        if (rawchunk == NULL)
>  	{
>  	nomem:
> -	  eu_tdelete (&key, &elf->state.elf.rawchunk_tree, &chunk_compare);
> +	  eu_tdelete_nolock (&key, &elf->state.elf.rawchunk_tree,
> +			     &chunk_compare);
>  	  __libelf_seterrno (ELF_E_NOMEM);
>  	  goto out;
>  	}
> @@ -147,7 +148,8 @@ elf_getdata_rawchunk (Elf *elf, int64_t offset, size_t size, Elf_Type type)
>  		    != size))
>  	{
>  	  /* Something went wrong.  */
> -	  eu_tdelete (&key, &elf->state.elf.rawchunk_tree, &chunk_compare);
> +	  eu_tdelete_nolock (&key, &elf->state.elf.rawchunk_tree,
> +			     &chunk_compare);
>  	  free (rawchunk);
>  	  __libelf_seterrno (ELF_E_READ_ERROR);
>  	  goto out;

OK. Same for using nolock for these two uses of rawchunk_tree.

> @@ -217,9 +219,6 @@ elf_getdata_rawchunk (Elf *elf, int64_t offset, size_t size, Elf_Type type)
>    chunk->data.d.d_version = EV_CURRENT;
>    chunk->offset = offset;
>  
> -  rwlock_unlock (elf->lock);
> -  rwlock_wrlock (elf->lock);
> -
>    *found = chunk;
>    result = &chunk->data.d;
>  

OK. The actual unlock is done at the end.

Thanks,

Mark