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

[Guillaume Berard <berardgui at gmail dot com>]

I dig a bit in the code to find a clean solution, and I found this comment:

  /* Lots of fun ahead.  We have to call the destructors for all still
     loaded objects, in all namespaces.  The problem is that the ELF
     specification now demands that dependencies between the modules
     are taken into account.  I.e., the destructor for a module is
     called before the ones for any of its dependencies.

     To make things more complicated, we cannot simply use the reverse
     order of the constructors.  Since the user might have loaded objects
     using `dlopen' there are possibly several other modules with its
     dependencies to be taken into account.  Therefore we have to start
     determining the order of the modules once again from the beginning.  */

So it seems that ELF specifications are not really working with the
initfirst specifications...
Just by curiosity, does anyone knows where I can find up-to-date ELF
specifications?
It could be a help to decide the behavior to adapt here.

Do you think initfirst flag should be prioritized over the
dependencies closure order?
It makes sense to me since it is the case for the init.

Guillaume.


On Sun, Sep 8, 2013 at 12:30 AM, Guillaume Berard <berardgui@gmail.com> wrote:
>
> Ok, here is an update of the patch to take in account only the
> first match of initfirst flag.
> Unfortunately, even with this change, if you have all your normal
> dependencies without initfirst and dlopened dependencies with initfirst,
> the first dlopened lib with initfirst will be init just after dlopen
> but fini at last.
> For example, lib1.so is linked with initfirst and lib2.so is not,
> I link my exec with lib2.so and run dlopen lib1.so in my exec.
> The LD_DEBUG output is:
>       2801:     calling init: ../lib2/lib2.so
>       2801:     calling init: ../lib1/lib1.so
>       2801:     calling fini: ./test [0]
>       2801:     calling fini: ../lib2/lib2.so [0]
>       2801:     calling fini: ../lib1/lib1.so [0]
> Therefore, this patch does not meet the specifications
> neither...
> I will try to create another patch soon in order to save the call
> order of init, I think this is the cleanest/saftest way to treat this issue.
> Also, Gl(dl_initfirst) would not be needed anymore for dl-fini.c.
>
>
> 2013-09-07  Guillaume Berard  <berardgui@gmail.com>
>
>         [BZ #15903]
>         * elf/dl-fini.c
>         * elf/dl-init.c
>         * elf/dl-load.c
>
> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
> index 6b245f0..40e66a3 100644
> --- a/elf/dl-fini.c
> +++ b/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;
> +        }
> +
>        /* 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 a/elf/dl-init.c b/elf/dl-init.c
> index a657eb6..6b76a4f 100644
> --- a/elf/dl-init.c
> +++ b/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.  */
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 6a73f27..7c8e7d5 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1571,7 +1571,7 @@ cannot enable executable stack as shared object requires");
>      }
>
>    /* Remember whether this object must be initialized first.  */
> -  if (l->l_flags_1 & DF_1_INITFIRST)
> +  if (l->l_flags_1 & DF_1_INITFIRST && GL(dl_initfirst) == NULL)
>      GL(dl_initfirst) = l;
>
>    /* Finally the file information.  */
>
>
>
>
> On Fri, 2013-09-06 at 13:08 -0500, Ryan S. Arnold wrote:
>> On Fri, Sep 6, 2013 at 7:33 AM, Guillaume Berard <berardgui@gmail.com>
> wrote:
>> > On Thu, Sep 5, 2013 at 7:04 PM, Ryan S. Arnold
> <ryan.arnold@gmail.com> wrote:
>> >> On Thu, Sep 5, 2013 at 8:40 AM, Guillaume Berard
> <berardgui@gmail.com> wrote:
>> >>> In my fix, if more than one is linked with initfirst, only the
> last
>> >>> library passed into the dl-open will be considered as the one to
> be
>> >>> init first, the others will be ignored and the init order will be
> as
>> >>> if the libraries do not have the initfirst flag. But this was
> already
>> >>> the case before.
>> >>> Having two libraries linked with initfirst sounds a bit
> contradictory.
>> >>
>> >> Right, when purposely doing this it doesn't make sense, but it is
> very
>> >> probable that a library with many dependencies might pull in a few
>> >> libraries that were linked with -Wl,-z,initfirst.  I think fixing
> this
>> >> will take a more thought-out proposal.  The patch will certainly be
> a
>> >> bit more invasive.
>> >>
>> >
>> > True, we this patch it is possible to "pollute" the init/fini order
> if you do
>> > not own all your dependencies...
>>
>> I think currently it doesn't conform to the documentation anyway.
>>
>> >
>> >>> We know we cannot guarantee multiple libraries to be init first.
> But
>> >>> maybe it would be better to guarantee that all initfirst go before
> all
>> >>> other inits (sounds more logical).
>> >>> I can try to provide another patch to support multiple initfirst
> later on.
>> >>
>> >> Sadly the _dl_initfirst pointer points to a link map structure
> pointer
>> >> which isn't a linked list of libraries marked initfirst.
>> >>
>> >> I think you're right to just focus on correcting the behavior and
>> >> making the first initfirst marked library 'fini' in the correct
> order
>> >> and dealing with the other issue in a separate patch.
>> >>
>> >> I'm curious whether your patch will work correctly in the following
> scenario:
>> >>
>> >> libfoo.so.0 and libbar.so.0 both being marked -Wl,-z,initfirst,
> where
>> >> libfoo.so.0 is an implicit dependency (automatically loaded), and
>> >> libbar.so.1 is a dlopened dependency.  Will libfoo.so.0 be
> initialized
>> >> first, but libbar.so.1 have fini called last (because it's dlopen
> will
>> >> override, and persist, the existing _dl_initfirst pointer from
>> >> libfoo.so.0)?
>> >>
>> >> I don't think the current code will work properly in this case
> either,
>> >> but your patch is a step in the right direction (by persisting the
>> >> _dl_initfirst pointer after _dl_init is called in elf/dl-init.c).
> I
>> >> think that in order to get the proper behavior you'll have to
> prevent
>> >> elf/dl-load.c:_dl_map_object_from_fd from setting the
>> >> GL(_dl_initfirst) pointer if it's already set.
>> >>
>> >
>> > Indeed! I think the best example is to have a normal link to a
> normal
>> > library and another one dlopened containing the initfirst flag. The
>> > dlopened one will be init last and fini last too :(. I did not think
> of such
>> > a case. I think the best way to fix this is to store the init order
>> > within a "GL" global variable and just revert this order for getting
>> > the fini order. This way would prevent additional treatment for the
>> > fini order sorting. I will update my changes.
>>
>> For a future patch, yes that would work.  For the current patch,
>> wouldn't simply disallowing a second initfirst setting accomplish what
>> you want?
>>
>> >
>> >> Are there any other implications of persisting the _dl_initfirst
>> >> pointer?  I do wonder why it was initially zeroed after _dl_init
> was
>> >> called.  Perhaps this was to eliminate redundant attempts at
>> >> initialization (which are benign since the load code won't load an
>> >> object twice).
>> >>
>> >
>> > I searched for a reason for this but did not find. Before calling
> init or
>> > fini, we always whether we already called init or not
> (l->l_init_called)
>> > and don't call it if not needed... But if I create a list containing
> the init
>> > order, I would not need to keep this pointer anymore.
>>
>> Which pointer, l->l_init_called or _dl_initfirst?
>>
>> > Do I need to start a new thread for proposing a new diff for this
> issue?
>>
>> If the scope changes and the subject of the current email is no longer
>> relevant sure.
>>
>> Ryan
>
>
>
>