Re: [PATCH] [BZ#15903] INITFIRST flag does not change fini order (ld.so)

["Ryan S. Arnold" <ryan dot arnold at gmail dot com>]

On Mon, Sep 2, 2013 at 2:02 PM, Guillaume Berard <berardgui@gmail.com> wrote:
> Hi,
>
> According to https://sourceware.org/binutils/docs/ld/Options.html
> ld.so should call fini for the linked libraries in the reverse order
> than the init order.
> However, if I build two libraries, lib1 and lib2, lib1 with -z
> initfirst and I link an executable with these two libraries, the fini
> order in not the reverse order of init (checked with LD_DEBUG).
> I changed a bit fl-fini.c in order to take in account the initfirst
> flag for sorting the fini order.

This certainly is what the expectation that the binutils documentation
broadcasts.

As a curiosity, how does your patch behave if more than one dependent
library is linked with initfirst?  As far as I can tell, the
documentation isn't clear on what might be expected.  I suspect that
one might expect that link order would take precedence.

>
> [BZ#15903]
> * elf/dl-fini.c
> * elf/dl-init.c

For future reference, your ChangeLog isn't properly formatted:

Reference: https://sourceware.org/glibc/wiki/Contribution%20checklist#Properly_Formatted_GNU_ChangeLog

>
> diff --git elf/dl-fini.c elf/dl-fini.c
> index 6b245f0..56f17c9 100644
> --- elf/dl-fini.c
> +++ elf/dl-fini.c
> @@ -39,6 +39,10 @@ _dl_sort_fini (struct link_map **maps, size_t
> nmaps, char *used, Lmid_t ns)
>    unsigned int i = ns == LM_ID_BASE;
>    uint16_t seen[nmaps];
>    memset (seen, 0, nmaps * sizeof (seen[0]));
> +
> +  /* Maximum index for sorting */
> +  size_t nmax = nmaps - 1;
> +
>    while (1)
>      {
>        /* Keep track of which object we looked at this round.  */
> @@ -50,10 +54,19 @@ _dl_sort_fini (struct link_map **maps, size_t
> nmaps, char *used, Lmid_t ns)
>        if (thisp != thisp->l_real || thisp->l_idx == -1)
>   goto skip;
>
> +      /* If initfirst flag is set, push the library at last. */
> +      if (__builtin_expect (thisp == GL(dl_initfirst), 0))
> + {
> +  maps[i] = maps[nmax];
> +  maps[nmax] = thisp;
> +  thisp = maps[i];
> +  --nmax;
> + }
> +

I think your indentation is all messed up here.

>        /* Find the last object in the list for which the current one is
>   a dependency and move the current object behind the object
>   with the dependency.  */
> -      unsigned int k = nmaps - 1;
> +      unsigned int k = nmax;
>        while (k > i)
>   {
>    struct link_map **runp = maps[k]->l_initfini;
> diff --git elf/dl-init.c elf/dl-init.c
> index a657eb6..6b76a4f 100644
> --- elf/dl-init.c
> +++ elf/dl-init.c
> @@ -97,7 +97,6 @@ _dl_init (struct link_map *main_map, int argc, char
> **argv, char **env)
>    if (__builtin_expect (GL(dl_initfirst) != NULL, 0))
>      {
>        call_init (GL(dl_initfirst), argc, argv, env);
> -      GL(dl_initfirst) = NULL;
>      }
>
>    /* Don't do anything if there is no preinit array.  */

I'll have to try this out before I can Ack the patch.

This patch doesn't satisfy the criteria for being legally significant
so it shouldn't need you to have FSF copyright assignment on file.

Ryan