diff mbox series

[06/10,v3] libdwP.h: Add locking to dwarf_formref_die and __libdw_dieabbrev

Message ID 20240802233847.690564-6-amerey@redhat.com
State Committed
Delegated to: Mark Wielaard
Headers
Series [01/10,v3] libelf: Fix deadlock in __libelf_readall |

Commit Message

Aaron Merey Aug. 2, 2024, 11:38 p.m. UTC 
  From: Heather McIntyre <hsm2@rice.edu>

	* libdw/dwarf_formref_die.c (dwarf_formref_die): Add locking
	  around call to __libdw_intern_next_unit.
        * libdw/libdwP.h (__libdw_dieabbrev): 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>

v3 changes:
Move dwarf_setalt.c changes to patch 5/9 v3.
Set __libdw_dieabbrev result before releasing lock.
---
 libdw/dwarf_formref_die.c |  3 +++
 libdw/libdwP.h            | 25 +++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

Comments

Mark Wielaard Aug. 16, 2024, 9:51 p.m. UTC | #1 
On Fri, Aug 02, 2024 at 07:38:05PM -0400, Aaron Merey wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
> 	* libdw/dwarf_formref_die.c (dwarf_formref_die): Add locking
> 	  around call to __libdw_intern_next_unit.
>         * libdw/libdwP.h (__libdw_dieabbrev): 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>
> 
> v3 changes:
> Move dwarf_setalt.c changes to patch 5/9 v3.
> Set __libdw_dieabbrev result before releasing lock.
> ---
>  libdw/dwarf_formref_die.c |  3 +++
>  libdw/libdwP.h            | 25 +++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
> index 48ba8194..69d1bf1c 100644
> --- a/libdw/dwarf_formref_die.c
> +++ b/libdw/dwarf_formref_die.c
> @@ -92,7 +92,10 @@ dwarf_formref_die (Dwarf_Attribute *attr, Dwarf_Die *result)
>  	  bool scan_debug_types = false;
>  	  do
>  	    {
> +	      rwlock_wrlock(attr->cu->dbg->dwarf_lock);
>  	      cu = __libdw_intern_next_unit (attr->cu->dbg, scan_debug_types);
> +	      rwlock_unlock(attr->cu->dbg->dwarf_lock);
> +
>  	      if (cu == NULL)
>  		{
>  		  if (scan_debug_types == false)

Aha, sorry, so there is extra locking around __libdw_intern_next_unit
here. Shouldn't this be combined with the next patch?

Thanks,

Mark
Aaron Merey Aug. 21, 2024, 12:34 a.m. UTC | #2 
Hi Mark,

On Fri, Aug 16, 2024 at 5:51 PM Mark Wielaard <mark@klomp.org> wrote:
>
> On Fri, Aug 02, 2024 at 07:38:05PM -0400, Aaron Merey wrote:
> > From: Heather McIntyre <hsm2@rice.edu>
> >
> >       * libdw/dwarf_formref_die.c (dwarf_formref_die): Add locking
> >         around call to __libdw_intern_next_unit.
> >         * libdw/libdwP.h (__libdw_dieabbrev): 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>
> >
> > v3 changes:
> > Move dwarf_setalt.c changes to patch 5/9 v3.
> > Set __libdw_dieabbrev result before releasing lock.
> > ---
> >  libdw/dwarf_formref_die.c |  3 +++
> >  libdw/libdwP.h            | 25 +++++++++++++++++++++----
> >  2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
> > index 48ba8194..69d1bf1c 100644
> > --- a/libdw/dwarf_formref_die.c
> > +++ b/libdw/dwarf_formref_die.c
> > @@ -92,7 +92,10 @@ dwarf_formref_die (Dwarf_Attribute *attr, Dwarf_Die *result)
> >         bool scan_debug_types = false;
> >         do
> >           {
> > +           rwlock_wrlock(attr->cu->dbg->dwarf_lock);
> >             cu = __libdw_intern_next_unit (attr->cu->dbg, scan_debug_types);
> > +           rwlock_unlock(attr->cu->dbg->dwarf_lock);
> > +
> >             if (cu == NULL)
> >               {
> >                 if (scan_debug_types == false)
>
> Aha, sorry, so there is extra locking around __libdw_intern_next_unit
> here. Shouldn't this be combined with the next patch?

I moved these dwarf_formref_die.c into the patch "libdw: Make
libdw_find_split_unit thread-safe", which was pushed as commit
2630bce747. The rest of the changes were pushed as commit
28b74a1bf73.

Aaron
diff mbox series

Patch

diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
index 48ba8194..69d1bf1c 100644
--- a/libdw/dwarf_formref_die.c
+++ b/libdw/dwarf_formref_die.c
@@ -92,7 +92,10 @@  dwarf_formref_die (Dwarf_Attribute *attr, Dwarf_Die *result)
 	  bool scan_debug_types = false;
 	  do
 	    {
+	      rwlock_wrlock(attr->cu->dbg->dwarf_lock);
 	      cu = __libdw_intern_next_unit (attr->cu->dbg, scan_debug_types);
+	      rwlock_unlock(attr->cu->dbg->dwarf_lock);
+
 	      if (cu == NULL)
 		{
 		  if (scan_debug_types == false)
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index c95296e9..492cfe8c 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -801,15 +801,28 @@  static inline Dwarf_Abbrev *
 __nonnull_attribute__ (1)
 __libdw_dieabbrev (Dwarf_Die *die, const unsigned char **readp)
 {
+  if (unlikely (die->cu == NULL))
+    {
+      die->abbrev = DWARF_END_ABBREV;
+      return DWARF_END_ABBREV;
+    }
+
+  rwlock_wrlock (die->cu->abbrev_lock);
+
   /* Do we need to get the abbreviation, or need to read after the code?  */
   if (die->abbrev == NULL || readp != NULL)
     {
       /* Get the abbreviation code.  */
       unsigned int code;
       const unsigned char *addr = die->addr;
-      if (unlikely (die->cu == NULL
-		    || addr >= (const unsigned char *) die->cu->endp))
-	return die->abbrev = DWARF_END_ABBREV;
+
+      if (addr >= (const unsigned char *) die->cu->endp)
+	{
+	  die->abbrev = DWARF_END_ABBREV;
+	  rwlock_unlock (die->cu->abbrev_lock);
+	  return DWARF_END_ABBREV;
+	}
+
       get_uleb128 (code, addr, die->cu->endp);
       if (readp != NULL)
 	*readp = addr;
@@ -818,7 +831,11 @@  __libdw_dieabbrev (Dwarf_Die *die, const unsigned char **readp)
       if (die->abbrev == NULL)
 	die->abbrev = __libdw_findabbrev (die->cu, code);
     }
-  return die->abbrev;
+
+  Dwarf_Abbrev *result = die->abbrev;
+  rwlock_unlock (die->cu->abbrev_lock);
+
+  return result; 
 }
 
 /* Helper functions for form handling.  */