[review v4] Avoid late dlopen failure due to scope, TLS slotinfo updates [BZ #25112]

["Florian Weimer (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

Florian Weimer has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/468
......................................................................


Patch Set 4:

(11 comments)

| --- elf/dl-open.c
| +++ elf/dl-open.c
| @@ -208,9 +209,26 @@ _dl_find_dso_for_object (const ElfW(Addr) addr)
|  	      || _dl_addr_inside_object (l, (ElfW(Addr)) addr)))
|  	{
|  	  assert (ns == l->l_ns);
|  	  return l;
|  	}
|    return NULL;
|  }
|  rtld_hidden_def (_dl_find_dso_for_object);
|  
| +/* Returned from scope_size if the new map was found in the existing
| +   scope.  */
| +enum { scope_size_found_new = (size_t) -1 };

PS3, Line 220:

Done

| +
| +/* Return the length of the scope for MAP.  If NEW->l_searchlist is
| +   found in the scope, return scope_size_found_new.  */
| +static size_t
| +scope_size (struct link_map *map, struct link_map *new)

PS3, Line 225:

I would be surprised if GCC can optimize this, but I've made the
change.

| +{
| +  size_t cnt;
| +  for (cnt = 0; map->l_scope[cnt] != NULL; ++cnt)
| +    if (map->l_scope[cnt] == &new->l_searchlist)
| +      return scope_size_found_new;
| +  return cnt;
| +}
| +
| +/* Resize the scopes of depended-upon objects, so that the new object

 ...

| @@ -217,0 +242,38 @@ resize_scopes (struct link_map *new)
| +  for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
| +    {
| +      struct link_map *imap = new->l_searchlist.r_list[i];
| +
| +      /* If the initializer has been called already, the object has
| +	 not been loaded here and now.  */
| +      if (imap->l_init_called && imap->l_type == lt_loaded)
| +	{
| +	  size_t cnt = scope_size (imap, new);
| +	  if (cnt == scope_size_found_new)
| +	    /* Avoid duplicates.  */
| +	    continue;

PS3, Line 253:

Done

| +
| +	  if (__glibc_unlikely (cnt + 1 >= imap->l_scope_max))
| +	    {
| +	      /* The 'r_scope' array is too small.  Allocate a new one
| +		 dynamically.  */

PS3, Line 258:

Done

| +	      size_t new_size;
| +	      struct r_scope_elem **newp;
| +
| +	      if (imap->l_scope != imap->l_scope_mem
| +		  && imap->l_scope_max < array_length (imap->l_scope_mem))
| +		{
| +		  new_size = array_length (imap->l_scope_mem);
| +		  newp = imap->l_scope_mem;

PS3, Line 266:

Done

| +		}
| +	      else
| +		{
| +		  new_size = imap->l_scope_max * 2;

PS3, Line 270:

I think this should be a separate change. It's unrelated to the
NODELETE bug.

| +		  newp = (struct r_scope_elem **)
| +		    malloc (new_size * sizeof (struct r_scope_elem *));
| +		  if (newp == NULL)
| +		    _dl_signal_error (ENOMEM, "dlopen", NULL,
| +				      N_("cannot create scope list"));
| +		}
| +
| +	      /* Copy the array and the terminating NULL.  */
| +	      memcpy (newp, imap->l_scope,

 ...

| @@ -217,0 +300,44 @@ static void
| +update_scopes (struct link_map *new)
| +{
| +  for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
| +    {
| +      struct link_map *imap = new->l_searchlist.r_list[i];
| +      int from_scope = 0;
| +
| +      if (imap->l_init_called && imap->l_type == lt_loaded)
| +	{
| +	  size_t cnt = scope_size (imap, new);
| +	  if (cnt == scope_size_found_new)
| +	    /* Avoid duplicates.  */
| +	    continue;

PS3, Line 312:

Done

| +
| +	  assert (cnt + 1 < imap->l_scope_max);

PS3, Line 314:

Done

| +
| +	  /* First terminate the extended list.  Otherwise a thread
| +	     might use the new last element and then use the garbage
| +	     at offset IDX+1.  */
| +	  imap->l_scope[cnt + 1] = NULL;
| +	  atomic_write_barrier ();
| +	  imap->l_scope[cnt] = &new->l_searchlist;
| +
| +	  from_scope = cnt;
| +	}
| +
| +      /* Print scope information.  */
| +      if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
| +	_dl_show_scope (imap, from_scope);
| +    }
| +}
| +
| +/* Call _dl_add_to_slotinfo with DO_ADD set to false, to allocate
| +   space in GL (dl_tls_dtv_slotinfo_list).  This can raise an
| +   exception.  */

PS3, Line 334:

Done

| +static bool
| +resize_tls_slotinfo (struct link_map *new)
| +{
| +  bool any_tls = false;
| +  for (unsigned int i = 0; i < new->l_searchlist.r_nlist; ++i)
| +    {
| +      struct link_map *imap = new->l_searchlist.r_list[i];
| +
| +      /* Only add TLS memory if this object is loaded now and

 ...

| @@ -217,0 +390,23 @@ TLS generation counter wrapped!  Please report this."));
| +	  && imap->l_tls_blocksize > 0)
| +	{
| +	  /* For static TLS we have to allocate the memory here and
| +	     now, but we can delay updating the DTV.  */
| +	  imap->l_need_tls_init = 0;
| +#ifdef SHARED
| +	  /* Update the slot information data for at least the
| +	     generation of the DSO we are allocating data for.  */
| +
| +	  /* FIXME: This can terminate the process on memory
| +	     allocation failure.  It is not possible to raise
| +	     exceptions from this context; to fix this bug,
| +	     _dl_update_slotinfo would have to be split into two
| +	     operations.  */

PS3, Line 403:

Done

| +	  _dl_update_slotinfo (imap->l_tls_modid);
| +#endif
| +
| +	  GL(dl_init_static_tls) (imap);
| +	  assert (imap->l_need_tls_init == 0);
| +	}
| +    }
| +}
| +

 ...

| @@ -555,18 +633,42 @@ #ifdef SHARED
| -	  /* Update the slot information data for at least the
| -	     generation of the DSO we are allocating data for.  */
| -	  _dl_update_slotinfo (imap->l_tls_modid);
| -#endif
| -
| -	  GL(dl_init_static_tls) (imap);
| -	  assert (imap->l_need_tls_init == 0);
| -	}
| -    }
| +  /* This only performs the memory allocations.  The actual update of
| +     the scopes happens below, after failure is impossible.  */
| +  resize_scopes (new);
| +
| +  /* Increase the size of the GL (dl_tls_dtv_slotinfo_list) data
| +     structure.  */
| +  bool any_tls = resize_tls_slotinfo (new);
| +
| +  /* Perform the necessary allocations for adding new global objects
| +     to the global scope below.  */
| +  if (mode & RTLD_GLOBAL)
| +    add_to_global_resize (new);
| +
| +  /* Demarcation point: After this, no recoverable errors are allowed.
| +     All memory allocations for new objects must have happened
| +     before.  */
| +
| +  /* Second stage after resize_scopes: Actually perform the scope
| +     update.  After this, dlsym and lazy binding can bind to new
| +     objects.  */
| +  update_scopes (new);
| +
| +  /* FIXME: It is unclear whether the order here is correct.
| +     Shouldn't new objects be made available for binding (and thus
| +     execution) only after there TLS data has been set up
| +     correctly?  */
| +
| +  /* Second stage after resize_tls_slotinfo: Update the slotinfo data
| +     structures.  */
| +  if (any_tls)
| +    /* FIXME: This calls _dl_update_slotinfo, which aborts the process
| +       on memory allocation failure.  */
| +    update_tls_slotinfo (new);

PS3, Line 665:

Done

|  
|    /* Notify the debugger all new objects have been relocated.  */
|    if (relocation_in_progress)
|      LIBC_PROBE (reloc_complete, 3, args->nsid, r, new);
|  
|  #ifndef SHARED
|    DL_STATIC_INIT (new);
|  #endif
|  

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I240c58387dabda3ca1bcab48b02115175fa83d6c
Gerrit-Change-Number: 468
Gerrit-PatchSet: 4
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 15:56:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: comment