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 <ame...@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
