diff mbox series

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

Message ID 20231010134300.53830-6-mark@klomp.org
State Committed
Headers
Series [01/16] lib: Add new once_define and once macros to eu-config.h |

Commit Message

Mark Wielaard Oct. 10, 2023, 1:42 p.m. UTC 
  From: Heather McIntyre <hsm2@rice.edu>

	* 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 <hsm2@rice.edu>
Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 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

Comments

Mark Wielaard Oct. 10, 2023, 4:28 p.m. UTC | #1 
Hi Heather,

On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
> 	* 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 <hsm2@rice.edu>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> ---
>  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 00000000..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;
> +}

It is interesting the general version and the version that is called
when the lock are held are totally separate functions. As far as I can
see that works out fine here.

Just surprising because in most other functions that have a wrlock
variant the general version takes a lock and then calls the wrlock
variant.

> +#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;

OK, so this is the only caller of the new getchdr_wrlock variants.

> 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_Data *data)
> +{
> +  Elf_Data *result;
> +
> +  if (scn == NULL)
> +    return NULL;
> +
> +  result = __elf_getdata_rdlock (scn, data);
> +
> +  return result;
> +}
>  INTDEF(elf_getdata)

Right. So now the getchdr_wrlock variants can call elf_getdata.
This is good. But I have been tinkering with trying to get rid of the
elf_getdata call from elf[32|64]_getchdr. It can result in a lot of
work (copying all of the compressed data) just to get the header. But
that is a work in progress (and would also change elf_compress a bit).

> diff --git a/libelf/libelfP.h b/libelf/libelfP.h
> index 96476064..ed061abb 100644
> --- a/libelf/libelfP.h
> +++ b/libelf/libelfP.h
> @@ -514,6 +514,8 @@ extern Elf32_Shdr *__elf32_getshdr_rdlock (Elf_Scn *__scn) internal_function;
>  extern Elf64_Shdr *__elf64_getshdr_rdlock (Elf_Scn *__scn) internal_function;
>  extern Elf32_Shdr *__elf32_getshdr_wrlock (Elf_Scn *__scn) internal_function;
>  extern Elf64_Shdr *__elf64_getshdr_wrlock (Elf_Scn *__scn) internal_function;
> +extern Elf32_Chdr *__elf32_getchdr_wrlock (Elf_Scn *__scn) internal_function;
> +extern Elf64_Chdr *__elf64_getchdr_wrlock (Elf_Scn *__scn) internal_function;
>  extern Elf_Scn *__elf_getscn_internal (Elf *__elf, size_t __index)
>       attribute_hidden;
>  extern Elf_Scn *__elf_nextscn_internal (Elf *__elf, Elf_Scn *__scn)
> @@ -523,6 +525,8 @@ extern Elf_Data *__elf_getdata_internal (Elf_Scn *__scn, Elf_Data *__data)
>       attribute_hidden;
>  extern Elf_Data *__elf_getdata_rdlock (Elf_Scn *__scn, Elf_Data *__data)
>       internal_function;
> +extern Elf_Data *__elf_getdata_wrlock (Elf_Scn *__scn, Elf_Data *__data)
> +     internal_function;
>  extern Elf_Data *__elf_rawdata_internal (Elf_Scn *__scn, Elf_Data *__data)
>       attribute_hidden;
>  /* Should be called to setup first section data element if

Looks good with that noinstall_HEADERS fix.

Cheers,

Mark
diff mbox series

Patch

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 00000000..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_Data *data)
+{
+  Elf_Data *result;
+
+  if (scn == NULL)
+    return NULL;
+
+  result = __elf_getdata_rdlock (scn, data);
+
+  return result;
+}
 INTDEF(elf_getdata)
diff --git a/libelf/libelfP.h b/libelf/libelfP.h
index 96476064..ed061abb 100644
--- a/libelf/libelfP.h
+++ b/libelf/libelfP.h
@@ -514,6 +514,8 @@  extern Elf32_Shdr *__elf32_getshdr_rdlock (Elf_Scn *__scn) internal_function;
 extern Elf64_Shdr *__elf64_getshdr_rdlock (Elf_Scn *__scn) internal_function;
 extern Elf32_Shdr *__elf32_getshdr_wrlock (Elf_Scn *__scn) internal_function;
 extern Elf64_Shdr *__elf64_getshdr_wrlock (Elf_Scn *__scn) internal_function;
+extern Elf32_Chdr *__elf32_getchdr_wrlock (Elf_Scn *__scn) internal_function;
+extern Elf64_Chdr *__elf64_getchdr_wrlock (Elf_Scn *__scn) internal_function;
 extern Elf_Scn *__elf_getscn_internal (Elf *__elf, size_t __index)
      attribute_hidden;
 extern Elf_Scn *__elf_nextscn_internal (Elf *__elf, Elf_Scn *__scn)
@@ -523,6 +525,8 @@  extern Elf_Data *__elf_getdata_internal (Elf_Scn *__scn, Elf_Data *__data)
      attribute_hidden;
 extern Elf_Data *__elf_getdata_rdlock (Elf_Scn *__scn, Elf_Data *__data)
      internal_function;
+extern Elf_Data *__elf_getdata_wrlock (Elf_Scn *__scn, Elf_Data *__data)
+     internal_function;
 extern Elf_Data *__elf_rawdata_internal (Elf_Scn *__scn, Elf_Data *__data)
      attribute_hidden;
 /* Should be called to setup first section data element if