date:20231010

Re: [PATCH] Fix thread-safety for elfutils

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-08-08 at 12:07 -0500, Heather McIntyre via Elfutils-devel
wrote:
> This patch was created to address thread-safety issues reported in bug 26921
>  and bug 26930
> .
> Additionally, other thread-safety fixes were applied during the process.
> 
> Brief Description:
> Locking was implemented for tsearch and tfind.
> Changes to multiple files within the libelf library were made such that
> USE_LOCKS no longer causes tests to fail when enabled.
> New tests were added to confirm thread-safety.

Very nice. That is a lot of work. And I must admit that I cannot hold
that much work in my little head at the same time. So I have split up
your commit into (what I hope are) logical independent parts. That will
make it easier to review. I might have split it into too many parts,
but that at least makes it easier to just add those parts that are
trivially correct.

The only changes I made were:
1. Move the ChangeLog entries into the commit message
   (This is something we do now and makes cherry picking small
changes easier, but I see it isn't actually documented
in CONTRIBUTING, sorry. I'll try to update that.)
2. Fixed up some Copyright notices as discussed off-list.
3. Made some whitespace/indentation changes which made the
   diffs slightly smaller (in most cases).

I'll comment/review the individual commits. Which I'll post to the
list.

Heather McIntyre (16):
  lib: Add new once_define and once macros to eu-config.h
  libelf: Make elf_version thread-safe
  libelf: Fix deadlock in __libelf_readall
  libelf: Fix deadlock in elf_cntl
  libelf: Fix elf_end deadlock
  libelf: Make elf32_getchdr and elf64_getchdr thread-safe
  lib: Add eu_tsearch and eu_tfind
  libcpu: Change calls for tsearch/tfind to eu_tsearch/eu_tfind.
  src: Use eu-search in nm and findtextrel.
  libdw: make dwarf_getalt thread-safe
  libdw: Add locking around __libdw_dieabbrev for dwarf_hasattr
  libdw: Make libdw_find_split_unit thread-safe
  libdw: Make libdw_findcu thread-safe
  libdw,libdwfl: Use eu-search for thread-safety
  tests: Add eu-search tests
  configure: No longer mark --enable-thread-safety as EXPERIMENTAL

Which can also be found here:
https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=thread-safety

Cheers,

Mark

[PATCH 03/16] libelf: Fix deadlock in __libelf_readall

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libelf/elf_readall.c (__libelf_readall): Move rwlock_unlock
before libelf_acquire_all.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libelf/elf_readall.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libelf/elf_readall.c b/libelf/elf_readall.c
index d0f9a28c..2d62d447 100644
--- a/libelf/elf_readall.c
+++ b/libelf/elf_readall.c
@@ -84,6 +84,7 @@ __libelf_readall (Elf *elf)
 
   /* If this is an archive and we have derived descriptors get the
 locks for all of them.  */
+  rwlock_unlock(elf->lock); // lock will be reacquired next line
   libelf_acquire_all (elf);
 
   if (elf->maximum_size == ~((size_t) 0))
@@ -141,10 +142,8 @@ __libelf_readall (Elf *elf)
__libelf_seterrno (ELF_E_NOMEM);
 
   /* Free the locks on the children.  */
-  libelf_release_all (elf);
+  libelf_release_all (elf); // lock is released
 }
 
-  rwlock_unlock (elf->lock);
-
   return (char *) elf->map_address;
 }
-- 
2.41.0

[PATCH 02/16] libelf: Make elf_version thread-safe

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* elf_version.c (version_once): Define once.
(initialize_version): New static function.
(elf_version): Use initialize_version version_once.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libelf/elf_version.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/libelf/elf_version.c b/libelf/elf_version.c
index 6ec534ab..8296bb65 100644
--- a/libelf/elf_version.c
+++ b/libelf/elf_version.c
@@ -32,12 +32,21 @@
 #endif
 
 #include 
+#include 
 
+/* Multiple threads may initialize __libelf_version.
+   pthread_once() ensures that __libelf_version is initialized only once. */
+once_define(static, version_once);
 
 /* Currently selected version.  Should be EV_CURRENT.
Will be EV_NONE if elf_version () has not been called yet.  */
 unsigned int __libelf_version = EV_NONE;
 
+static void initialize_version(void)
+{
+  __libelf_version = EV_CURRENT;
+}
+
 unsigned int
 elf_version (unsigned int version)
 {
@@ -49,7 +58,7 @@ elf_version (unsigned int version)
   /* Phew, we know this version.  */
 
   /* Signal that the version is now initialized.  */
-  __libelf_version = EV_CURRENT;
+  once(version_once, initialize_version);
 
   /* And return the last (or initial) version.  */
   return EV_CURRENT;
-- 
2.41.0

[PATCH 05/16] libelf: Fix elf_end deadlock

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libelf/elf_end.c (elf_end): Add rwlock_unlock before
early return.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libelf/elf_end.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libelf/elf_end.c b/libelf/elf_end.c
index 89727cb3..80f4d13f 100644
--- a/libelf/elf_end.c
+++ b/libelf/elf_end.c
@@ -82,7 +82,10 @@ elf_end (Elf *elf)
   elf->state.ar.ar_sym = NULL;
 
   if (elf->state.ar.children != NULL)
-   return 0;
+   {
+ rwlock_unlock(elf->lock);
+ return 0;
+   }
 }
 
   /* Remove this structure from the children list.  */
-- 
2.41.0

[PATCH 04/16] libelf: Fix deadlock in elf_cntl

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

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

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libelf/elf_cntl.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/libelf/elf_cntl.c b/libelf/elf_cntl.c
index 04aa9132..64087c7d 100644
--- a/libelf/elf_cntl.c
+++ b/libelf/elf_cntl.c
@@ -48,13 +48,16 @@ elf_cntl (Elf *elf, Elf_Cmd cmd)
   return -1;
 }
 
-  rwlock_wrlock (elf->lock);
+
 
   switch (cmd)
 {
 case ELF_C_FDREAD:
+ rwlock_rdlock (elf->lock);
+ int addr_isnull = elf->map_address == NULL;
+ rwlock_unlock(elf->lock);
   /* If not all of the file is in the memory read it now.  */
-  if (elf->map_address == NULL && __libelf_readall (elf) == NULL)
+  if (addr_isnull && __libelf_readall (elf) == NULL)
{
  /* We were not able to read everything.  */
  result = -1;
@@ -64,7 +67,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 +78,5 @@ elf_cntl (Elf *elf, Elf_Cmd cmd)
   break;
 }
 
-  rwlock_unlock (elf->lock);
-
   return result;
 }
-- 
2.41.0

[PATCH 01/16] lib: Add new once_define and once macros to eu-config.h

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* lib/eu-config.h New macros.
[USE_LOCKS] (ONCE_CALL): (once_define, once)

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 lib/eu-config.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/lib/eu-config.h b/lib/eu-config.h
index 78a5c4fe..feb079db 100644
--- a/lib/eu-config.h
+++ b/lib/eu-config.h
@@ -33,13 +33,18 @@
 # include 
 # include 
 # define rwlock_define(class,name) class pthread_rwlock_t name
+# define once_define(class,name)  class pthread_once_t name = PTHREAD_ONCE_INIT
 # define RWLOCK_CALL(call) \
   ({ int _err = pthread_rwlock_ ## call; assert_perror (_err); })
+# define ONCE_CALL(call)  \
+  ({ int _err = pthread_ ## call; assert_perror (_err); })
 # define rwlock_init(lock) RWLOCK_CALL (init (&lock, NULL))
 # define rwlock_fini(lock) RWLOCK_CALL (destroy (&lock))
 # define rwlock_rdlock(lock)   RWLOCK_CALL (rdlock (&lock))
 # define rwlock_wrlock(lock)   RWLOCK_CALL (wrlock (&lock))
 # define rwlock_unlock(lock)   RWLOCK_CALL (unlock (&lock))
+# define once(once_control, init_routine)  \
+  ONCE_CALL (once (&once_control, init_routine))
 #else
 /* Eventually we will allow multi-threaded applications to use the
libraries.  Therefore we will add the necessary locking although
@@ -50,6 +55,8 @@
 # define rwlock_rdlock(lock) ((void) (lock))
 # define rwlock_wrlock(lock) ((void) (lock))
 # define rwlock_unlock(lock) ((void) (lock))
+# define once_define(class,name)
+# define once(once_control, init_routine)  init_routine()
 #endif /* USE_LOCKS */
 
 #include 
-- 
2.41.0

[PATCH 08/16] libcpu: Change calls for tsearch/tfind to eu_tsearch/eu_tfind.

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libcpu/Makefile.am: Add USE_LOCKS condition for -pthread.
* libcpu/i386_parse.y: Add eu-search.h and remove search.h.
Change calls for tsearch/tfind to eu_tsearch/eu_tfind.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libcpu/Makefile.am  |  3 +++
 libcpu/i386_parse.y | 48 ++---
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/libcpu/Makefile.am b/libcpu/Makefile.am
index 4ba1be56..a51334b5 100644
--- a/libcpu/Makefile.am
+++ b/libcpu/Makefile.am
@@ -33,6 +33,9 @@ AM_CPPFLAGS += -I$(srcdir)/../libelf -I$(srcdir)/../libebl \
 if BUILD_STATIC
 AM_CFLAGS += $(fpic_CFLAGS)
 endif
+if USE_LOCKS
+  AM_CFLAGS += -pthread
+endif
 AM_CFLAGS += -fdollars-in-identifiers
 LEXCOMPILE = $(LEX) $(LFLAGS) $(AM_LFLAGS) -P$(
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -269,12 +269,12 @@ mask:   kMASK kBITFIELD kNUMBER
  struct synonym *newp = xmalloc (sizeof (*newp));
  newp->from = $2;
  newp->to = $3;
- if (tfind (newp, &synonyms, compare_syn) != NULL)
+ if (eu_tfind (newp, &synonyms, compare_syn) != NULL)
error (0, 0,
   "%d: duplicate definition for synonym '%s'",
   i386_lineno, $2);
- else if (tsearch ( newp, &synonyms, compare_syn) == NULL)
-   error (EXIT_FAILURE, 0, "tsearch");
+ else if (eu_tsearch ( newp, &synonyms, compare_syn) == 
NULL)
+   error (EXIT_FAILURE, 0, "eu_tsearch");
}
|
;
@@ -308,12 +308,12 @@ instr:  bytes ':' bitfieldopt kID bitfieldopt 
optargs
  newp->bytes = $1;
  newp->mnemonic = $4;
  if (newp->mnemonic != (void *) -1l
- && tfind ($4, &mnemonics,
+ && eu_tfind ($4, &mnemonics,
(int (*)(const void *, const void *)) 
strcmp) == NULL)
{
- if (tsearch ($4, &mnemonics,
+ if (eu_tsearch ($4, &mnemonics,
   (int (*)(const void *, const void 
*)) strcmp) == NULL)
-   error (EXIT_FAILURE, errno, "tsearch");
+   error (EXIT_FAILURE, errno, "eu_tsearch");
  ++nmnemonics;
}
 
@@ -339,15 +339,15 @@ instr:  bytes ':' bitfieldopt kID bitfieldopt 
optargs
   infname, i386_lineno - 1, $5->name);
 
  struct suffix search = { .name = $5->name };
- if (tfind (&search, &suffixes, compare_suf)
+ if (eu_tfind (&search, &suffixes, compare_suf)
  == NULL)
{
  struct suffix *ns = xmalloc (sizeof (*ns));
  ns->name = $5->name;
  ns->idx = ++nsuffixes;
- if (tsearch (ns, &suffixes, compare_suf)
+ if (eu_tsearch (ns, &suffixes, compare_suf)
  == NULL)
-   error (EXIT_FAILURE, errno, "tsearch");
+   error (EXIT_FAILURE, errno, "eu_tsearch");
}
}
 
@@ -374,7 +374,7 @@ bitfieldopt:  kBITFIELD
  struct known_bitfield search;
  search.name = $1;
  struct known_bitfield **res;
- res = tfind (&search, &bitfields, bitfield_compare);
+ res = eu_tfind (&search, &bitfields, bitfield_compare);
  if (res == NULL)
{
  error (0, 0, "%d: unknown bitfield '%s'",
@@ -437,7 +437,7 @@ bit:  '0'
  struct known_bitfield search;
  search.name = $1;
  struct known_bitfield **res;
- res = tfind (&search, &bitfields, bitfield_compare);
+ res = eu_tfind (&search, &bitfields, bitfield_compare);
  if (res == NULL)
{
  error (0, 0, "%d: unknown bitfield '%s'",
@@ -497,7 +497,7 @@ argcomp:  kBITFIELD
  struct known_bitfield search;
  search.name = $1;
  struct known_bitfield **res;
- res = tfind (&search, &bitfields,

[PATCH 06/16] libelf: Make elf32_getchdr and elf64_getchdr thread-safe

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libelf/elf32_getchdr.c: Move getchdr function to
elf32_getchdr.h.
* libelf/elf32_getchdr.h: New file.
Add macro to create getchdr_wrlock.
* libelf/elf32_updatenull.c: Change call from getchdr to
getchdr_wrlock.
* libelf/elf_getdata.c: Add elf_getdata_wrlock.
* libelf/libelfP.h: Add internal function declarations.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libelf/elf32_getchdr.c| 46 +++--
 libelf/elf32_getchdr.h| 61 +++
 libelf/elf32_updatenull.c |  2 +-
 libelf/elf_getdata.c  | 14 +
 libelf/libelfP.h  |  4 +++
 5 files changed, 84 insertions(+), 43 deletions(-)
 create mode 100644 libelf/elf32_getchdr.h

diff --git a/libelf/elf32_getchdr.c b/libelf/elf32_getchdr.c
index 982a614c..41591300 100644
--- a/libelf/elf32_getchdr.c
+++ b/libelf/elf32_getchdr.c
@@ -38,46 +38,8 @@
 # define LIBELFBITS 32
 #endif
 
+#define ELF_WRLOCK_HELD 1
+#include "elf32_getchdr.h"
 
-ElfW2(LIBELFBITS,Chdr) *
-elfw2(LIBELFBITS,getchdr) (Elf_Scn *scn)
-{
-  ElfW2(LIBELFBITS,Shdr) *shdr = elfw2(LIBELFBITS,getshdr) (scn);
-  if (shdr == NULL)
-return NULL;
-
-  /* Must have SHF_COMPRESSED flag set.  Allocated or no bits sections
- can never be compressed.  */
-  if ((shdr->sh_flags & SHF_ALLOC) != 0)
-{
-  __libelf_seterrno (ELF_E_INVALID_SECTION_FLAGS);
-  return NULL;
-}
-
-  if (shdr->sh_type == SHT_NULL
-  || shdr->sh_type == SHT_NOBITS)
-{
-  __libelf_seterrno (ELF_E_INVALID_SECTION_TYPE);
-  return NULL;
-}
-
-  if ((shdr->sh_flags & SHF_COMPRESSED) == 0)
-{
-  __libelf_seterrno (ELF_E_NOT_COMPRESSED);
-  return NULL;
-}
-
-  /* This makes sure the data is in the correct format, so we don't
- need to swap fields. */
-  Elf_Data *d  = elf_getdata (scn, NULL);
-  if (d == NULL)
-return NULL;
-
-  if (d->d_size < sizeof (ElfW2(LIBELFBITS,Chdr)) || d->d_buf == NULL)
-{
-  __libelf_seterrno (ELF_E_INVALID_DATA);
-  return NULL;
-}
-
-  return (ElfW2(LIBELFBITS,Chdr) *) d->d_buf;
-}
+#define ELF_WRLOCK_HELD 0
+#include "elf32_getchdr.h"
\ No newline at end of file
diff --git a/libelf/elf32_getchdr.h b/libelf/elf32_getchdr.h
new file mode 100644
index ..04d47e7a
--- /dev/null
+++ b/libelf/elf32_getchdr.h
@@ -0,0 +1,61 @@
+#undef ADD_ROUTINE_PREFIX
+#undef ADD_ROUTINE_SUFFIX
+
+#if ELF_WRLOCK_HELD
+#define CONCAT(x,y) x##y
+#define ADD_ROUTINE_PREFIX(y) CONCAT(__,y)
+#define ADD_ROUTINE_SUFFIX(x) x ## _wrlock
+#define INTERNAL internal_function
+#else
+#define ADD_ROUTINE_PREFIX(y) y
+#define ADD_ROUTINE_SUFFIX(x) x
+#define INTERNAL
+#endif
+
+ElfW2(LIBELFBITS,Chdr) *
+INTERNAL
+ADD_ROUTINE_PREFIX(elfw2(LIBELFBITS, ADD_ROUTINE_SUFFIX(getchdr))) (Elf_Scn 
*scn)
+{
+
+  ElfW2(LIBELFBITS,Shdr) *shdr = ADD_ROUTINE_PREFIX(elfw2(LIBELFBITS, 
ADD_ROUTINE_SUFFIX(getshdr)))(scn);
+
+  if (shdr == NULL)
+return NULL;
+
+  /* Must have SHF_COMPRESSED flag set.  Allocated or no bits sections
+ can never be compressed.  */
+  if ((shdr->sh_flags & SHF_ALLOC) != 0)
+{
+  __libelf_seterrno (ELF_E_INVALID_SECTION_FLAGS);
+  return NULL;
+}
+
+  if (shdr->sh_type == SHT_NULL
+  || shdr->sh_type == SHT_NOBITS)
+{
+  __libelf_seterrno (ELF_E_INVALID_SECTION_TYPE);
+  return NULL;
+}
+
+  if ((shdr->sh_flags & SHF_COMPRESSED) == 0)
+{
+  __libelf_seterrno (ELF_E_NOT_COMPRESSED);
+  return NULL;
+}
+
+  /* This makes sure the data is in the correct format, so we don't
+ need to swap fields. */
+  Elf_Data *d  = ADD_ROUTINE_PREFIX(ADD_ROUTINE_SUFFIX(elf_getdata)) (scn, 
NULL);
+  if (d == NULL)
+return NULL;
+
+  if (d->d_size < sizeof (ElfW2(LIBELFBITS,Chdr)) || d->d_buf == NULL)
+{
+  __libelf_seterrno (ELF_E_INVALID_DATA);
+  return NULL;
+}
+
+  return (ElfW2(LIBELFBITS,Chdr) *) d->d_buf;
+}
+#undef INTERNAL
+#undef ELF_WRLOCK_HELD
\ No newline at end of file
diff --git a/libelf/elf32_updatenull.c b/libelf/elf32_updatenull.c
index c5d26b00..3594e8ba 100644
--- a/libelf/elf32_updatenull.c
+++ b/libelf/elf32_updatenull.c
@@ -407,7 +407,7 @@ __elfw2(LIBELFBITS,updatenull_wrlock) (Elf *elf, int 
*change_bop, size_t shnum)
  else
{
  ElfW2(LIBELFBITS,Chdr) *chdr;
- chdr = elfw2(LIBELFBITS,getchdr) (scn);
+ chdr = __elfw2(LIBELFBITS,getchdr_wrlock) (scn);
  if (unlikely (chdr == NULL))
return -1;
  sh_size = chdr->ch_size;
diff --git a/libelf/elf_getdata.c b/libelf/elf_getdata.c
index 5ebd270f..7c3ac043 100644
--- a/libelf/elf_getdata.c
+++ b/libelf/elf_getdata.c
@@ -582,4 +582,18 @@ elf_getdata (Elf_Scn *scn, Elf_Data *data)
 
   return result;
 }
+
+Elf_Data *
+internal_function
+__elf_getdata_wrlock (Elf_Scn *scn, Elf_Da

[PATCH 07/16] lib: Add eu_tsearch and eu_tfind

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* lib/eu-search.h: New file.
Declarations for read/write locked eu_tsearch/eu_tfind.
* lib/eu-search.c: New file.
Definitions for read/write locked eu_tsearch/eu_tfind.
* Makefile.am (libeu_a_SOURCES): Add eu-search.c.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 lib/Makefile.am |  2 +-
 lib/eu-search.c | 60 +
 lib/eu-search.h | 39 
 3 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 lib/eu-search.c
 create mode 100644 lib/eu-search.h

diff --git a/lib/Makefile.am b/lib/Makefile.am
index b3bb929f..ce8f3e1b 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -34,7 +34,7 @@ AM_CPPFLAGS += -I$(srcdir)/../libelf
 noinst_LIBRARIES = libeu.a
 
 libeu_a_SOURCES = xasprintf.c xstrdup.c xstrndup.c xmalloc.c next_prime.c \
- crc32.c crc32_file.c \
+ crc32.c crc32_file.c eu-search.c \
  color.c error.c printversion.c
 
 noinst_HEADERS = fixedsizehash.h libeu.h system.h dynamicsizehash.h list.h \
diff --git a/lib/eu-search.c b/lib/eu-search.c
new file mode 100644
index ..a6b04f4f
--- /dev/null
+++ b/lib/eu-search.c
@@ -0,0 +1,60 @@
+/* Definitions for thread-safe tsearch/tfind
+   Copyright (C) 2023 Rice University
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of either
+
+ * the GNU Lesser General Public License as published by the Free
+   Software Foundation; either version 3 of the License, or (at
+   your option) any later version
+
+   or
+
+ * the GNU General Public License as published by the Free
+   Software Foundation; either version 2 of the License, or (at
+   your option) any later version
+
+   or both in parallel, as here.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received copies of the GNU General Public License and
+   the GNU Lesser General Public License along with this program.  If
+   not, see .  */
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#include 
+#include "eu-search.h"
+
+rwlock_define(static, search_find_lock);
+
+void *eu_tsearch(const void *key, void **rootp,
+int (*compar)(const void *, const void *))
+{
+  void *ret = NULL;
+  rwlock_wrlock(search_find_lock);
+
+  ret = tsearch(key, rootp, compar);
+
+  rwlock_unlock(search_find_lock);
+  return ret;
+}
+
+void *eu_tfind(const void *key, void *const *rootp,
+  int (*compar)(const void *, const void *))
+{
+  void *ret = NULL;
+  rwlock_rdlock(search_find_lock);
+
+  ret = tfind(key, rootp, compar);
+
+  rwlock_unlock(search_find_lock);
+  return ret;
+}
diff --git a/lib/eu-search.h b/lib/eu-search.h
new file mode 100644
index ..4ce0139a
--- /dev/null
+++ b/lib/eu-search.h
@@ -0,0 +1,39 @@
+/* Calls for thread-safe tsearch/tfind
+   Copyright (C) 2023 Rice University
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of either
+
+ * the GNU Lesser General Public License as published by the Free
+   Software Foundation; either version 3 of the License, or (at
+   your option) any later version
+
+   or
+
+ * the GNU General Public License as published by the Free
+   Software Foundation; either version 2 of the License, or (at
+   your option) any later version
+
+   or both in parallel, as here.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received copies of the GNU General Public License and
+   the GNU Lesser General Public License along with this program.  If
+   not, see .  */
+
+#ifndef EU_SEARCH_H
+#define EU_SEARCH_H 1
+
+#include 
+
+extern void *eu_tsearch(const void *key, void **rootp,
+   int (*compar)(const void *, const void *));
+extern void *eu_tfind(const void *key, void *const *rootp,
+ int (*compar)(const void *, const void *));
+
+#endif
-- 
2.41.0

[PATCH 11/16] libdw: Add locking around __libdw_dieabbrev for dwarf_hasattr

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libdw/dwarf_hasattr.c (dwarf_hasattr): Use die_abbrev_lock
around __libdw_dieabbrev call.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libdw/dwarf_hasattr.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/libdw/dwarf_hasattr.c b/libdw/dwarf_hasattr.c
index eca08394..92f8de68 100644
--- a/libdw/dwarf_hasattr.c
+++ b/libdw/dwarf_hasattr.c
@@ -34,6 +34,10 @@
 #include 
 #include "libdwP.h"
 
+/* dwarf_hasattr() calls __libdw_dieabbrev() in libdwP.h.
+   __libdw_dieabbrev() reads/writes "die->abbrev".
+   Mutual exclusion is enforced around the call to __libdw_dieabbrev to 
prevent a race. */
+rwlock_define(static, die_abbrev_lock);
 
 int
 dwarf_hasattr (Dwarf_Die *die, unsigned int search_name)
@@ -41,8 +45,13 @@ dwarf_hasattr (Dwarf_Die *die, unsigned int search_name)
   if (die == NULL)
 return 0;
 
+  rwlock_wrlock(die_abbrev_lock);
+
   /* Find the abbreviation entry.  */
   Dwarf_Abbrev *abbrevp = __libdw_dieabbrev (die, NULL);
+
+  rwlock_unlock(die_abbrev_lock);
+
   if (unlikely (abbrevp == DWARF_END_ABBREV))
 {
   __libdw_seterrno (DWARF_E_INVALID_DWARF);
-- 
2.41.0

[PATCH 12/16] libdw: Make libdw_find_split_unit thread-safe

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* (try_split_file): Use eu_tsearch.
Add lock for cu->split.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libdw/libdw_find_split_unit.c | 43 +--
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/libdw/libdw_find_split_unit.c b/libdw/libdw_find_split_unit.c
index 533f8072..a288e9bd 100644
--- a/libdw/libdw_find_split_unit.c
+++ b/libdw/libdw_find_split_unit.c
@@ -34,13 +34,19 @@
 #include "libelfP.h"
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
 
+/* __libdw_link_skel_split() modifies "skel->split = split"
+   and "split->split = skel".
+   "cu->split" is read at multiple locations and conditionally updated.
+   Mutual exclusion is enforced to prevent a race. */
+rwlock_define(static, cu_split_lock);
+
 static void
 try_split_file (Dwarf_CU *cu, const char *dwo_path)
 {
@@ -57,7 +63,7 @@ try_split_file (Dwarf_CU *cu, const char *dwo_path)
  if (split->unit_type == DW_UT_split_compile
  && cu->unit_id8 == split->unit_id8)
{
- if (tsearch (split->dbg, &cu->dbg->split_tree,
+ if (eu_tsearch (split->dbg, &cu->dbg->split_tree,
   __libdw_finddbg_cb) == NULL)
{
  /* Something went wrong.  Don't link.  */
@@ -65,9 +71,13 @@ try_split_file (Dwarf_CU *cu, const char *dwo_path)
  break;
}
 
+ rwlock_wrlock(cu_split_lock);
+
  /* Link skeleton and split compile units.  */
  __libdw_link_skel_split (cu, split);
 
+ rwlock_unlock(cu_split_lock);
+
  /* We have everything we need from this ELF
 file.  And we are going to close the fd to
 not run out of file descriptors.  */
@@ -75,8 +85,13 @@ try_split_file (Dwarf_CU *cu, const char *dwo_path)
  break;
}
}
+
+ rwlock_rdlock(cu_split_lock);
+
  if (cu->split == (Dwarf_CU *) -1)
dwarf_end (split_dwarf);
+
+ rwlock_unlock(cu_split_lock);
}
   /* Always close, because we don't want to run out of file
 descriptors.  See also the elf_fcntl ELF_C_FDDONE call
@@ -89,9 +104,13 @@ Dwarf_CU *
 internal_function
 __libdw_find_split_unit (Dwarf_CU *cu)
 {
+  rwlock_rdlock(cu_split_lock);
+  Dwarf_CU *cu_split_local = cu->split;
+  rwlock_unlock(cu_split_lock);
+
   /* Only try once.  */
-  if (cu->split != (Dwarf_CU *) -1)
-return cu->split;
+  if (cu_split_local != (Dwarf_CU *) -1)
+return cu_split_local;
 
   /* We need a skeleton unit with a comp_dir and [GNU_]dwo_name attributes.
  The split unit will be the first in the dwo file and should have the
@@ -116,7 +135,11 @@ __libdw_find_split_unit (Dwarf_CU *cu)
  free (dwo_path);
}
 
- if (cu->split == (Dwarf_CU *) -1)
+ rwlock_rdlock(cu_split_lock);
+ cu_split_local = cu->split;
+ rwlock_unlock(cu_split_lock);
+
+ if (cu_split_local == (Dwarf_CU *) -1)
{
  /* Try compdir plus dwo_name.  */
  Dwarf_Attribute compdir;
@@ -138,9 +161,15 @@ __libdw_find_split_unit (Dwarf_CU *cu)
}
 }
 
+  rwlock_wrlock(cu_split_lock);
+
   /* If we found nothing, make sure we don't try again.  */
   if (cu->split == (Dwarf_CU *) -1)
-cu->split = NULL;
+{
+  cu->split = NULL;
+  cu_split_local = cu->split;
+}
 
-  return cu->split;
+  rwlock_unlock(cu_split_lock);
+  return cu_split_local;
 }
-- 
2.41.0

[PATCH 10/16] libdw: make dwarf_getalt thread-safe

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libdw/dwarf_getalt.c: Add lock for
dbg->alt_dwarf/main->alt_dwarf.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libdw/dwarf_getalt.c | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c
index 0a12dfae..e3894c8c 100644
--- a/libdw/dwarf_getalt.c
+++ b/libdw/dwarf_getalt.c
@@ -44,6 +44,10 @@
 #include 
 #include 
 
+/* find_debug_altlink() modifies "dbg->alt_dwarf".
+   dwarf_getalt() reads "main->alt_dwarf".
+   Mutual exclusion is enforced to prevent a race. */
+rwlock_define(static, alt_dwarf_lock);
 
 char *
 internal_function
@@ -152,7 +156,9 @@ find_debug_altlink (Dwarf *dbg)
   Dwarf *alt = dwarf_begin (fd, O_RDONLY);
   if (alt != NULL)
{
+ rwlock_wrlock(alt_dwarf_lock);
  dbg->alt_dwarf = alt;
+ rwlock_unlock(alt_dwarf_lock);
  dbg->alt_fd = fd;
}
   else
@@ -163,22 +169,33 @@ find_debug_altlink (Dwarf *dbg)
 Dwarf *
 dwarf_getalt (Dwarf *main)
 {
+  rwlock_rdlock(alt_dwarf_lock);
+  Dwarf *alt_dwarf_local = main->alt_dwarf;
+  rwlock_unlock(alt_dwarf_lock);
+
   /* Only try once.  */
-  if (main == NULL || main->alt_dwarf == (void *) -1)
+  if (main == NULL || alt_dwarf_local == (void *) -1)
 return NULL;
 
-  if (main->alt_dwarf != NULL)
-return main->alt_dwarf;
+  if (alt_dwarf_local != NULL)
+return alt_dwarf_local;
 
   find_debug_altlink (main);
 
+  rwlock_rdlock(alt_dwarf_lock);
+  alt_dwarf_local = main->alt_dwarf;
+  rwlock_unlock(alt_dwarf_lock);
+
   /* If we found nothing, make sure we don't try again.  */
-  if (main->alt_dwarf == NULL)
+  if (alt_dwarf_local == NULL)
 {
+  rwlock_wrlock(alt_dwarf_lock);
   main->alt_dwarf = (void *) -1;
+  rwlock_unlock(alt_dwarf_lock);
+
   return NULL;
 }
 
-  return main->alt_dwarf;
+  return alt_dwarf_local;
 }
 INTDEF (dwarf_getalt)
-- 
2.41.0

[PATCH 16/16] configure: No longer mark --enable-thread-safety as EXPERIMENTAL

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* configure.ac (--enable-thread-safety): Remove experimental
warning.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 configure.ac | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index 29ed32fe..be2103e2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -79,12 +79,10 @@ AC_DEFINE_UNQUOTED(DEFAULT_AR_DETERMINISTIC, 
$default_ar_deterministic,
 
 AC_ARG_ENABLE([thread-safety],
 AS_HELP_STRING([--enable-thread-safety],
-   [enable thread safety of libraries EXPERIMENTAL]),
+   [enable thread safety of libraries]),
use_locks=$enableval, use_locks=no)
 AM_CONDITIONAL(USE_LOCKS, test "$use_locks" = yes)
 AS_IF([test "$use_locks" = yes], [AC_DEFINE(USE_LOCKS)])
-AS_IF([test "$use_locks" = yes],
-  [AC_MSG_WARN([thread-safety is EXPERIMENTAL tests might fail.])])
 
 AH_TEMPLATE([USE_LOCKS], [Defined if libraries should be thread-safe.])
 
@@ -907,10 +905,10 @@ AC_MSG_NOTICE([
 Symbol versioning  : ${enable_symbol_versioning}
 
   NOT RECOMMENDED FEATURES (should all be no)
-Experimental thread safety : ${use_locks}
 install elf.h  : ${install_elfh}
 
   OTHER FEATURES
+Enable thread safety   : ${use_locks}
 Deterministic archives by default  : ${default_ar_deterministic}
 Native language support: ${USE_NLS}
 Extra Valgrind annotations : ${use_vg_annotations}
-- 
2.41.0

[PATCH 13/16] libdw: Make libdw_findcu thread-safe

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libdw/libdw_findcu.c (findcu_cb): Use eu_tsearch.
(__libdw_findcu): Use eu_tfind and next_tucu_offset_lock.
(__libdw_findcu_addr): Use eu_tfind.
(__libdw_find_split_dbg_addr): Likewise.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libdw/libdw_findcu.c | 54 
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
index ed744231..e546fb0f 100644
--- a/libdw/libdw_findcu.c
+++ b/libdw/libdw_findcu.c
@@ -32,9 +32,13 @@
 #endif
 
 #include 
-#include 
+#include 
 #include "libdwP.h"
 
+/* __libdw_findcu modifies "&dbg->next_tu_offset : &dbg->next_cu_offset".
+   May read or write, so mutual exclusion is enforced to prevent a race. */
+rwlock_define(static, next_tucu_offset_lock);
+
 static int
 findcu_cb (const void *arg1, const void *arg2)
 {
@@ -213,7 +217,7 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
 Dwarf_Sig8_Hash_insert (&dbg->sig8_hash, unit_id8, newp);
 
   /* Add the new entry to the search tree.  */
-  if (tsearch (newp, tree, findcu_cb) == NULL)
+  if (eu_tsearch (newp, tree, findcu_cb) == NULL)
 {
   /* Something went wrong.  Undo the operation.  */
   *offsetp = oldoff;
@@ -234,28 +238,40 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool 
v4_debug_types)
 
   /* Maybe we already know that CU.  */
   struct Dwarf_CU fake = { .start = start, .end = 0 };
-  struct Dwarf_CU **found = tfind (&fake, tree, findcu_cb);
+  struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
+  struct Dwarf_CU *result = NULL;
+
   if (found != NULL)
 return *found;
 
-  if (start < *next_offset)
-{
-  __libdw_seterrno (DWARF_E_INVALID_DWARF);
-  return NULL;
-}
+  rwlock_wrlock(next_tucu_offset_lock);
 
-  /* No.  Then read more CUs.  */
-  while (1)
+  if (start < *next_offset)
+__libdw_seterrno (DWARF_E_INVALID_DWARF);
+  else
 {
-  struct Dwarf_CU *newp = __libdw_intern_next_unit (dbg, v4_debug_types);
-  if (newp == NULL)
-   return NULL;
+  /* No.  Then read more CUs.  */
+  while (1)
+   {
+ struct Dwarf_CU *newp = __libdw_intern_next_unit (dbg,
+   v4_debug_types);
+ if (newp == NULL)
+   {
+ result = NULL;
+ break;
+   }
 
-  /* Is this the one we are looking for?  */
-  if (start < *next_offset || start == newp->start)
-   return newp;
+ /* Is this the one we are looking for?  */
+ if (start < *next_offset || start == newp->start)
+   {
+ result = newp;
+ break;
+   }
+   }
 }
-  /* NOTREACHED */
+
+  rwlock_unlock(next_tucu_offset_lock);
+  return result;
 }
 
 struct Dwarf_CU *
@@ -283,7 +299,7 @@ __libdw_findcu_addr (Dwarf *dbg, void *addr)
 return NULL;
 
   struct Dwarf_CU fake = { .start = start, .end = 0 };
-  struct Dwarf_CU **found = tfind (&fake, tree, findcu_cb);
+  struct Dwarf_CU **found = eu_tfind (&fake, tree, findcu_cb);
 
   if (found != NULL)
 return *found;
@@ -298,7 +314,7 @@ __libdw_find_split_dbg_addr (Dwarf *dbg, void *addr)
   /* XXX Assumes split DWARF only has CUs in main IDX_debug_info.  */
   Elf_Data fake_data = { .d_buf = addr, .d_size = 0 };
   Dwarf fake = { .sectiondata[IDX_debug_info] = &fake_data };
-  Dwarf **found = tfind (&fake, &dbg->split_tree, __libdw_finddbg_cb);
+  Dwarf **found = eu_tfind (&fake, &dbg->split_tree, __libdw_finddbg_cb);
 
   if (found != NULL)
 return *found;
-- 
2.41.0

[PATCH 09/16] src: Use eu-search in nm and findtextrel.

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* src/Makefile.am: Add USE_LOCKS condition for -pthread.
* src/findtextrel.c: Add eu-search.h and remove search.h.
Change calls of tsearch/tfind to eu_tsearch/eu_tfind.
* src/nm.c: Likewise.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 src/Makefile.am   |  3 +++
 src/findtextrel.c | 10 +-
 src/nm.c  | 10 +-
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 10d59a48..fea5d43e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -22,6 +22,9 @@ DEFS += $(YYDEBUG) -DDEBUGPRED=@DEBUGPRED@ \
 AM_CPPFLAGS += -I$(srcdir)/../libelf -I$(srcdir)/../libebl \
-I$(srcdir)/../libdw -I$(srcdir)/../libdwelf \
-I$(srcdir)/../libdwfl -I$(srcdir)/../libasm
+if USE_LOCKS
+  AM_CFLAGS += -pthread
+endif
 
 AM_LDFLAGS = -Wl,-rpath-link,../libelf:../libdw $(STACK_USAGE_NO_ERROR)
 
diff --git a/src/findtextrel.c b/src/findtextrel.c
index d3021a3a..5ac4c29a 100644
--- a/src/findtextrel.c
+++ b/src/findtextrel.c
@@ -27,7 +27,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -501,10 +501,10 @@ check_rel (size_t nsegments, struct segments 
segments[nsegments],
/* There can be more than one relocation against one file.
   Try to avoid multiple messages.  And yes, the code uses
   pointer comparison.  */
-   if (tfind (src, knownsrcs, ptrcompare) == NULL)
+   if (eu_tfind (src, knownsrcs, ptrcompare) == NULL)
  {
printf (_("%s not compiled with -fpic/-fPIC\n"), src);
-   tsearch (src, knownsrcs, ptrcompare);
+   eu_tsearch (src, knownsrcs, ptrcompare);
  }
return;
  }
@@ -555,12 +555,12 @@ check_rel (size_t nsegments, struct segments 
segments[nsegments],
if (sym->st_value + sym->st_size > addr)
  {
/* It is this function.  */
-   if (tfind (lowstr, knownsrcs, ptrcompare) == NULL)
+   if (eu_tfind (lowstr, knownsrcs, ptrcompare) == NULL)
  {
printf (_("\
 the file containing the function '%s' is not compiled with -fpic/-fPIC\n"),
lowstr);
-   tsearch (lowstr, knownsrcs, ptrcompare);
+   eu_tsearch (lowstr, knownsrcs, ptrcompare);
  }
  }
else if (highidx == -1)
diff --git a/src/nm.c b/src/nm.c
index fbdee8e1..44c20fb2 100644
--- a/src/nm.c
+++ b/src/nm.c
@@ -32,7 +32,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -537,7 +537,7 @@ static int
 get_global (Dwarf *dbg __attribute__ ((unused)), Dwarf_Global *global,
void *arg __attribute__ ((unused)))
 {
-  tsearch (memcpy (xmalloc (sizeof (Dwarf_Global)), global,
+  eu_tsearch (memcpy (xmalloc (sizeof (Dwarf_Global)), global,
   sizeof (Dwarf_Global)),
   &global_root, global_compare);
 
@@ -696,7 +696,7 @@ get_local_names (Dwarf *dbg)
   /* Check whether a similar local_name is already in the
  cache.  That should not happen.  But if it does, we
  don't want to leak memory.  */
-   struct local_name **tres = tsearch (newp, &local_root,
+   struct local_name **tres = eu_tsearch (newp, &local_root,
local_compare);
if (tres == NULL)
   error_exit (errno, _("cannot create search tree"));
@@ -1387,7 +1387,7 @@ show_symbols (int fd, Ebl *ebl, GElf_Ehdr *ehdr,
  && global_root != NULL)
{
  Dwarf_Global fake = { .name = symstr };
- Dwarf_Global **found = tfind (&fake, &global_root,
+ Dwarf_Global **found = eu_tfind (&fake, &global_root,
global_compare);
  if (found != NULL)
{
@@ -1442,7 +1442,7 @@ show_symbols (int fd, Ebl *ebl, GElf_Ehdr *ehdr,
  .lowpc = sym->st_value,
  .highpc = sym->st_value,
};
- struct local_name **found = tfind (&fake, &local_root,
+ struct local_name **found = eu_tfind (&fake, &local_root,
 local_compare);
  if (found != NULL)
{
-- 
2.41.0

[PATCH 14/16] libdw,libdwfl: Use eu-search for thread-safety

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* libdw/cie.c: Add eu-search.h and remove search.h.
Change calls of tsearch/tfind to eu_tsearch/eu_tfind.
* libdw/dwarf_getlocation.c: Likewise.
* libdw/dwarf_getmacros.c: Likewise.
* libdw/dwarf_getsrclines.c: Likewise.
* libdw/fde.c: Likewise.
* libdwfl/cu.c: Likewise.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 libdw/cie.c   |  8 
 libdw/dwarf_getlocation.c | 16 
 libdw/dwarf_getmacros.c   |  6 +++---
 libdw/dwarf_getsrclines.c |  6 +++---
 libdw/fde.c   |  6 +++---
 libdwfl/cu.c  |  4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/libdw/cie.c b/libdw/cie.c
index 1b0aae7c..758daef5 100644
--- a/libdw/cie.c
+++ b/libdw/cie.c
@@ -33,7 +33,7 @@
 #include "cfi.h"
 #include "encoded-value.h"
 #include 
-#include 
+#include 
 #include 
 
 
@@ -144,7 +144,7 @@ intern_new_cie (Dwarf_CFI *cache, Dwarf_Off offset, const 
Dwarf_CIE *info)
   cie->initial_state = NULL;
 
   /* Add the new entry to the search tree.  */
-  if (tsearch (cie, &cache->cie_tree, &compare_cie) == NULL)
+  if (eu_tsearch (cie, &cache->cie_tree, &compare_cie) == NULL)
 {
   free (cie);
   __libdw_seterrno (DWARF_E_NOMEM);
@@ -160,7 +160,7 @@ internal_function
 __libdw_find_cie (Dwarf_CFI *cache, Dwarf_Off offset)
 {
   const struct dwarf_cie cie_key = { .offset = offset };
-  struct dwarf_cie **found = tfind (&cie_key, &cache->cie_tree, &compare_cie);
+  struct dwarf_cie **found = eu_tfind (&cie_key, &cache->cie_tree, 
&compare_cie);
   if (found != NULL)
 return *found;
 
@@ -189,7 +189,7 @@ internal_function
 __libdw_intern_cie (Dwarf_CFI *cache, Dwarf_Off offset, const Dwarf_CIE *info)
 {
   const struct dwarf_cie cie_key = { .offset = offset };
-  struct dwarf_cie **found = tfind (&cie_key, &cache->cie_tree, &compare_cie);
+  struct dwarf_cie **found = eu_tfind (&cie_key, &cache->cie_tree, 
&compare_cie);
   if (found == NULL)
 /* We have not read this CIE yet.  Enter it.  */
 (void) intern_new_cie (cache, offset, info);
diff --git a/libdw/dwarf_getlocation.c b/libdw/dwarf_getlocation.c
index 553fdc98..e26afe37 100644
--- a/libdw/dwarf_getlocation.c
+++ b/libdw/dwarf_getlocation.c
@@ -31,7 +31,7 @@
 #endif
 
 #include 
-#include 
+#include 
 #include 
 #include 
 
@@ -137,7 +137,7 @@ loc_compare (const void *p1, const void *p2)
 
 /* For each DW_OP_implicit_value, we store a special entry in the cache.
This points us directly to the block data for later fetching.
-   Returns zero on success, -1 on bad DWARF or 1 if tsearch failed.  */
+   Returns zero on success, -1 on bad DWARF or 1 if eu_tsearch failed.  */
 static int
 store_implicit_value (Dwarf *dbg, void **cache, Dwarf_Op *op)
 {
@@ -154,7 +154,7 @@ store_implicit_value (Dwarf *dbg, void **cache, Dwarf_Op 
*op)
   block->addr = op;
   block->data = (unsigned char *) data;
   block->length = op->number;
-  if (unlikely (tsearch (block, cache, loc_compare) == NULL))
+  if (unlikely (eu_tsearch (block, cache, loc_compare) == NULL))
 return 1;
   return 0;
 }
@@ -167,7 +167,7 @@ dwarf_getlocation_implicit_value (Dwarf_Attribute *attr, 
const Dwarf_Op *op,
 return -1;
 
   struct loc_block_s fake = { .addr = (void *) op };
-  struct loc_block_s **found = tfind (&fake, &attr->cu->locs, loc_compare);
+  struct loc_block_s **found = eu_tfind (&fake, &attr->cu->locs, loc_compare);
   if (unlikely (found == NULL))
 {
   __libdw_seterrno (DWARF_E_NO_BLOCK);
@@ -211,7 +211,7 @@ is_constant_offset (Dwarf_Attribute *attr,
 
   /* Check whether we already cached this location.  */
   struct loc_s fake = { .addr = attr->valp };
-  struct loc_s **found = tfind (&fake, &attr->cu->locs, loc_compare);
+  struct loc_s **found = eu_tfind (&fake, &attr->cu->locs, loc_compare);
 
   if (found == NULL)
 {
@@ -235,7 +235,7 @@ is_constant_offset (Dwarf_Attribute *attr,
   newp->loc = result;
   newp->nloc = 1;
 
-  found = tsearch (newp, &attr->cu->locs, loc_compare);
+  found = eu_tsearch (newp, &attr->cu->locs, loc_compare);
 }
 
   assert ((*found)->nloc == 1);
@@ -266,7 +266,7 @@ __libdw_intern_expression (Dwarf *dbg, bool 
other_byte_order,
 
   /* Check whether we already looked at this list.  */
   struct loc_s fake = { .addr = block->data };
-  struct loc_s **found = tfind (&fake, cache, loc_compare);
+  struct loc_s **found = eu_tfind (&fake, cache, loc_compare);
   if (found != NULL)
 {
   /* We already saw it.  */
@@ -655,7 +655,7 @@ __libdw_intern_expression (Dwarf *dbg, bool 
other_byte_order,
   newp->addr = block->data;
   newp->loc = result;
   newp->nloc = *listlen;
-  (void) tsearch (newp, cache, loc_compare);
+  (void) eu_tsearch (newp, cache, loc_compare);
 
   /* We did it.  */
   return 0;
diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index a3a78884..d150f05e 100644
--- a/libdw/dwarf_getmacros.c
+++ b/lib

[PATCH 15/16] tests: Add eu-search tests

2023-10-10 Thread Mark Wielaard

From: Heather McIntyre 

* tests/eu_search_cfi.c: New file.
* tests/eu_search_die.c: New file.
* tests/eu_search_lines.c: New file.
* tests/eu_search_macros.c: New file.
* tests/run-eu-search-tests.sh: New test.
* tests/Makefile.am: Add USE_LOCKS condition for -pthread.
(check_PROGRAMS): Add eu_search_cfi, eu_search_die,
eu_search_lines, and eu_search_macros.
(TESTS): Add run-eu-search-tests.sh
(eu_search_cfi_LDADD): New variable.
(eu_search_die_LDADD): New variable.
(eu_search_lines_LDADD): New variable.
(eu_search_macros_LDADD): New variable.
(eu_search_cfi_LDFLAGS): New variable.
Add -pthread if USE_LOCKS is not defined.
(eu_search_die_LDFLAGS): Likewise.
(eu_search_lines_LDFLAGS): Likewise.
(eu_search_macros_LDFLAGS): Likewise.

Signed-off-by: Heather S. McIntyre 
Signed-off-by: Mark Wielaard 
---
 tests/Makefile.am|  16 ++-
 tests/eu_search_cfi.c| 234 +++
 tests/eu_search_die.c| 262 +++
 tests/eu_search_lines.c  | 228 ++
 tests/eu_search_macros.c | 216 +
 tests/run-eu-search-tests.sh | 168 ++
 6 files changed, 1123 insertions(+), 1 deletion(-)
 create mode 100644 tests/eu_search_cfi.c
 create mode 100644 tests/eu_search_die.c
 create mode 100644 tests/eu_search_lines.c
 create mode 100644 tests/eu_search_macros.c
 create mode 100755 tests/run-eu-search-tests.sh

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 32b18e6e..a66b1033 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -32,6 +32,10 @@ else
 tests_rpath = no
 endif
 
+if USE_LOCKS
+  AM_CFLAGS += -pthread
+endif
+
 check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
  showptable update1 update2 update3 update4 test-nlist \
  show-die-info get-files next-files get-lines next-lines \
@@ -63,6 +67,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames 
sectiondump \
  getphdrnum leb128 read_unaligned \
  msg_tst system-elf-libelf-test system-elf-gelf-test \
  nvidia_extended_linemap_libdw \
+ eu_search_cfi eu_search_die eu_search_lines eu_search_macros \
  $(asm_TESTS)
 
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
@@ -211,7 +216,8 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile 
test-nlist \
$(asm_TESTS) run-disasm-bpf.sh run-low_high_pc-dw-form-indirect.sh \
run-nvidia-extended-linemap-libdw.sh 
run-nvidia-extended-linemap-readelf.sh \
run-readelf-dw-form-indirect.sh run-strip-largealign.sh \
-   run-readelf-Dd.sh
+   run-readelf-Dd.sh \
+   run-eu-search-tests.sh
 
 if !BIARCH
 export ELFUTILS_DISABLE_BIARCH = 1
@@ -804,6 +810,14 @@ getphdrnum_LDADD = $(libelf) $(libdw)
 leb128_LDADD = $(libelf) $(libdw)
 read_unaligned_LDADD = $(libelf) $(libdw)
 nvidia_extended_linemap_libdw_LDADD = $(libelf) $(libdw)
+eu_search_cfi_LDFLAGS = $(if $(filter undefined,$(origin USE_LOCKS)),-pthread) 
$(AM_LDFLAGS)
+eu_search_die_LDFLAGS = $(if $(filter undefined,$(origin USE_LOCKS)),-pthread) 
$(AM_LDFLAGS)
+eu_search_lines_LDFLAGS = $(if $(filter undefined,$(origin 
USE_LOCKS)),-pthread) $(AM_LDFLAGS)
+eu_search_macros_LDFLAGS = $(if $(filter undefined,$(origin 
USE_LOCKS)),-pthread) $(AM_LDFLAGS)
+eu_search_cfi_LDADD = $(libeu) $(libelf) $(libdw)
+eu_search_die_LDADD = $(libdw)
+eu_search_lines_LDADD = $(libdw) $(libelf)
+eu_search_macros_LDADD = $(libdw)
 
 # We want to test the libelf headers against the system elf.h header.
 # Don't include any -I CPPFLAGS. Except when we install our own elf.h.
diff --git a/tests/eu_search_cfi.c b/tests/eu_search_cfi.c
new file mode 100644
index ..0b63b213
--- /dev/null
+++ b/tests/eu_search_cfi.c
@@ -0,0 +1,234 @@
+/*Test program for eu_search_cfi
+   Copyright (C) 2023 Red Hat, Inc.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see.  */
+
+#include 
+#include 
+#include 
+#include ELFUTILS_HEADER(dw)
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "system.h"
+#include 
+
+static

Re: [PATCH 01/16] lib: Add new once_define and once macros to eu-config.h

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * lib/eu-config.h New macros.
>   [USE_LOCKS] (ONCE_CALL): (once_define, once)
> 
> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  lib/eu-config.h | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/eu-config.h b/lib/eu-config.h
> index 78a5c4fe..feb079db 100644
> --- a/lib/eu-config.h
> +++ b/lib/eu-config.h
> @@ -33,13 +33,18 @@
>  # include 
>  # include 
>  # define rwlock_define(class,name)   class pthread_rwlock_t name
> +# define once_define(class,name)  class pthread_once_t name = 
> PTHREAD_ONCE_INIT
>  # define RWLOCK_CALL(call)   \
>({ int _err = pthread_rwlock_ ## call; assert_perror (_err); })
> +# define ONCE_CALL(call)  \
> +  ({ int _err = pthread_ ## call; assert_perror (_err); })
>  # define rwlock_init(lock)   RWLOCK_CALL (init (&lock, NULL))
>  # define rwlock_fini(lock)   RWLOCK_CALL (destroy (&lock))
>  # define rwlock_rdlock(lock) RWLOCK_CALL (rdlock (&lock))
>  # define rwlock_wrlock(lock) RWLOCK_CALL (wrlock (&lock))
>  # define rwlock_unlock(lock) RWLOCK_CALL (unlock (&lock))
> +# define once(once_control, init_routine)  \
> +  ONCE_CALL (once (&once_control, init_routine))
>  #else
>  /* Eventually we will allow multi-threaded applications to use the
> libraries.  Therefore we will add the necessary locking although
> @@ -50,6 +55,8 @@
>  # define rwlock_rdlock(lock) ((void) (lock))
>  # define rwlock_wrlock(lock) ((void) (lock))
>  # define rwlock_unlock(lock) ((void) (lock))
> +# define once_define(class,name)
> +# define once(once_control, init_routine)init_routine()
>  #endif   /* USE_LOCKS */
>  
>  #include 

I made sure that only the newly added lines are changed. That made the
diff a bit easier to review.

This looks good. In the case without locks, the init_routine will of
course get called multiple times, but that should in theory be fine
(these init routines are in generally really simple).

Hopefully we'll eventually end up with just the locked version.

Cheers,

Mark

Re: [PATCH 02/16] libelf: Make elf_version thread-safe

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * elf_version.c (version_once): Define once.
>   (initialize_version): New static function.
>   (elf_version): Use initialize_version version_once.
> 
> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  libelf/elf_version.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/libelf/elf_version.c b/libelf/elf_version.c
> index 6ec534ab..8296bb65 100644
> --- a/libelf/elf_version.c
> +++ b/libelf/elf_version.c
> @@ -32,12 +32,21 @@
>  #endif
>  
>  #include 
> +#include 
>  
> +/* Multiple threads may initialize __libelf_version.
> +   pthread_once() ensures that __libelf_version is initialized only once. */
> +once_define(static, version_once);
>  
>  /* Currently selected version.  Should be EV_CURRENT.
> Will be EV_NONE if elf_version () has not been called yet.  */
>  unsigned int __libelf_version = EV_NONE;
>  
> +static void initialize_version(void)
> +{
> +  __libelf_version = EV_CURRENT;
> +}
> +
>  unsigned int
>  elf_version (unsigned int version)
>  {
> @@ -49,7 +58,7 @@ elf_version (unsigned int version)
>/* Phew, we know this version.  */
>  
>/* Signal that the version is now initialized.  */
> -  __libelf_version = EV_CURRENT;
> +  once(version_once, initialize_version);
>  
>/* And return the last (or initial) version.  */
>return EV_CURRENT;

This is an odd function. The intention clearly was to support more "ELF
versions" at some point. But (luckily) that never happened and I doubt
there will ever be a different (incompatible) ELF version that we'll
have to support. So in the end this will always be EV_CURRENT == 1. But
the function has to be called to make the rest of the library work.

I think this works and is fine. There will most likely never be real
contention calling elf_version because normally a program just calls it
once at the start.

But have you thought about using some atomic operation here instead?

Cheers,

Mark

Re: [PATCH 03/16] libelf: Fix deadlock in __libelf_readall

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * libelf/elf_readall.c (__libelf_readall): Move rwlock_unlock
>   before libelf_acquire_all.
> 
> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  libelf/elf_readall.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libelf/elf_readall.c b/libelf/elf_readall.c
> index d0f9a28c..2d62d447 100644
> --- a/libelf/elf_readall.c
> +++ b/libelf/elf_readall.c
> @@ -84,6 +84,7 @@ __libelf_readall (Elf *elf)
>  
>/* If this is an archive and we have derived descriptors get the
>locks for all of them.  */
> +  rwlock_unlock(elf->lock); // lock will be reacquired next line
>libelf_acquire_all (elf);
>  
>if (elf->maximum_size == ~((size_t) 0))
> @@ -141,10 +142,8 @@ __libelf_readall (Elf *elf)
>   __libelf_seterrno (ELF_E_NOMEM);
>  
>/* Free the locks on the children.  */
> -  libelf_release_all (elf);
> +  libelf_release_all (elf); // lock is released
>  }
>  
> -  rwlock_unlock (elf->lock);
> -
>return (char *) elf->map_address;
>  }

I think this is wrong when this if statement, at the start of the
block, fails:

  /* If the file is not mmap'ed and not previously loaded, do it now.  */
  if (elf->map_address == NULL)

So if elf->map_address != NULL we now never call
rwlock_unlock (elf->lock).

One way to simplify this locking might be to rewrite libelf_acquire_all
and libelf_release_all to libelf_acquire_all_children and
libelf_release_all_children (which would only be called with the elf-
>lock already acquired).

__libelf_readall is the only caller of these functions.

Cheers,

Mark

Re: [PATCH 04/16] libelf: Fix deadlock in elf_cntl

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * libelf/elf_cntl.c (elf_cntl): Move rwlock_wrlock, rwlock_unlock,
>   inside case switch statements.
> 
> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  libelf/elf_cntl.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/libelf/elf_cntl.c b/libelf/elf_cntl.c
> index 04aa9132..64087c7d 100644
> --- a/libelf/elf_cntl.c
> +++ b/libelf/elf_cntl.c
> @@ -48,13 +48,16 @@ elf_cntl (Elf *elf, Elf_Cmd cmd)
>return -1;
>  }
>  
> -  rwlock_wrlock (elf->lock);
> +
>  
>switch (cmd)
>  {
>  case ELF_C_FDREAD:
> + rwlock_rdlock (elf->lock);
> + int addr_isnull = elf->map_address == NULL;
> + rwlock_unlock(elf->lock);
>/* If not all of the file is in the memory read it now.  */
> -  if (elf->map_address == NULL && __libelf_readall (elf) == NULL)
> +  if (addr_isnull && __libelf_readall (elf) == NULL)
>   {
> /* We were not able to read everything.  */
> result = -1;

Can't we just rely on if (__libelf_readall (elf) == NULL)?

__libelf_readall already does locking and will return non-NULL if elf-
>map_address is already set. So it looks like the extra check (and
locking) to check addr_isnull is redundant and just make the code more
complex.

> @@ -64,7 +67,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 looks correct. All other accesses to elf->fildes seem to be done
under the elf->lock too.

> @@ -73,7 +78,5 @@ elf_cntl (Elf *elf, Elf_Cmd cmd)
>break;
>  }
>  
> -  rwlock_unlock (elf->lock);
> -
>return result;
>  }

Re: [PATCH 05/16] libelf: Fix elf_end deadlock

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * libelf/elf_end.c (elf_end): Add rwlock_unlock before
>   early return.
> 
> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  libelf/elf_end.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libelf/elf_end.c b/libelf/elf_end.c
> index 89727cb3..80f4d13f 100644
> --- a/libelf/elf_end.c
> +++ b/libelf/elf_end.c
> @@ -82,7 +82,10 @@ elf_end (Elf *elf)
>elf->state.ar.ar_sym = NULL;
>  
>if (elf->state.ar.children != NULL)
> - return 0;
> + {
> +   rwlock_unlock(elf->lock);
> +   return 0;
> + }
>  }
>  
>/* Remove this structure from the children list.  */

This looks obviously correct. All other early returns also release the
lock.

Thanks,

Mark

Re: [PATCH 06/16] libelf: Make elf32_getchdr and elf64_getchdr thread-safe

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * libelf/elf32_getchdr.c: Move getchdr function to
>   elf32_getchdr.h.
>   * libelf/elf32_getchdr.h: New file.
>   Add macro to create getchdr_wrlock.

That is clever. I do wonder if the new file should be named
elf_getchdr.h (since it isn't really directly 32 or 64 bit, but
included (indirectly) from one of them. This is a nitpick though.

You do have to include it in the noinst_HEADERS:

diff --git a/libelf/Makefile.am b/libelf/Makefile.am
index aabce43e..3402863e 100644
--- a/libelf/Makefile.am
+++ b/libelf/Makefile.am
@@ -41,7 +41,7 @@ include_HEADERS = libelf.h gelf.h nlist.h
 
 noinst_HEADERS = abstract.h common.h exttypes.h gelf_xlate.h libelfP.h \
 version_xlate.h gnuhash_xlate.h note_xlate.h dl-hash.h \
-chdr_xlate.h
+chdr_xlate.h elf32_getchdr.h
 
 if INSTALL_ELFH
 include_HEADERS += elf.h

Otherwise it won't be included in a dist (make distcheck would show
that as an issue).

>   * libelf/elf32_updatenull.c: Change call from getchdr to
>   getchdr_wrlock.
>   * libelf/elf_getdata.c: Add elf_getdata_wrlock.
>   * libelf/libelfP.h: Add internal function declarations.
> 
> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  libelf/elf32_getchdr.c| 46 +++--
>  libelf/elf32_getchdr.h| 61 +++
>  libelf/elf32_updatenull.c |  2 +-
>  libelf/elf_getdata.c  | 14 +
>  libelf/libelfP.h  |  4 +++
>  5 files changed, 84 insertions(+), 43 deletions(-)
>  create mode 100644 libelf/elf32_getchdr.h
> 
> diff --git a/libelf/elf32_getchdr.c b/libelf/elf32_getchdr.c
> index 982a614c..41591300 100644
> --- a/libelf/elf32_getchdr.c
> +++ b/libelf/elf32_getchdr.c
> @@ -38,46 +38,8 @@
>  # define LIBELFBITS 32
>  #endif
>  
> +#define ELF_WRLOCK_HELD 1
> +#include "elf32_getchdr.h"
>  
> -ElfW2(LIBELFBITS,Chdr) *
> -elfw2(LIBELFBITS,getchdr) (Elf_Scn *scn)
> -{
> -  ElfW2(LIBELFBITS,Shdr) *shdr = elfw2(LIBELFBITS,getshdr) (scn);
> -  if (shdr == NULL)
> -return NULL;
> -
> -  /* Must have SHF_COMPRESSED flag set.  Allocated or no bits sections
> - can never be compressed.  */
> -  if ((shdr->sh_flags & SHF_ALLOC) != 0)
> -{
> -  __libelf_seterrno (ELF_E_INVALID_SECTION_FLAGS);
> -  return NULL;
> -}
> -
> -  if (shdr->sh_type == SHT_NULL
> -  || shdr->sh_type == SHT_NOBITS)
> -{
> -  __libelf_seterrno (ELF_E_INVALID_SECTION_TYPE);
> -  return NULL;
> -}
> -
> -  if ((shdr->sh_flags & SHF_COMPRESSED) == 0)
> -{
> -  __libelf_seterrno (ELF_E_NOT_COMPRESSED);
> -  return NULL;
> -}
> -
> -  /* This makes sure the data is in the correct format, so we don't
> - need to swap fields. */
> -  Elf_Data *d  = elf_getdata (scn, NULL);
> -  if (d == NULL)
> -return NULL;
> -
> -  if (d->d_size < sizeof (ElfW2(LIBELFBITS,Chdr)) || d->d_buf == NULL)
> -{
> -  __libelf_seterrno (ELF_E_INVALID_DATA);
> -  return NULL;
> -}
> -
> -  return (ElfW2(LIBELFBITS,Chdr) *) d->d_buf;
> -}
> +#define ELF_WRLOCK_HELD 0
> +#include "elf32_getchdr.h"
> \ No newline at end of file
> diff --git a/libelf/elf32_getchdr.h b/libelf/elf32_getchdr.h
> new file mode 100644
> index ..04d47e7a
> --- /dev/null
> +++ b/libelf/elf32_getchdr.h
> @@ -0,0 +1,61 @@
> +#undef ADD_ROUTINE_PREFIX
> +#undef ADD_ROUTINE_SUFFIX
> +
> +#if ELF_WRLOCK_HELD
> +#define CONCAT(x,y) x##y
> +#define ADD_ROUTINE_PREFIX(y) CONCAT(__,y)
> +#define ADD_ROUTINE_SUFFIX(x) x ## _wrlock
> +#define INTERNAL internal_function
> +#else
> +#define ADD_ROUTINE_PREFIX(y) y
> +#define ADD_ROUTINE_SUFFIX(x) x
> +#define INTERNAL
> +#endif
> +
> +ElfW2(LIBELFBITS,Chdr) *
> +INTERNAL
> +ADD_ROUTINE_PREFIX(elfw2(LIBELFBITS, ADD_ROUTINE_SUFFIX(getchdr))) (Elf_Scn 
> *scn)
> +{
> +
> +  ElfW2(LIBELFBITS,Shdr) *shdr = ADD_ROUTINE_PREFIX(elfw2(LIBELFBITS, 
> ADD_ROUTINE_SUFFIX(getshdr)))(scn);
> +
> +  if (shdr == NULL)
> +return NULL;
> +
> +  /* Must have SHF_COMPRESSED flag set.  Allocated or no bits sections
> + can never be compressed.  */
> +  if ((shdr->sh_flags & SHF_ALLOC) != 0)
> +{
> +  __libelf_seterrno (ELF_E_INVALID_SECTION_FLAGS);
> +  return NULL;
> +}
> +
> +  if (shdr->sh_type == SHT_NULL
> +  || shdr->sh_type == SHT_NOBITS)
> +{
> +  __libelf_seterrno (ELF_E_INVALID_SECTION_TYPE);
> +  return NULL;
> +}
> +
> +  if ((shdr->sh_flags & SHF_COMPRESSED) == 0)
> +{
> +  __libelf_seterrno (ELF_E_NOT_COMPRESSED);
> +  return NULL;
> +}
> +
> +  /* This makes sure the data is in the correct format, so we don't
> + need to swap fields. */
> +  Elf_Data *d  = ADD_ROUTINE_PREFIX(ADD_ROUTINE_SUFFIX(elf_getdata)) (scn, 
> NULL);
> +  if (d == NULL)
> +return NULL;
> +
> +  if (d->d_size < sizeof (ElfW2(LIBELFBITS,Chdr)) || d->d_buf == NULL)
> +

Re: [PATCH 07/16] lib: Add eu_tsearch and eu_tfind

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * lib/eu-search.h: New file.
>   Declarations for read/write locked eu_tsearch/eu_tfind.

Like in the previous case, don't forget to add such a new header to
noinst_HEADERS so make dist adds them.

diff --git a/lib/Makefile.am b/lib/Makefile.am
index ce8f3e1b..9df0a8c6 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -39,5 +39,6 @@ libeu_a_SOURCES = xasprintf.c xstrdup.c xstrndup.c xmalloc.c 
next_prime.c \
 
 noinst_HEADERS = fixedsizehash.h libeu.h system.h dynamicsizehash.h list.h \
 eu-config.h color.h printversion.h bpf.h \
-atomics.h stdatomic-fbsd.h dynamicsizehash_concurrent.h
+atomics.h stdatomic-fbsd.h dynamicsizehash_concurrent.h \
+eu-search.h
 EXTRA_DIST = dynamicsizehash.c dynamicsizehash_concurrent.c

>   * lib/eu-search.c: New file.
>   Definitions for read/write locked eu_tsearch/eu_tfind.
>   * Makefile.am (libeu_a_SOURCES): Add eu-search.c.
> 
> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  lib/Makefile.am |  2 +-
>  lib/eu-search.c | 60 +
>  lib/eu-search.h | 39 
>  3 files changed, 100 insertions(+), 1 deletion(-)
>  create mode 100644 lib/eu-search.c
>  create mode 100644 lib/eu-search.h
> 
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index b3bb929f..ce8f3e1b 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -34,7 +34,7 @@ AM_CPPFLAGS += -I$(srcdir)/../libelf
>  noinst_LIBRARIES = libeu.a
>  
>  libeu_a_SOURCES = xasprintf.c xstrdup.c xstrndup.c xmalloc.c next_prime.c \
> -   crc32.c crc32_file.c \
> +   crc32.c crc32_file.c eu-search.c \
> color.c error.c printversion.c

OK.

>  noinst_HEADERS = fixedsizehash.h libeu.h system.h dynamicsizehash.h list.h \
> diff --git a/lib/eu-search.c b/lib/eu-search.c
> new file mode 100644
> index ..a6b04f4f
> --- /dev/null
> +++ b/lib/eu-search.c
> @@ -0,0 +1,60 @@
> +/* Definitions for thread-safe tsearch/tfind
> +   Copyright (C) 2023 Rice University
> +   This file is part of elfutils.
> +
> +   This file is free software; you can redistribute it and/or modify
> +   it under the terms of either
> +
> + * the GNU Lesser General Public License as published by the Free
> +   Software Foundation; either version 3 of the License, or (at
> +   your option) any later version
> +
> +   or
> +
> + * the GNU General Public License as published by the Free
> +   Software Foundation; either version 2 of the License, or (at
> +   your option) any later version
> +
> +   or both in parallel, as here.
> +
> +   elfutils is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received copies of the GNU General Public License and
> +   the GNU Lesser General Public License along with this program.  If
> +   not, see .  */
> +
> +#ifdef HAVE_CONFIG_H
> +#include 
> +#endif
> +
> +#include 
> +#include "eu-search.h"
> +
> +rwlock_define(static, search_find_lock);
> +
> +void *eu_tsearch(const void *key, void **rootp,
> +  int (*compar)(const void *, const void *))
> +{
> +  void *ret = NULL;
> +  rwlock_wrlock(search_find_lock);
> +
> +  ret = tsearch(key, rootp, compar);
> +
> +  rwlock_unlock(search_find_lock);
> +  return ret;
> +}
> +
> +void *eu_tfind(const void *key, void *const *rootp,
> +int (*compar)(const void *, const void *))
> +{
> +  void *ret = NULL;
> +  rwlock_rdlock(search_find_lock);
> +
> +  ret = tfind(key, rootp, compar);
> +
> +  rwlock_unlock(search_find_lock);
> +  return ret;
> +}

So this uses on static lock for all eu-tsearch/eu-tfind calls. But
searches with different roots should be independent. Would it make
sense to add different locks for different roots?

That would make the code a little more complicated, but we could hide
the root and lock in a new structure that will be passed the the
eu_tsearch/eu_tfind calls.

Maybe something like:

struct eu_search_root
{
  void *root;
  rwlock_define (,lock);
};

With helper functions to create/init and destroy/delete them?
void eu_tinit (struct eu_search_root *search_root);
(Or is there no real initing to do?)

void eu_tdestroy (struct eu_search_root *search_root,
  void (*free_node)(void *nodep));

Or is all this way too complicated and a global lock just fine?

Cheers,

Mark

> diff --git a/lib/eu-search.h b/lib/eu-search.h
> new file mode 100644
> index ..4ce0139a
> --- /dev/null
> +++ b/lib/eu-search.h
> @@ -0,0 +1,39 @@
> +/* Calls for thread-safe tsearch/tfind
> +   Copyright (C) 2023 Rice University
> +   This file is

[Bug debuginfod/30962] New: ensure x-debuginfod-file / -archive headers always return full path names

2023-10-10 Thread fche at redhat dot com

https://sourceware.org/bugzilla/show_bug.cgi?id=30962

Bug ID: 30962
   Summary: ensure x-debuginfod-file / -archive headers always
return full path names
   Product: elfutils
   Version: unspecified
Status: NEW
  Severity: normal
  Priority: P2
 Component: debuginfod
  Assignee: unassigned at sourceware dot org
  Reporter: fche at redhat dot com
CC: elfutils-devel at sourceware dot org
  Target Milestone: ---

We kind of decided haphazardly to `basename` some of those strings and not
others.  This was partly with some halfbaked notion of security in mind, to
avoid exposing path names on a server.

We should switch to full path names: basically just return everything in its
raw form: file names, archive names, names-from-archives.  This would simplify
things, and making naming consistent between x-debuginfod-file: BASENAME and
the forthcoming metadata-query FULLPATH.   And we can document how to strip
sensitive prefixes if admins want to keep things hush-hush.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

PATCH PR30962, debuginfod

2023-10-10 Thread Frank Ch. Eigler

commit e967988e419121cad1d7f40013a316059b1173f0
Author: Frank Ch. Eigler 
Date:   Tue Oct 10 16:21:00 2023 -0400

PR30962: debuginfod: full paths for X-DEBUGINFOD-FILE/ARCHIVE response 
headers

Previous code was inconsistent in offering basename versus full
pathname for these headers.  The documentation was not explicit on
this issue.  We now simplify by always passing full names back, and
document this in the debuginfod.8 man page, along with pointers to
how to use proxy front-end servers to strip them if needed.

Signed-Off-By: Frank Ch. Eigler 

diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index e53228803bb0..c11aeda1a3af 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1876,11 +1876,10 @@ handle_buildid_f_match (bool internal_req_t,
 }
   else
 {
-  std::string file = b_source0.substr(b_source0.find_last_of("/")+1, 
b_source0.length());
   add_mhd_response_header (r, "Content-Type", "application/octet-stream");
   add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
   to_string(s.st_size).c_str());
-  add_mhd_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
+  add_mhd_response_header (r, "X-DEBUGINFOD-FILE", b_source0.c_str());
   add_mhd_last_modified (r, s.st_mtime);
   if (verbose > 1)
obatched(clog) << "serving file " << b_source0 << " section=" << 
section << endl;
@@ -2164,14 +2163,12 @@ handle_buildid_r_match (bool internal_req_p,
 }
   else
 {
-  std::string file = b_source1.substr(b_source1.find_last_of("/")+1, 
b_source1.length());
   add_mhd_response_header (r, "Content-Type",
"application/octet-stream");
   add_mhd_response_header (r, "X-DEBUGINFOD-SIZE",
to_string(archive_entry_size(e)).c_str());
-  add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE",
-   b_source0.c_str());
-  add_mhd_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
+  add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE", 
b_source0.c_str());
+  add_mhd_response_header (r, "X-DEBUGINFOD-FILE", b_source1.c_str());
   add_mhd_last_modified (r, archive_entry_mtime(e));
   if (verbose > 1)
obatched(clog) << "serving archive " << b_source0
diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
index d4316bec8175..7003a5823d34 100644
--- a/doc/debuginfod.8
+++ b/doc/debuginfod.8
@@ -307,11 +307,11 @@ can take advantage of standard HTTP management 
infrastructure.
 Upon finding a file in an archive or simply in the database, some
 custom http headers are added to the response. For files in the
 database X-DEBUGINFOD-FILE and X-DEBUGINFOD-SIZE are added.
-X-DEBUGINFOD-FILE is simply the unescaped filename and
+X-DEBUGINFOD-FILE is simply the full path name and
 X-DEBUGINFOD-SIZE is the size of the file. For files found in archives,
 in addition to X-DEBUGINFOD-FILE and X-DEBUGINFOD-SIZE,
-X-DEBUGINFOD-ARCHIVE is added.  X-DEBUGINFOD-ARCHIVE is the name of the
-archive the file was found in.
+X-DEBUGINFOD-ARCHIVE is added.  X-DEBUGINFOD-ARCHIVE is the full path
+name of the archive the file was found in.
 
 There are three requests.  In each case, the buildid is encoded as a
 lowercase hexadecimal string.  For example, for a program \fI/bin/ls\fP,
@@ -485,8 +485,9 @@ a denial-of-service in terms of RAM, CPU, disk I/O, or 
network I/O.
 If this is a problem, users are advised to install debuginfod with a
 HTTPS reverse-proxy front-end that enforces site policies for
 firewalling, authentication, integrity, authorization, and load
-control.  The \fI/metrics\fP webapi endpoint is probably not
-appropriate for disclosure to the public.
+control.  Front-end proxies can also elide sensitive path name
+components in X-DEBUGINFOD-FILE/ARCHIVE response headers,
+for example using Apache httpd's \fBmod_header\fP "Header edit".
 
 When relaying queries to upstream debuginfods, debuginfod \fBdoes not\fP
 include any particular security features.  It trusts that the binaries
diff --git a/tests/run-debuginfod-response-headers.sh 
b/tests/run-debuginfod-response-headers.sh
index 8cb7b843d19d..fbb6a4842fa4 100755
--- a/tests/run-debuginfod-response-headers.sh
+++ b/tests/run-debuginfod-response-headers.sh
@@ -78,8 +78,8 @@ tempfiles vlog-find$PORT1.1
 errfiles vlog-find$PORT1.1
 cat vlog-find$PORT1.1
 grep 'Headers:' vlog-find$PORT1.1
-grep -i 'X-DEBUGINFOD-FILE: prog' vlog-find$PORT1.1
-grep -i 'X-DEBUGINFOD-SIZE: ' vlog-find$PORT1.1
+grep -i 'X-DEBUGINFOD-FILE: .*/prog' vlog-find$PORT1.1
+grep -i 'X-DEBUGINFOD-SIZE: 'vlog-find$PORT1.1
 
 # Check to see if an executable file located in an archive prints the file's 
description and archive
 env DEBUGINFOD_URLS="http://127.0.0.1:"$PORT1 LD_LIBRARY_PATH=$ldpath 
${abs_top_builddir}/debuginfod/debuginfod-find\
@@ -88,9 +88,9 @@

Re: [PATCH 08/16] libcpu: Change calls for tsearch/tfind to eu_tsearch/eu_tfind.

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, Oct 10, 2023 at 03:42:52PM +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * libcpu/Makefile.am: Add USE_LOCKS condition for -pthread.
>   * libcpu/i386_parse.y: Add eu-search.h and remove search.h.
>   Change calls for tsearch/tfind to eu_tsearch/eu_tfind.

So in theory this looks like a simple obvious change to use eu-search
instead of search. But I am not sure ths bison parser can even be used
concurrently. It looks like it is using a lot of statics. Also
i386_parse.y uses twalk, which also would have to be wrapped (no other
part of elfutils uses twalk).

I think I would advise to skip the i386 assembler. It isn't really
complete at the moment and it is probably a lot of work to get a
concurrent bison parser.

Cheers,

Mark

> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  libcpu/Makefile.am  |  3 +++
>  libcpu/i386_parse.y | 48 ++---
>  2 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/libcpu/Makefile.am b/libcpu/Makefile.am
> index 4ba1be56..a51334b5 100644
> --- a/libcpu/Makefile.am
> +++ b/libcpu/Makefile.am
> @@ -33,6 +33,9 @@ AM_CPPFLAGS += -I$(srcdir)/../libelf -I$(srcdir)/../libebl \
>  if BUILD_STATIC
>  AM_CFLAGS += $(fpic_CFLAGS)
>  endif
> +if USE_LOCKS
> +  AM_CFLAGS += -pthread
> +endif
>  AM_CFLAGS += -fdollars-in-identifiers
>  LEXCOMPILE = $(LEX) $(LFLAGS) $(AM_LFLAGS) -P$(  LEX_OUTPUT_ROOT = lex.$( diff --git a/libcpu/i386_parse.y b/libcpu/i386_parse.y
> index 459684c6..3d7cb89e 100644
> --- a/libcpu/i386_parse.y
> +++ b/libcpu/i386_parse.y
> @@ -37,7 +37,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -269,12 +269,12 @@ mask: kMASK kBITFIELD kNUMBER
> struct synonym *newp = xmalloc (sizeof (*newp));
> newp->from = $2;
> newp->to = $3;
> -   if (tfind (newp, &synonyms, compare_syn) != NULL)
> +   if (eu_tfind (newp, &synonyms, compare_syn) != NULL)
>   error (0, 0,
>  "%d: duplicate definition for synonym '%s'",
>  i386_lineno, $2);
> -   else if (tsearch ( newp, &synonyms, compare_syn) == NULL)
> - error (EXIT_FAILURE, 0, "tsearch");
> +   else if (eu_tsearch ( newp, &synonyms, compare_syn) == 
> NULL)
> + error (EXIT_FAILURE, 0, "eu_tsearch");
>   }
>   |
>   ;
> @@ -308,12 +308,12 @@ instr:bytes ':' bitfieldopt kID bitfieldopt 
> optargs
> newp->bytes = $1;
> newp->mnemonic = $4;
> if (newp->mnemonic != (void *) -1l
> -   && tfind ($4, &mnemonics,
> +   && eu_tfind ($4, &mnemonics,
>   (int (*)(const void *, const void *)) 
> strcmp) == NULL)
>   {
> -   if (tsearch ($4, &mnemonics,
> +   if (eu_tsearch ($4, &mnemonics,
>  (int (*)(const void *, const void 
> *)) strcmp) == NULL)
> - error (EXIT_FAILURE, errno, "tsearch");
> + error (EXIT_FAILURE, errno, "eu_tsearch");
> ++nmnemonics;
>   }
>  
> @@ -339,15 +339,15 @@ instr:bytes ':' bitfieldopt kID bitfieldopt 
> optargs
>  infname, i386_lineno - 1, $5->name);
>  
> struct suffix search = { .name = $5->name };
> -   if (tfind (&search, &suffixes, compare_suf)
> +   if (eu_tfind (&search, &suffixes, compare_suf)
> == NULL)
>   {
> struct suffix *ns = xmalloc (sizeof (*ns));
> ns->name = $5->name;
> ns->idx = ++nsuffixes;
> -   if (tsearch (ns, &suffixes, compare_suf)
> +   if (eu_tsearch (ns, &suffixes, compare_suf)
> == NULL)
> - error (EXIT_FAILURE, errno, "tsearch");
> + error (EXIT_FAILURE, errno, "eu_tsearch");
>   }
>   }
>  
> @@ -374,7 +374,7 @@ bitfieldopt:kBITFIELD
> struct known_bitfield search;
> search.name = $1;
> struct known_bitfield **res;
> -   res = tfind (&search, &bitfields, bitfield_compare);
> +   res = eu_tfind (&search, &bitfields, bitfield_compare);
>

Re: [PATCH 09/16] src: Use eu-search in nm and findtextrel.

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, Oct 10, 2023 at 03:42:53PM +0200, Mark Wielaard wrote:
> From: Heather McIntyre 
> 
>   * src/Makefile.am: Add USE_LOCKS condition for -pthread.
>   * src/findtextrel.c: Add eu-search.h and remove search.h.
>   Change calls of tsearch/tfind to eu_tsearch/eu_tfind.
>   * src/nm.c: Likewise.

This does look technically correct. But both nm and findtextrel are
single threaded programs. It might be interesting to try to make them
parallel. But currently I think this would just introduce unnecessary
locking.

Cheers,

Mark

> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  src/Makefile.am   |  3 +++
>  src/findtextrel.c | 10 +-
>  src/nm.c  | 10 +-
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 10d59a48..fea5d43e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -22,6 +22,9 @@ DEFS += $(YYDEBUG) -DDEBUGPRED=@DEBUGPRED@ \
>  AM_CPPFLAGS += -I$(srcdir)/../libelf -I$(srcdir)/../libebl \
>   -I$(srcdir)/../libdw -I$(srcdir)/../libdwelf \
>   -I$(srcdir)/../libdwfl -I$(srcdir)/../libasm
> +if USE_LOCKS
> +  AM_CFLAGS += -pthread
> +endif
>  
>  AM_LDFLAGS = -Wl,-rpath-link,../libelf:../libdw $(STACK_USAGE_NO_ERROR)
>  
> diff --git a/src/findtextrel.c b/src/findtextrel.c
> index d3021a3a..5ac4c29a 100644
> --- a/src/findtextrel.c
> +++ b/src/findtextrel.c
> @@ -27,7 +27,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -501,10 +501,10 @@ check_rel (size_t nsegments, struct segments 
> segments[nsegments],
>   /* There can be more than one relocation against one file.
>  Try to avoid multiple messages.  And yes, the code uses
>  pointer comparison.  */
> - if (tfind (src, knownsrcs, ptrcompare) == NULL)
> + if (eu_tfind (src, knownsrcs, ptrcompare) == NULL)
> {
>   printf (_("%s not compiled with -fpic/-fPIC\n"), src);
> - tsearch (src, knownsrcs, ptrcompare);
> + eu_tsearch (src, knownsrcs, ptrcompare);
> }
>   return;
> }
> @@ -555,12 +555,12 @@ check_rel (size_t nsegments, struct segments 
> segments[nsegments],
>   if (sym->st_value + sym->st_size > addr)
> {
>   /* It is this function.  */
> - if (tfind (lowstr, knownsrcs, ptrcompare) == NULL)
> + if (eu_tfind (lowstr, knownsrcs, ptrcompare) == NULL)
> {
>   printf (_("\
>  the file containing the function '%s' is not compiled with -fpic/-fPIC\n"),
>   lowstr);
> - tsearch (lowstr, knownsrcs, ptrcompare);
> + eu_tsearch (lowstr, knownsrcs, ptrcompare);
> }
> }
>   else if (highidx == -1)
> diff --git a/src/nm.c b/src/nm.c
> index fbdee8e1..44c20fb2 100644
> --- a/src/nm.c
> +++ b/src/nm.c
> @@ -32,7 +32,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -537,7 +537,7 @@ static int
>  get_global (Dwarf *dbg __attribute__ ((unused)), Dwarf_Global *global,
>   void *arg __attribute__ ((unused)))
>  {
> -  tsearch (memcpy (xmalloc (sizeof (Dwarf_Global)), global,
> +  eu_tsearch (memcpy (xmalloc (sizeof (Dwarf_Global)), global,
>  sizeof (Dwarf_Global)),
>  &global_root, global_compare);
>  
> @@ -696,7 +696,7 @@ get_local_names (Dwarf *dbg)
>  /* Check whether a similar local_name is already in the
> cache.  That should not happen.  But if it does, we
> don't want to leak memory.  */
> - struct local_name **tres = tsearch (newp, &local_root,
> + struct local_name **tres = eu_tsearch (newp, &local_root,
>   local_compare);
>   if (tres == NULL)
>error_exit (errno, _("cannot create search tree"));
> @@ -1387,7 +1387,7 @@ show_symbols (int fd, Ebl *ebl, GElf_Ehdr *ehdr,
> && global_root != NULL)
>   {
> Dwarf_Global fake = { .name = symstr };
> -   Dwarf_Global **found = tfind (&fake, &global_root,
> +   Dwarf_Global **found = eu_tfind (&fake, &global_root,
>   global_compare);
> if (found != NULL)
>   {
> @@ -1442,7 +1442,7 @@ show_symbols (int fd, Ebl *ebl, GElf_Ehdr *ehdr,
> .lowpc = sym->st_value,
> .highpc = sym->st_value,
>   };
> -   struct local_name **found = tfind (&fake, &local_root,
> +   struct local_name **found = eu_tfind (&fake, &local_root,
>local_compare);
> if (found != NU

Re: [PATCH 10/16] libdw: make dwarf_getalt thread-safe

2023-10-10 Thread Mark Wielaard

Hi Heather,

On Tue, Oct 10, 2023 at 03:42:54PM +0200, Mark Wielaard wrote:
>   * libdw/dwarf_getalt.c: Add lock for
>   dbg->alt_dwarf/main->alt_dwarf.

This takes care of dwarf_getalt. Shouldn't dwarf_setalt also use
locking?

> Signed-off-by: Heather S. McIntyre 
> Signed-off-by: Mark Wielaard 
> ---
>  libdw/dwarf_getalt.c | 27 ++-
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c
> index 0a12dfae..e3894c8c 100644
> --- a/libdw/dwarf_getalt.c
> +++ b/libdw/dwarf_getalt.c
> @@ -44,6 +44,10 @@
>  #include 
>  #include 
>  
> +/* find_debug_altlink() modifies "dbg->alt_dwarf".
> +   dwarf_getalt() reads "main->alt_dwarf".
> +   Mutual exclusion is enforced to prevent a race. */
> +rwlock_define(static, alt_dwarf_lock);

Probably overkill, but should we consider a Dwarf lock object instead
of having a static global lock?

>  char *
>  internal_function
> @@ -152,7 +156,9 @@ find_debug_altlink (Dwarf *dbg)
>Dwarf *alt = dwarf_begin (fd, O_RDONLY);
>if (alt != NULL)
>   {
> +   rwlock_wrlock(alt_dwarf_lock);
> dbg->alt_dwarf = alt;
> +   rwlock_unlock(alt_dwarf_lock);
> dbg->alt_fd = fd;
>   }
>else

Is this lock wide enough? See also below. It looks like multiple
threads could arrive at this point at the same time. so alt_dwarf and
alt_fd can be (re)set multiple times, causing leaks of the Dwarf and
fd.

> @@ -163,22 +169,33 @@ find_debug_altlink (Dwarf *dbg)
>  Dwarf *
>  dwarf_getalt (Dwarf *main)
>  {
> +  rwlock_rdlock(alt_dwarf_lock);
> +  Dwarf *alt_dwarf_local = main->alt_dwarf;
> +  rwlock_unlock(alt_dwarf_lock);
> +
>/* Only try once.  */
> -  if (main == NULL || main->alt_dwarf == (void *) -1)
> +  if (main == NULL || alt_dwarf_local == (void *) -1)
>  return NULL;
>  
> -  if (main->alt_dwarf != NULL)
> -return main->alt_dwarf;
> +  if (alt_dwarf_local != NULL)
> +return alt_dwarf_local;
>  

So at this point it looks like we can have multiple threads running
(because the lock has been dropped above) all with alt_dwarf_local
(and main->alt_dwarf) being NULL.

>find_debug_altlink (main);
>  
> +  rwlock_rdlock(alt_dwarf_lock);
> +  alt_dwarf_local = main->alt_dwarf;
> +  rwlock_unlock(alt_dwarf_lock);
> +
>/* If we found nothing, make sure we don't try again.  */
> -  if (main->alt_dwarf == NULL)
> +  if (alt_dwarf_local == NULL)
>  {
> +  rwlock_wrlock(alt_dwarf_lock);
>main->alt_dwarf = (void *) -1;
> +  rwlock_unlock(alt_dwarf_lock);
> +
>return NULL;
>  }
>  
> -  return main->alt_dwarf;
> +  return alt_dwarf_local;
>  }
>  INTDEF (dwarf_getalt)
> -- 
> 2.41.0
> 

It might be better to take the lock over the whole function (and only
call find_debug_altlink with the lock held).

Cheers,

Mark

Re: [PATCH] Fix thread-safety for elfutils

[PATCH 03/16] libelf: Fix deadlock in __libelf_readall

[PATCH 02/16] libelf: Make elf_version thread-safe

[PATCH 05/16] libelf: Fix elf_end deadlock

[PATCH 04/16] libelf: Fix deadlock in elf_cntl

[PATCH 01/16] lib: Add new once_define and once macros to eu-config.h

[PATCH 08/16] libcpu: Change calls for tsearch/tfind to eu_tsearch/eu_tfind.

[PATCH 06/16] libelf: Make elf32_getchdr and elf64_getchdr thread-safe

[PATCH 07/16] lib: Add eu_tsearch and eu_tfind

[PATCH 11/16] libdw: Add locking around __libdw_dieabbrev for dwarf_hasattr

[PATCH 12/16] libdw: Make libdw_find_split_unit thread-safe

[PATCH 10/16] libdw: make dwarf_getalt thread-safe

[PATCH 16/16] configure: No longer mark --enable-thread-safety as EXPERIMENTAL

[PATCH 13/16] libdw: Make libdw_findcu thread-safe

[PATCH 09/16] src: Use eu-search in nm and findtextrel.

[PATCH 14/16] libdw,libdwfl: Use eu-search for thread-safety

[PATCH 15/16] tests: Add eu-search tests

Re: [PATCH 01/16] lib: Add new once_define and once macros to eu-config.h

Re: [PATCH 02/16] libelf: Make elf_version thread-safe

Re: [PATCH 03/16] libelf: Fix deadlock in __libelf_readall

Re: [PATCH 04/16] libelf: Fix deadlock in elf_cntl

Re: [PATCH 05/16] libelf: Fix elf_end deadlock

Re: [PATCH 06/16] libelf: Make elf32_getchdr and elf64_getchdr thread-safe

Re: [PATCH 07/16] lib: Add eu_tsearch and eu_tfind

[Bug debuginfod/30962] New: ensure x-debuginfod-file / -archive headers always return full path names

PATCH PR30962, debuginfod

Re: [PATCH 08/16] libcpu: Change calls for tsearch/tfind to eu_tsearch/eu_tfind.

Re: [PATCH 09/16] src: Use eu-search in nm and findtextrel.

Re: [PATCH 10/16] libdw: make dwarf_getalt thread-safe

29 matches

Site Navigation

Mail list logo

Footer information