mbox series

[0/2] Predictable ELF destructor ordering

Message ID cover.1639515239.git.fweimer@redhat.com
Headers show
Series Predictable ELF destructor ordering | expand

Message

Florian Weimer Dec. 14, 2021, 9:01 p.m. UTC
These patches remove the dependency sorting from dlclose and process
shutdown, so that destructor order is the reverse of constructor order
in more cases (always if the process does not call dlclose).

Tested on i686-linux-gnu and x86_64-linux-gnu.

I would like to include this in glibc 2.35 if possible, among the other
dependency sorting changes.

I believe this fixes bugs 15311 and 15903.

Florian Weimer (2):
  elf: Do not rely on relocation dependencies for destructor sorting
  elf: Always call destructors in reverse constructor order

 elf/dl-close.c             | 130 +++++++++++++---------
 elf/dl-deps.c              |   3 +-
 elf/dl-fini.c              | 216 ++++++++++++++-----------------------
 elf/dl-init.c              |  14 +++
 elf/dl-sort-maps.c         | 105 ++----------------
 elf/dso-sort-tests-1.def   |   6 +-
 include/link.h             |   4 +
 sysdeps/generic/ldsodefs.h |   6 +-
 8 files changed, 194 insertions(+), 290 deletions(-)


base-commit: 0884724a95b60452ad483dbe086d237d02ba624d

Comments

Stafford Horne Dec. 20, 2021, 12:31 p.m. UTC | #1
On Tue, Dec 14, 2021 at 10:01:57PM +0100, Florian Weimer via Libc-alpha wrote:
> These patches remove the dependency sorting from dlclose and process
> shutdown, so that destructor order is the reverse of constructor order
> in more cases (always if the process does not call dlclose).
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.
> 
> I would like to include this in glibc 2.35 if possible, among the other
> dependency sorting changes.
> 
> I believe this fixes bugs 15311 and 15903.
> 
> Florian Weimer (2):
>   elf: Do not rely on relocation dependencies for destructor sorting
>   elf: Always call destructors in reverse constructor order
> 
>  elf/dl-close.c             | 130 +++++++++++++---------
>  elf/dl-deps.c              |   3 +-
>  elf/dl-fini.c              | 216 ++++++++++++++-----------------------
>  elf/dl-init.c              |  14 +++
>  elf/dl-sort-maps.c         | 105 ++----------------
>  elf/dso-sort-tests-1.def   |   6 +-
>  include/link.h             |   4 +
>  sysdeps/generic/ldsodefs.h |   6 +-
>  8 files changed, 194 insertions(+), 290 deletions(-)

Hi Florian,

This does seem to fix an issue for me in the OpenRISC port.  Discussed [1] in
the OpenRISC port thread.  It fixes:

  elf/tst-bz15311

Failure details in tst-bz15311.out (fixed with these patches) [2].

However this also seems to break:

  elf/tst-glibc-hwcaps-prepend-cache

I am not able to get much detail from the failure other than a SIGDEGV in fork,
I then bisected it to these patches I have on my branch to fix elf/tst-bz15311.
Let me know if I can help.

-Stafford

[1] https://sourceware.org/pipermail/libc-alpha/2021-December/134184.html
[2] https://gist.github.com/5a5dacaeef1eac1f2f5d89701d14c0ad
Florian Weimer Dec. 20, 2021, 1:20 p.m. UTC | #2
* Stafford Horne:

> On Tue, Dec 14, 2021 at 10:01:57PM +0100, Florian Weimer via Libc-alpha wrote:
>> These patches remove the dependency sorting from dlclose and process
>> shutdown, so that destructor order is the reverse of constructor order
>> in more cases (always if the process does not call dlclose).
>> 
>> Tested on i686-linux-gnu and x86_64-linux-gnu.
>> 
>> I would like to include this in glibc 2.35 if possible, among the other
>> dependency sorting changes.
>> 
>> I believe this fixes bugs 15311 and 15903.
>> 
>> Florian Weimer (2):
>>   elf: Do not rely on relocation dependencies for destructor sorting
>>   elf: Always call destructors in reverse constructor order
>> 
>>  elf/dl-close.c             | 130 +++++++++++++---------
>>  elf/dl-deps.c              |   3 +-
>>  elf/dl-fini.c              | 216 ++++++++++++++-----------------------
>>  elf/dl-init.c              |  14 +++
>>  elf/dl-sort-maps.c         | 105 ++----------------
>>  elf/dso-sort-tests-1.def   |   6 +-
>>  include/link.h             |   4 +
>>  sysdeps/generic/ldsodefs.h |   6 +-
>>  8 files changed, 194 insertions(+), 290 deletions(-)
>
> Hi Florian,
>
> This does seem to fix an issue for me in the OpenRISC port.  Discussed [1] in
> the OpenRISC port thread.  It fixes:
>
>   elf/tst-bz15311
>
> Failure details in tst-bz15311.out (fixed with these patches) [2].
>
> However this also seems to break:
>
>   elf/tst-glibc-hwcaps-prepend-cache
>
> I am not able to get much detail from the failure other than a SIGDEGV
> in fork, I then bisected it to these patches I have on my branch to
> fix elf/tst-bz15311.  Let me know if I can help.

What are the DT_NEEDED entries on the shared objects, and the run-time
relocations?  I suspect you must have some unexpected dependency which
confuses the old sorting code.

The elf/tst-glibc-hwcaps-prepend-cache failure is very suspicious, it
really should not happen.  Would you be able to debug it further?

Thanks,
Florian
Stafford Horne Dec. 22, 2021, 12:38 a.m. UTC | #3
On Mon, Dec 20, 2021, 10:20 PM Florian Weimer <fweimer@redhat.com> wrote:

> * Stafford Horne:
>
> > On Tue, Dec 14, 2021 at 10:01:57PM +0100, Florian Weimer via Libc-alpha
> wrote:
> >> These patches remove the dependency sorting from dlclose and process
> >> shutdown, so that destructor order is the reverse of constructor order
> >> in more cases (always if the process does not call dlclose).
> >>
> >> Tested on i686-linux-gnu and x86_64-linux-gnu.
> >>
> >> I would like to include this in glibc 2.35 if possible, among the other
> >> dependency sorting changes.
> >>
> >> I believe this fixes bugs 15311 and 15903.
> >>
> >> Florian Weimer (2):
> >>   elf: Do not rely on relocation dependencies for destructor sorting
> >>   elf: Always call destructors in reverse constructor order
> >>
> >>  elf/dl-close.c             | 130 +++++++++++++---------
> >>  elf/dl-deps.c              |   3 +-
> >>  elf/dl-fini.c              | 216 ++++++++++++++-----------------------
> >>  elf/dl-init.c              |  14 +++
> >>  elf/dl-sort-maps.c         | 105 ++----------------
> >>  elf/dso-sort-tests-1.def   |   6 +-
> >>  include/link.h             |   4 +
> >>  sysdeps/generic/ldsodefs.h |   6 +-
> >>  8 files changed, 194 insertions(+), 290 deletions(-)
> >
> > Hi Florian,
> >
> > This does seem to fix an issue for me in the OpenRISC port.  Discussed
> [1] in
> > the OpenRISC port thread.  It fixes:
> >
> >   elf/tst-bz15311
> >
> > Failure details in tst-bz15311.out (fixed with these patches) [2].
> >
> > However this also seems to break:
> >
> >   elf/tst-glibc-hwcaps-prepend-cache
> >
> > I am not able to get much detail from the failure other than a SIGDEGV
> > in fork, I then bisected it to these patches I have on my branch to
> > fix elf/tst-bz15311.  Let me know if I can help.
>
> What are the DT_NEEDED entries on the shared objects, and the run-time
> relocations?  I suspect you must have some unexpected dependency which
> confuses the old sorting code.
>
> The elf/tst-glibc-hwcaps-prepend-cache failure is very suspicious, it
> really should not happen.  Would you be able to debug it further?
>


Hi Florian

After further debugging I found i cannot reproduce this after a full
toolchain rebuild.

I will leave this to a bad environment setup on my side. Sorry for the noise

-Stafford