diff mbox series

[2/9,v2] libelf: Fix deadlock in elf_cntl

Message ID 20240717223409.1788719-3-amerey@redhat.com
State Superseded
Headers
Series Fix thread-safety for elfutils |

Commit Message

Aaron Merey July 17, 2024, 10:34 p.m. UTC 
  From: Heather McIntyre <hsm2@rice.edu>

        * libelf/elf_cntl.c (elf_cntl): Move rwlock_wrlock, rwlock_unlock,
        inside case switch statements.

Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
Signed-off-by: Aaron Merey <amerey@redhat.com>
Signed-off-by: Mark Wielaard <mark@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(-)

Comments

Mark Wielaard July 18, 2024, 5:55 p.m. UTC | #1 
Hi,

On Wed, 2024-07-17 at 18:34 -0400, Aaron Merey wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
>         * libelf/elf_cntl.c (elf_cntl): Move rwlock_wrlock, rwlock_unlock,
>         inside case switch statements.
> 
> Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> Signed-off-by: Mark Wielaard <mark@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
diff mbox series

Patch

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;
     }
 
-  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)
 	{
 	  /* 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:
@@ -73,7 +74,5 @@  elf_cntl (Elf *elf, Elf_Cmd cmd)
       break;
     }
 
-  rwlock_unlock (elf->lock);
-
   return result;
 }