[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20151129234309.GS3818@brightrain.aerifal.cx>
Date: Sun, 29 Nov 2015 18:43:09 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 2/2] add preinit_array support
On Sun, Nov 29, 2015 at 05:59:25PM +0100, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@...t70.net> [2015-11-29 16:33:22 +0100]:
> > redefined DYN_CNT because 32 is not enough for DT_PREINIT_ARRAY.
> > ---
> > src/env/__libc_start_main.c | 5 +++++
> > src/ldso/dynlink.c | 12 ++++++++++--
> > 2 files changed, 15 insertions(+), 2 deletions(-)
> >
>
> src/internal change was missing and legacy _init vs
> preinit_array order was wrong.
>
> attached new version, array type is fixed too.
> >From 8820faea5a365387c8fe54c8ffa1f796c2084f83 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@...t70.net>
> Date: Sun, 29 Nov 2015 16:36:24 +0000
> Subject: [PATCH 2/2] add preinit_array support
>
> handle DT_PREINIT_ARRAY{SZ} in case of dynamic linking and
> __preinit_array_{start,end} in case of static linking.
Omission of preinit array support was intentional since it's reserved
for use by the [library] implementation and musl does not use it, and
I've tried to keep mandatory-linked startup code as small as possible
to keep the small-static-binaries people happy.
However on IRC we discussed that ASan and VTV and perhaps other
similar tools might use it, and such use makes sense, so we may need
to add it. I just don't want to get in a habit of adding whatever
random junk is in the ELF spec when there's no reasonable,
well-defined way for application code to use or depend on it.
> redefined DYN_CNT because 32 is not enough for DT_PREINIT_ARRAY.
This should be reviewed for possible problems; IIRC I reduced it to 32
to avoid having to worry about undefined bitshifts in some code that
can't do 64-bit shifts (because libgcc functions might not yet be
callable) but that code should really hard-code 32 rather than using
DYN_CNT if it's still a problem.
> ---
> src/env/__libc_start_main.c | 5 +++++
> src/internal/dynlink.h | 2 +-
> src/ldso/dynlink.c | 12 ++++++++++--
> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
> index 14a2825..7502bc3 100644
> --- a/src/env/__libc_start_main.c
> +++ b/src/env/__libc_start_main.c
> @@ -12,6 +12,9 @@ static void dummy(void) {}
> weak_alias(dummy, _init);
>
> __attribute__((__weak__, __visibility__("hidden")))
> +extern void (*const __preinit_array_start)(void), (*const __preinit_array_end)(void);
> +
> +__attribute__((__weak__, __visibility__("hidden")))
> extern void (*const __init_array_start)(void), (*const __init_array_end)(void);
>
> static void dummy1(void *p) {}
> @@ -56,6 +59,8 @@ void __init_libc(char **envp, char *pn)
> static void libc_start_init(void)
> {
> void (*const *f)(void);
> + for (f=&__preinit_array_start; f<&__preinit_array_end; f++)
> + (*f)();
> _init();
> for (f=&__init_array_start; f<&__init_array_end; f++)
> (*f)();
> diff --git a/src/internal/dynlink.h b/src/internal/dynlink.h
> index 9c494e4..fb706d3 100644
> --- a/src/internal/dynlink.h
> +++ b/src/internal/dynlink.h
> @@ -84,7 +84,7 @@ struct fdpic_dummy_loadmap {
> #endif
>
> #define AUX_CNT 32
> -#define DYN_CNT 32
> +#define DYN_CNT DT_NUM
DT_NUM is probably excessive and just increases stack usage with no
benefit.
> typedef void (*stage2_func)(unsigned char *, size_t *);
> typedef _Noreturn void (*stage3_func)(size_t *);
> diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
> index 93e7d67..51da820 100644
> --- a/src/ldso/dynlink.c
> +++ b/src/ldso/dynlink.c
> @@ -135,11 +135,14 @@ static struct fdpic_dummy_loadmap app_dummy_loadmap;
> struct debug *_dl_debug_addr = &debug;
>
> __attribute__((__visibility__("hidden")))
> -void (*const __init_array_start)(void)=0, (*const __fini_array_start)(void)=0;
> +void (*const __init_array_start)(void)=0, (*const __fini_array_start)(void)=0,
> +(*const __preinit_array_start)(void)=0;
>
> __attribute__((__visibility__("hidden")))
> -extern void (*const __init_array_end)(void), (*const __fini_array_end)(void);
> +extern void (*const __init_array_end)(void), (*const __fini_array_end)(void),
> +(*const __preinit_array_end)(void);
>
> +weak_alias(__preinit_array_start, __preinit_array_end);
> weak_alias(__init_array_start, __init_array_end);
> weak_alias(__fini_array_start, __fini_array_end);
>
> @@ -1225,6 +1228,11 @@ static void do_init_fini(struct dso *p)
> p->fini_next = fini_head;
> fini_head = p;
> }
> + if (dyn[0] & (1UL<<DT_PREINIT_ARRAY)) {
This invokes UB on 32-bit archs, and couldn't possibly work anyway
since the bit simply doesn't exist. Probably the test just needs to
be "if (dyn[DT_PREINIT_ARRAYSZ])" or similar. Alternatively,
search_vec could be used.
Further, I don't think having this code inside the do_init_fini loop
makes sense -- that loop runs backwards starting from the last-loaded
library, whereas preinit arrays are only valid in the main program (at
least according to ld, IIRC) and must run before any normal init
arrays or legacy _init functions (for any DSOs) are run.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
