[PATCH 1/2] init_array/fini_array cleanup

[PATCH 1/2] init_array/fini_array cleanup

[Szabolcs Nagy]

use correct types to access the array (not consistent with dynlink.c).
---
not sure if the declarations in dynlink.c can be changed as well
while keeping the end->start weak alias.

 src/env/__libc_start_main.c | 8 ++++----
 src/exit/exit.c             | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
index 5c79be2..f9c7ca5 100644
--- a/src/env/__libc_start_main.c
+++ b/src/env/__libc_start_main.c
@@ -12,7 +12,7 @@ static void dummy(void) {}
 weak_alias(dummy, _init);
 
 __attribute__((__weak__, __visibility__("hidden")))
-extern void (*const __init_array_start)(void), (*const __init_array_end)(void);
+extern void (*const __init_array_start[])(void), (*const __init_array_end[])(void);
 
 static void dummy1(void *p) {}
 weak_alias(dummy1, __init_ssp);
@@ -55,10 +55,10 @@ void __init_libc(char **envp, char *pn)
 
 static void libc_start_init(void)
 {
+	void (*const *f)(void);
 	_init();
-	uintptr_t a = (uintptr_t)&__init_array_start;
-	for (; a<(uintptr_t)&__init_array_end; a+=sizeof(void(*)()))
-		(*(void (**)())a)();
+	for (f=__init_array_start; f<__init_array_end; f++)
+		(*f)();
 }
 
 weak_alias(libc_start_init, __libc_start_init);
diff --git a/src/exit/exit.c b/src/exit/exit.c
index bf7835a..5c17290 100644
--- a/src/exit/exit.c
+++ b/src/exit/exit.c
@@ -13,13 +13,13 @@ weak_alias(dummy, __stdio_exit);
 weak_alias(dummy, _fini);
 
 __attribute__((__weak__, __visibility__("hidden")))
-extern void (*const __fini_array_start)(void), (*const __fini_array_end)(void);
+extern void (*const __fini_array_start[])(void), (*const __fini_array_end[])(void);
 
 static void libc_exit_fini(void)
 {
-	uintptr_t a = (uintptr_t)&__fini_array_end;
-	for (; a>(uintptr_t)&__fini_array_start; a-=sizeof(void(*)()))
-		(*(void (**)())(a-sizeof(void(*)())))();
+	void (*const *f)(void);
+	for (f=__fini_array_end; f>__fini_array_start; f--)
+		f[-1]();
 	_fini();
 }
 
-- 
2.4.1

Re: [PATCH 1/2] init_array/fini_array cleanup

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

* Szabolcs Nagy <nsz@port70.net> [2015-11-29 16:30:51 +0100]:
> -extern void (*const __init_array_start)(void), (*const __init_array_end)(void);
> +extern void (*const __init_array_start[])(void), (*const __init_array_end[])(void);

Alexander Monakov pointed out that the compiler may
consider arrays to be separate objects and then
f<__init_array_end would be ub.

attached an updated version, the same code is generated
on x86_64 as before, except there is no xor %eax,%eax
now before the call (because there is prototype).

[-- Attachment #2: 0001-cleaner-init-fini-array-handling.patch --]
[-- Type: text/x-diff, Size: 1515 bytes --]

From 3c29dc0d9bdb2e4d5f735d7b3d272d92cb82d2c6 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <nsz@port70.net>
Date: Sun, 29 Nov 2015 16:24:17 +0000
Subject: [PATCH 1/2] cleaner init/fini array handling

Avoid casts and make sure there is a prototype when init/fini
functions are called.
---
 src/env/__libc_start_main.c | 6 +++---
 src/exit/exit.c             | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
index 5c79be2..14a2825 100644
--- a/src/env/__libc_start_main.c
+++ b/src/env/__libc_start_main.c
@@ -55,10 +55,10 @@ void __init_libc(char **envp, char *pn)
 
 static void libc_start_init(void)
 {
+	void (*const *f)(void);
 	_init();
-	uintptr_t a = (uintptr_t)&__init_array_start;
-	for (; a<(uintptr_t)&__init_array_end; a+=sizeof(void(*)()))
-		(*(void (**)())a)();
+	for (f=&__init_array_start; f<&__init_array_end; f++)
+		(*f)();
 }
 
 weak_alias(libc_start_init, __libc_start_init);
diff --git a/src/exit/exit.c b/src/exit/exit.c
index bf7835a..b0603b4 100644
--- a/src/exit/exit.c
+++ b/src/exit/exit.c
@@ -17,9 +17,9 @@ extern void (*const __fini_array_start)(void), (*const __fini_array_end)(void);
 
 static void libc_exit_fini(void)
 {
-	uintptr_t a = (uintptr_t)&__fini_array_end;
-	for (; a>(uintptr_t)&__fini_array_start; a-=sizeof(void(*)()))
-		(*(void (**)())(a-sizeof(void(*)())))();
+	void (*const *f)(void) = &__fini_array_end;
+	while (f > &__fini_array_start)
+		(*--f)();
 	_fini();
 }
 
-- 
2.4.1

Re: [PATCH 1/2] init_array/fini_array cleanup

[Rich Felker]

On Sun, Nov 29, 2015 at 05:47:24PM +0100, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2015-11-29 16:30:51 +0100]:
> > -extern void (*const __init_array_start)(void), (*const __init_array_end)(void);
> > +extern void (*const __init_array_start[])(void), (*const __init_array_end[])(void);
> 
> Alexander Monakov pointed out that the compiler may
> consider arrays to be separate objects and then
> f<__init_array_end would be ub.
> 
> attached an updated version, the same code is generated
> on x86_64 as before, except there is no xor %eax,%eax
> now before the call (because there is prototype).

> >From 3c29dc0d9bdb2e4d5f735d7b3d272d92cb82d2c6 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@port70.net>
> Date: Sun, 29 Nov 2015 16:24:17 +0000
> Subject: [PATCH 1/2] cleaner init/fini array handling
> 
> Avoid casts and make sure there is a prototype when init/fini
> functions are called.
> ---
>  src/env/__libc_start_main.c | 6 +++---
>  src/exit/exit.c             | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
> index 5c79be2..14a2825 100644
> --- a/src/env/__libc_start_main.c
> +++ b/src/env/__libc_start_main.c
> @@ -55,10 +55,10 @@ void __init_libc(char **envp, char *pn)
>  
>  static void libc_start_init(void)
>  {
> +	void (*const *f)(void);
>  	_init();
> -	uintptr_t a = (uintptr_t)&__init_array_start;
> -	for (; a<(uintptr_t)&__init_array_end; a+=sizeof(void(*)()))
> -		(*(void (**)())a)();
> +	for (f=&__init_array_start; f<&__init_array_end; f++)
> +		(*f)();

This version still contains UB: since the initial f points to one
object (__init_array_start), the comparison against a different
object's address with < is UB, and after the first f++, all subsequent
f++ operations are also UB.

The original code was carefully written to avoid this by performing
all arithmetic in uintptr_t, but reportedly gcc and llvm even perform
origin analysis on integers, despite it being an invalid optimization,
so we may need to harden this further.

Using an array type (like your first patch) and using != instead of <
might be another suitable approach. Use of != would not be UB, and
since the result of equality comparisons between "one past the end" of
an array and a fixed other object must be consistent between multiple
ways of evaluating it, a compiler can't optimize out the !=. However,
this does not translate into a viable approach for the
opposite-direction iteration in the fini array.

I'm open to other ideas that are more robust against bad compiler
behavior.

Rich