diff mbox series

[4/9,v2] libdw: make dwarf_getalt thread-safe

Message ID	20240717223409.1788719-5-amerey@redhat.com
State	Superseded
Headers	DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CB6E2385ED4B From: Aaron Merey <amerey@redhat.com> To: elfutils-devel@sourceware.org Cc: hsm2@rice.edu, johnmc@rice.edu, Aaron Merey <amerey@redhat.com>, Mark Wielaard <mark@klomp.org> Subject: [PATCH 4/9 v2] libdw: make dwarf_getalt thread-safe Date: Wed, 17 Jul 2024 18:34:03 -0400 Message-ID: <20240717223409.1788719-5-amerey@redhat.com> In-Reply-To: <20240717223409.1788719-1-amerey@redhat.com> References: <20240717223409.1788719-1-amerey@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true Precedence: list Errors-To: elfutils-devel-bounces~patchwork=sourceware.org@sourceware.org
Series	Fix thread-safety for elfutils \| [0/9,v2] Fix thread-safety for elfutils [1/9,v2] libelf: Fix deadlock in __libelf_readall [2/9,v2] libelf: Fix deadlock in elf_cntl [3/9,v2] lib: Add eu_tsearch, eu_tfind, eu_tdelete and eu_tdestroy [4/9,v2] libdw: make dwarf_getalt thread-safe [5/9,v2] libdwP.h: Add locking to __libdw_dieabbrev, [6/9,v2] libdw: Make libdw_find_split_unit thread-safe [7/9,v2] libdw: Make libdw_findcu thread-safe [8/9,v2] tests: Add eu-search tests [9/9,v2] configure: No longer mark --enable-thread-safety as EXPERIMENTAL

Commit Message

Aaron Merey July 17, 2024, 10:34 p.m. UTC

  From: Heather McIntyre <hsm2@rice.edu>

	* libdw/dwarf_getalt.c (dwarf_getalt): Add locking.

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:

Write lock now applies to all of dwarf_getalt.
---
 libdw/dwarf_getalt.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Mark Wielaard July 18, 2024, 8:40 p.m. UTC | #1

Hi,

On Wed, Jul 17, 2024 at 06:34:03PM -0400, Aaron Merey wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
> 	* libdw/dwarf_getalt.c (dwarf_getalt): Add locking.
> 
> 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:
> 
> Write lock now applies to all of dwarf_getalt.

This looks good, but two nitpicks.

> ---
>  libdw/dwarf_getalt.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c
> index 0a12dfae..3b827d0b 100644
> --- a/libdw/dwarf_getalt.c
> +++ b/libdw/dwarf_getalt.c
> @@ -160,15 +160,27 @@ find_debug_altlink (Dwarf *dbg)
>      }
>  }
>  
> +/* find_debug_altlink() modifies "dbg->alt_dwarf".
> +   dwarf_getalt() reads "main->alt_dwarf".
> +   Mutual exclusion is enforced to prevent a race. */
> +
>  Dwarf *
>  dwarf_getalt (Dwarf *main)
>  {
> +  rwlock_wrlock(main->dwarf_lock);
> +
>    /* Only try once.  */
>    if (main == NULL || main->alt_dwarf == (void *) -1)
> -    return NULL;
> +    {
> +      rwlock_unlock (main->dwarf_lock);
> +      return NULL;
> +    }

The main == NULL check has to be done before taking the lock since
that would cause an SEGV otherwise.

The return on NULL input (without setting error) is a common pattern
in libdw, it allows to "chain" calls where some might fail (set error)
and return NULL. The chained call then also returns NULL (and you get
the original error).

>    if (main->alt_dwarf != NULL)
> -    return main->alt_dwarf;
> +    {
> +      rwlock_unlock (main->dwarf_lock);
> +      return main->alt_dwarf;
> +    }
>  
>    find_debug_altlink (main);
>  
> @@ -176,9 +188,11 @@ dwarf_getalt (Dwarf *main)
>    if (main->alt_dwarf == NULL)
>      {
>        main->alt_dwarf = (void *) -1;
> +      rwlock_unlock (main->dwarf_lock);
>        return NULL;
>      }
>  
> +  rwlock_unlock (main->dwarf_lock);
>    return main->alt_dwarf;
>  }

In both these unlock and return main->alt_dwarf I think this is fine
in practice, since they are done after the final write to
alt_dwarf. But to make absolutely sure we don't read/write to variable
we should only access under a lock I like the pattern of defining a
Dwarf *result; assign result = main->alt_dwarf before dropping the
lock, then return result. Call me paranoid :)

Cheers,

Mark

diff mbox series

Patch

diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c
index 0a12dfae..3b827d0b 100644
--- a/libdw/dwarf_getalt.c
+++ b/libdw/dwarf_getalt.c
@@ -160,15 +160,27 @@  find_debug_altlink (Dwarf *dbg)
     }
 }
 
+/* find_debug_altlink() modifies "dbg->alt_dwarf".
+   dwarf_getalt() reads "main->alt_dwarf".
+   Mutual exclusion is enforced to prevent a race. */
+
 Dwarf *
 dwarf_getalt (Dwarf *main)
 {
+  rwlock_wrlock(main->dwarf_lock);
+
   /* Only try once.  */
   if (main == NULL || main->alt_dwarf == (void *) -1)
-    return NULL;
+    {
+      rwlock_unlock (main->dwarf_lock);
+      return NULL;
+    }
 
   if (main->alt_dwarf != NULL)
-    return main->alt_dwarf;
+    {
+      rwlock_unlock (main->dwarf_lock);
+      return main->alt_dwarf;
+    }
 
   find_debug_altlink (main);
 
@@ -176,9 +188,11 @@  dwarf_getalt (Dwarf *main)
   if (main->alt_dwarf == NULL)
     {
       main->alt_dwarf = (void *) -1;
+      rwlock_unlock (main->dwarf_lock);
       return NULL;
     }
 
+  rwlock_unlock (main->dwarf_lock);
   return main->alt_dwarf;
 }
 INTDEF (dwarf_getalt)