Re: [PATCH v2 3/3] elf: Always call destructors in reverse constructor order

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 03/02/2022 12:18, Florian Weimer via Libc-alpha wrote:
> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
> index f841868cdb..ba53c2a4cb 100644
> --- a/elf/dl-fini.c
> +++ b/elf/dl-fini.c
> @@ -29,147 +29,87 @@ typedef void (*fini_t) (void);
>  void
>  _dl_fini (void)
>  {
> -  /* 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.  */
> -
> -  /* We run the destructors of the main namespaces last.  As for the
> -     other namespaces, we pick run the destructors in them in reverse
> -     order of the namespace ID.  */
> +  /* Call destructors strictly in the reverse order of constructors.
> +     This causes fewer surprises than some arbitrary reordering based
> +     on new (relocation) dependencies.  None of the objects are
> +     unmapped, so applications can deal with this if their DSOs remain
> +     in a consistent state after destructors have run.  */
> +
> +  /* Protect against concurrent loads and unloads.  */
> +  __rtld_lock_lock_recursive (GL(dl_load_lock));
> +
> +  /* Ignore objects which are opened during shutdown.  */
> +  struct link_map *local_init_called_list = _dl_init_called_list;
> +
> +  for (struct link_map *l = local_init_called_list; l != NULL;
> +       l = l->l_init_called_next)
> +      /* Bump l_direct_opencount of all objects so that they
> +	 are not dlclose()ed from underneath us.  */
> +      ++l->l_direct_opencount;
> +
> +  /* After this point, Everything linked from local_init_called_list
> +     cannot be unloaded because of the reference counter update.  */
> +  __rtld_lock_unlock_recursive (GL(dl_load_lock));
> +
> +  /* Perform two passes: One for non-audit modules, one for audit
> +     modules.  This way, audit modules receive unload notifications
> +     for non-audit objects, and the destructors for audit modules
> +     still run.  */
>  #ifdef SHARED
> -  int do_audit = 0;
> - again:
> +  int last_pass = GLRO(dl_naudit) > 0;
> +  Lmid_t last_ns = -1;	     /* Avoid some LA_ACT_DELETE callouts.  */

This has caused as lot of audit regressions, since it will make
_dl_audit_activity_nsid access

  struct link_map *head = GL(dl_ns)[-1]._ns_loaded;

> +  for (int do_audit = 0; do_audit <= last_pass; ++do_audit)
>  #endif
> -  for (Lmid_t ns = GL(dl_nns) - 1; ns >= 0; --ns)
> -    {
> -      /* Protect against concurrent loads and unloads.  */
> -      __rtld_lock_lock_recursive (GL(dl_load_lock));
> -
> -      unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded;
> -      /* No need to do anything for empty namespaces or those used for
> -	 auditing DSOs.  */
> -      if (nloaded == 0
> +    for (struct link_map *l = local_init_called_list; l != NULL;
> +	 l = l->l_init_called_next)
> +      {
>  #ifdef SHARED
> -	  || GL(dl_ns)[ns]._ns_loaded->l_auditing != do_audit
> -#endif
> -	  )
> -	__rtld_lock_unlock_recursive (GL(dl_load_lock));
> -      else
> -	{
> -#ifdef SHARED
> -	  _dl_audit_activity_nsid (ns, LA_ACT_DELETE);
> +	if (GL(dl_ns)[l->l_ns]._ns_loaded->l_auditing != do_audit)
> +	  continue;
> +
> +	if (last_ns != l->l_ns)
> +	  {

I think you will need a

   if (last_ns != -1)
     _dl_audit_activity_nsid (last_ns, LA_ACT_CONSISTENT);

> +	    _dl_audit_activity_nsid (last_ns, LA_ACT_CONSISTENT);
> +	    _dl_audit_activity_nsid (l->l_ns, LA_ACT_DELETE);
> +	    last_ns = l->l_ns;
> +	  }
>  #endif