Hi Heather,
On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote:
> From: Heather McIntyre <h...@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 <h...@rice.edu>
> Signed-off-by: Mark Wielaard <m...@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
