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

[Guillaume Berard <berardgui at gmail dot com>]

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