From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id E2A5D3858D28 for ; Tue, 10 Oct 2023 16:28:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E2A5D3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from r6.localdomain (82-217-174-174.cable.dynamic.v4.ziggo.nl [82.217.174.174]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 541C2300B302; Tue, 10 Oct 2023 18:28:08 +0200 (CEST) Received: by r6.localdomain (Postfix, from userid 1000) id 726AA34032D; Tue, 10 Oct 2023 18:28:07 +0200 (CEST) Message-ID: <43fe189a78b6fec08bfec638ecfea0c99b9fd2cd.camel@klomp.org> Subject: Re: [PATCH 06/16] libelf: Make elf32_getchdr and elf64_getchdr thread-safe From: Mark Wielaard To: elfutils-devel@sourceware.org Cc: hsm2@rice.edu Date: Tue, 10 Oct 2023 18:28:07 +0200 In-Reply-To: <20231010134300.53830-6-mark@klomp.org> References: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> <20231010134300.53830-1-mark@klomp.org> <20231010134300.53830-6-mark@klomp.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) MIME-Version: 1.0 X-Spam-Status: No, score=-3033.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Heather, On Tue, 2023-10-10 at 15:42 +0200, Mark Wielaard wrote: > From: Heather McIntyre >=20 > * 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 =3D libelf.h gelf.h nlist.h =20 noinst_HEADERS =3D 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 =20 if INSTALL_ELFH include_HEADERS +=3D 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. >=20 > 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 >=20 > 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 > =20 > +#define ELF_WRLOCK_HELD 1 > +#include "elf32_getchdr.h" > =20 > -ElfW2(LIBELFBITS,Chdr) * > -elfw2(LIBELFBITS,getchdr) (Elf_Scn *scn) > -{ > - ElfW2(LIBELFBITS,Shdr) *shdr =3D elfw2(LIBELFBITS,getshdr) (scn); > - if (shdr =3D=3D NULL) > - return NULL; > - > - /* Must have SHF_COMPRESSED flag set. Allocated or no bits sections > - can never be compressed. */ > - if ((shdr->sh_flags & SHF_ALLOC) !=3D 0) > - { > - __libelf_seterrno (ELF_E_INVALID_SECTION_FLAGS); > - return NULL; > - } > - > - if (shdr->sh_type =3D=3D SHT_NULL > - || shdr->sh_type =3D=3D SHT_NOBITS) > - { > - __libelf_seterrno (ELF_E_INVALID_SECTION_TYPE); > - return NULL; > - } > - > - if ((shdr->sh_flags & SHF_COMPRESSED) =3D=3D 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 =3D elf_getdata (scn, NULL); > - if (d =3D=3D NULL) > - return NULL; > - > - if (d->d_size < sizeof (ElfW2(LIBELFBITS,Chdr)) || d->d_buf =3D=3D NUL= L) > - { > - __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 =3D ADD_ROUTINE_PREFIX(elfw2(LIBELFBITS, = ADD_ROUTINE_SUFFIX(getshdr)))(scn); > + > + if (shdr =3D=3D NULL) > + return NULL; > + > + /* Must have SHF_COMPRESSED flag set. Allocated or no bits sections > + can never be compressed. */ > + if ((shdr->sh_flags & SHF_ALLOC) !=3D 0) > + { > + __libelf_seterrno (ELF_E_INVALID_SECTION_FLAGS); > + return NULL; > + } > + > + if (shdr->sh_type =3D=3D SHT_NULL > + || shdr->sh_type =3D=3D SHT_NOBITS) > + { > + __libelf_seterrno (ELF_E_INVALID_SECTION_TYPE); > + return NULL; > + } > + > + if ((shdr->sh_flags & SHF_COMPRESSED) =3D=3D 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 =3D ADD_ROUTINE_PREFIX(ADD_ROUTINE_SUFFIX(elf_getdata)) (= scn, NULL); > + if (d =3D=3D NULL) > + return NULL; > + > + if (d->d_size < sizeof (ElfW2(LIBELFBITS,Chdr)) || d->d_buf =3D=3D NUL= L) > + { > + __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 =3D elfw2(LIBELFBITS,getchdr) (scn); > + chdr =3D __elfw2(LIBELFBITS,getchdr_wrlock) (scn); > if (unlikely (chdr =3D=3D NULL)) > return -1; > sh_size =3D 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) > =20 > return result; > } > + > +Elf_Data * > +internal_function > +__elf_getdata_wrlock (Elf_Scn *scn, Elf_Data *data) > +{ > + Elf_Data *result; > + > + if (scn =3D=3D NULL) > + return NULL; > + > + result =3D __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_func= tion; > extern Elf32_Shdr *__elf32_getshdr_wrlock (Elf_Scn *__scn) internal_func= tion; > extern Elf64_Shdr *__elf64_getshdr_wrlock (Elf_Scn *__scn) internal_func= tion; > +extern Elf32_Chdr *__elf32_getchdr_wrlock (Elf_Scn *__scn) internal_func= tion; > +extern Elf64_Chdr *__elf64_getchdr_wrlock (Elf_Scn *__scn) internal_func= tion; > 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 *__s= cn, 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 *__dat= a) > attribute_hidden; > /* Should be called to setup first section data element if Looks good with that noinstall_HEADERS fix. Cheers, Mark