Hi,
On Wed, 2024-07-17 at 18:34 -0400, Aaron Merey wrote:
> From: Heather McIntyre <h...@rice.edu>
>
> * libelf/elf_cntl.c (elf_cntl): Move rwlock_wrlock, rwlock_unlock,
> inside case switch statements.
>
> Signed-off-by: Heather S. McIntyre <h...@rice.edu>
> Signed-off-by: Aaron Merey <ame...@redhat.com>
> Signed-off-by: Mark Wielaard <m...@klomp.org>
>
> ---
> v2 changes:
> Remove unnecessary locking and checking of elf->map_address
>
> libelf/elf_cntl.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/libelf/elf_cntl.c b/libelf/elf_cntl.c
> index 04aa9132..3fbc7d97 100644
> --- a/libelf/elf_cntl.c
> +++ b/libelf/elf_cntl.c
> @@ -48,13 +48,12 @@ elf_cntl (Elf *elf, Elf_Cmd cmd)
> return -1;
> }
>
Out of view of this patch, but in the line before this code does:
if (elf->fildes == -1)
{
__libelf_seterrno (ELF_E_INVALID_HANDLE);
return -1;
}
Below we are careful to take a lock (in __libelf_readall or explicitly
in the case of ELF_C_FDDONE, before reading or writing fildes.
So we either have to add a lock around this check too. Or maybe better
just remove this check. In the case of ELF_C_FDREAD __libelf_readall
will return NULL and so already produce an error. In the case of
ELF_C_FDDONE I would make the point that it doesn't matter, if the
fildes was already -1, then it still is. No need for error in that
case?
>
> - rwlock_wrlock (elf->lock);
>
> switch (cmd)
> {
> case ELF_C_FDREAD:
> /* If not all of the file is in the memory read it now. */
> - if (elf->map_address == NULL && __libelf_readall (elf) == NULL)
> + if (__libelf_readall (elf) == NULL)
This looks good, __libelf_readall (elf) already does that check (after
locking).
> {
> /* We were not able to read everything. */
> result = -1;
> @@ -64,7 +63,9 @@ elf_cntl (Elf *elf, Elf_Cmd cmd)
>
> case ELF_C_FDDONE:
> /* Mark the file descriptor as not usable. */
> + rwlock_wrlock (elf->lock);
> elf->fildes = -1;
> + rwlock_unlock (elf->lock);
> break;
>
> default:
This also looks correct.
Cheers,
Mark
