[review v3] Avoid late failure in dlopen in global scope update [BZ #25112]

["Carlos O'Donell (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

Carlos O'Donell has posted comments on this change.

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


Patch Set 3: Code-Review+2

(14 comments)

This patch looks good to me as-is.

I added two unresolved comments where I would like to see some additional comments about the +8 and *2 factors, but they are entirely optional.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

| --- elf/dl-open.c
| +++ elf/dl-open.c
| @@ -48,13 +48,17 @@ struct dl_open_args
|    /* This is the caller of the dlopen() function.  */
|    const void *caller_dlopen;
|    struct link_map *map;
|    /* Namespace ID.  */
|    Lmid_t nsid;
| +
| +  /* Original value of _ns_global_scope_pending_adds.  Set by
| +     dl_open_worker.  Only valid if nsid is a real namespace
| +     (non-negative).  */
| +  unsigned int original_global_scope_pending_adds;

PS3, Line 57:

OK, we add a new entry to struct dl_open_args to track the pending
additions in recursive dlopens.

| +
|    /* Original parameters to the program and the current environment.  */
|    int argc;
|    char **argv;
|    char **env;
|  };
|  
| -
| -static int

 ...

| @@ -65,8 +65,24 @@ add_to_global (struct link_map *new)
| -  unsigned int cnt;
| +/* Called in case the global scope cannot be extended.  */
| +static void __attribute__ ((noreturn))
| +add_to_global_resize_failure (struct link_map *new)
| +{
| +  _dl_signal_error (ENOMEM, new->l_libname->name, NULL,
| +		    N_ ("cannot extend global scope"));
| +}
| +
| +/* Grow the global scope array for the namespace, so that all the new
| +   global objects can be added later in add_to_global_update, without
| +   risk of memory allocation failure.  add_to_global_resize raises
| +   exceptions for memory allocation errors.  */

PS3, Line 76:

OK. We are only preparing here and not yet at the commit phase where
we move forward with operations we cannot undo.

| +static void
| +add_to_global_resize (struct link_map *new)
| +{
| +  struct link_namespaces *ns = &GL (dl_ns)[new->l_ns];

PS3, Line 80:

OK. Get the current namespace. We use this later to do proper counting
of recursive dlopen additions since they will be namespace specific.

|  
|    /* Count the objects we have to put in the global scope.  */
| -  for (cnt = 0; cnt < new->l_searchlist.r_nlist; ++cnt)
| +  unsigned int to_add = 0;
| +  for (unsigned int cnt = 0; cnt < new->l_searchlist.r_nlist; ++cnt)
|      if (new->l_searchlist.r_list[cnt]->l_global == 0)
|        ++to_add;
|  
|    /* The symbols of the new objects and its dependencies are to be

 ...

| @@ -78,16 +94,41 @@ add_to_global_resize (struct link_map *new)
|       since this structure was allocated very early (before the libc
|       is loaded) the memory it uses is allocated by the malloc()-stub
|       in the ld.so.  When we come here these functions are not used
|       anymore.  Instead the malloc() implementation of the libc is
|       used.  But this means the block from the main map cannot be used
|       in an realloc() call.  Therefore we allocate a completely new
|       array the first time we have to add something to the locale scope.  */
|  
| -  struct link_namespaces *ns = &GL(dl_ns)[new->l_ns];
| +  if (__builtin_add_overflow (ns->_ns_global_scope_pending_adds, to_add,
| +			      &ns->_ns_global_scope_pending_adds))
| +    add_to_global_resize_failure (new);

PS3, Line 104:

OK. Check for overflow when adding to_add.

| +
| +  unsigned int new_size = 0; /* 0 means no new allocation.  */
| +  void *old_global = NULL; /* Old allocation if free-able.  */
| +
| +  /* Minimum required element count for resizing.  Adjusted below for
| +     an exponential resizing policy.  */
| +  size_t required_new_size;
| +  if (__builtin_add_overflow (ns->_ns_main_searchlist->r_nlist,
| +			      ns->_ns_global_scope_pending_adds,
| +			      &required_new_size))
| +    add_to_global_resize_failure (new);

PS3, Line 115:

OK. Again check that searchlist doesn't overflow.

| +
|    if (ns->_ns_global_scope_alloc == 0)
|      {
| -      /* This is the first dynamic object given global scope.  */
| -      ns->_ns_global_scope_alloc
| -	= ns->_ns_main_searchlist->r_nlist + to_add + 8;
| -      new_global = (struct link_map **)
| -	malloc (ns->_ns_global_scope_alloc * sizeof (struct link_map *));
| +      if (__builtin_add_overflow (required_new_size, 8, &new_size))
| +	add_to_global_resize_failure (new);

PS3, Line 120:

We add +8 to the initial allocation. Please add a comment to that
effect. The +8 is random and not picked for any reason that I know of.

| +    }
| +  else if (required_new_size > ns->_ns_global_scope_alloc)
| +    {
| +      if (__builtin_mul_overflow (required_new_size, 2, &new_size))
| +	add_to_global_resize_failure (new);

PS3, Line 125:

Please add a comment that we are using x2 as our growth factor for the
size, and that this case handles the growth. Indicate that x2 is just
a default that has not been tuned in any way. Maybe someone else can
see if x1.5 is better or not (we'd eventually reuse other
allocations).

| +
| +      /* The old array was allocated with our malloc, not the minimal
| +	 malloc.  */
| +      old_global = ns->_ns_main_searchlist->r_list;
| +    }
| +
| +  if (new_size > 0)
| +    {
| +      size_t allocation_size;

 ...

| @@ -121,13 +143,13 @@ add_to_global_resize (struct link_map *new)
| -	goto nomem;
| -
| -      memcpy (new_global, old_global,
| -	      ns->_ns_global_scope_alloc * sizeof (struct link_map *));
| -
| -      ns->_ns_global_scope_alloc = new_nalloc;
| +      memcpy (new_global, ns->_ns_main_searchlist->r_list,
| +	      ns->_ns_main_searchlist->r_nlist * sizeof (struct link_map *));
| +
| +      ns->_ns_global_scope_alloc = new_size;

PS3, Line 146:

OK, Record the new size.

|        ns->_ns_main_searchlist->r_list = new_global;
|  
|        if (!RTLD_SINGLE_THREAD_P)
|  	THREAD_GSCOPE_WAIT ();
|  
|        free (old_global);
|      }
| +}
| +

 ...

| @@ -138,14 +165,19 @@ add_to_global_update (struct link_map *new)
| +  for (unsigned int cnt = 0; cnt < new->l_searchlist.r_nlist; ++cnt)
|      {
|        struct link_map *map = new->l_searchlist.r_list[cnt];
|  
|        if (map->l_global == 0)
|  	{
|  	  map->l_global = 1;
| +
| +	  /* The array has been resized by add_to_global_resize.  */
| +	  assert (new_nlist < ns->_ns_global_scope_alloc);

PS3, Line 174:

OK. I like that you can assert this here.

| +
|  	  ns->_ns_main_searchlist->r_list[new_nlist++] = map;
|  
|  	  /* We modify the global scope.  Report this.  */
|  	  if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
|  	    _dl_debug_printf ("\nadd %s [%lu] to global scope\n",
|  			      map->l_name, map->l_ns);
|  	}
|      }

 ...

| @@ -204,15 +241,19 @@ dl_open_worker (void *a)
|        if (l)
|  	call_map = l;
|  
|        if (args->nsid == __LM_ID_CALLER)
|  	args->nsid = call_map->l_ns;
|      }
|  
| +  /* Retain the old value, so that it can be restored.  */
| +  args->original_global_scope_pending_adds
| +    = GL (dl_ns)[args->nsid]._ns_global_scope_pending_adds;

PS3, Line 250:

OK. We will need this again to return as we return from the recursive
dlopen. We are in dl_open_worker here, so this is the entry point for
doing all the work.

| +
|    /* One might be tempted to assert that we are RT_CONSISTENT at this point, but that
|       may not be true if this is a recursive call to dlopen.  */
|    _dl_debug_initialize (0, args->nsid);
|  
|    /* Load the named object.  */
|    struct link_map *new;
|    args->map = new = _dl_map_object (call_map, file, lt_loaded, 0,
|  				    mode | __RTLD_CALLMAP, args->nsid);

 ...

| @@ -244,18 +285,21 @@ dl_open_worker (void *a)
|        /* Let the user know about the opencount.  */
|        if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
|  	_dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",
|  			  new->l_name, new->l_ns, new->l_direct_opencount);
|  
|        /* If the user requested the object to be in the global namespace
|  	 but it is not so far, add it now.  */
|        if ((mode & RTLD_GLOBAL) && new->l_global == 0)
| -	(void) add_to_global (new);
| +	{
| +	  add_to_global_resize (new);
| +	  add_to_global_update (new);
| +	}

PS3, Line 296:

OK. This is the already open object case, so we must redo the sizing
and update if we are promoting an object from local to global scope.

|  
|        assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
|  
|        return;
|      }
|  
|    /* Load that object's dependencies.  */
|    _dl_map_object_deps (new, NULL, 0, 0,
|  		       mode & (__RTLD_DLOPEN | RTLD_DEEPBIND | __RTLD_AUDIT));

 ...

| @@ -505,25 +549,27 @@ #endif
|    if (relocation_in_progress)
|      LIBC_PROBE (reloc_complete, 3, args->nsid, r, new);
|  
|  #ifndef SHARED
|    DL_STATIC_INIT (new);
|  #endif
|  
| +  /* Perform the necessary allocations for adding new global objects
| +     to the global scope below, via add_to_global_update.  */
| +  if (mode & RTLD_GLOBAL)
| +    add_to_global_resize (new);
| +
|    /* Run the initializer functions of new objects.  */
|    _dl_init (new, args->argc, args->argv, args->env);
|  
|    /* Now we can make the new map available in the global scope.  */
|    if (mode & RTLD_GLOBAL)
| -    /* Move the object in the global namespace.  */
| -    if (add_to_global (new) != 0)
| -      /* It failed.  */
| -      return;
| +    add_to_global_update (new);

PS3, Line 566:

OK. This is the normal case if we haven't already been loaded. We
resize, then run the initializers, then add to global scope. We will
fail via throwing an error in add_to_global_resize_failure in
add_to_global_resize *before* running the initializers, so that's
good.

|  
|  #ifndef SHARED
|    /* We must be the static _dl_open in libc.a.  A static program that
|       has loaded a dynamic object now has competition.  */
|    __libc_multiple_libcs = 1;
|  #endif
|  
|    /* Let the user know about the opencount.  */
|    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))

 ...

| @@ -602,8 +647,21 @@ #endif
| +  /* Do this for both the error and success cases.  The old value has
| +     only been determined if the namespace ID was assigned (i.e., it
| +     is not __LM_ID_CALLER).  In the success case, we actually may
| +     have consumed more pending adds than planned (because the local
| +     scopes overlap in case of a recursive dlopen, the inner dlopen
| +     doing some of the globalization work of the outer dlopen), so the
| +     old pending adds value is larger than absolutely necessary.
| +     Since it is just a conservative upper bound, this is harmless.
| +     The top-level dlopen call will restore the field to zero.  */
| +  if (args.nsid >= 0)
| +    GL (dl_ns)[args.nsid]._ns_global_scope_pending_adds
| +      = args.original_global_scope_pending_adds;

PS3, Line 658:

OK.

| +
|    /* See if an error occurred during loading.  */
|    if (__glibc_unlikely (exception.errstring != NULL))
|      {
|        /* Remove the object from memory.  It may be in an inconsistent
|  	 state if relocation failed, for example.  */
|        if (args.map)
|  	{
|  	  /* Maybe some of the modules which were loaded use TLS.
| --- sysdeps/generic/ldsodefs.h
| +++ sysdeps/generic/ldsodefs.h
| @@ -325,15 +325,22 @@ #endif
|      unsigned int _ns_nloaded;
|      /* Direct pointer to the searchlist of the main object.  */
|      struct r_scope_elem *_ns_main_searchlist;
|      /* This is zero at program start to signal that the global scope map is
|         allocated by rtld.  Later it keeps the size of the map.  It might be
|         reset if in _dl_close if the last global object is removed.  */
| -    size_t _ns_global_scope_alloc;
| +    unsigned int _ns_global_scope_alloc;
| +
| +    /* During dlopen, this is the number of objects that still need to
| +       be added to the global scope map.  It has to be taken into
| +       account when resizing the map, for future map additions after
| +       recursive dlopen calls from ELF constructors.  */
| +    unsigned int _ns_global_scope_pending_adds;

PS3, Line 337:

OK.

| +
|      /* Search table for unique objects.  */
|      struct unique_sym_table
|      {
|        __rtld_lock_define_recursive (, lock)
|        struct unique_sym
|        {
|  	uint32_t hashval;
|  	const char *name;

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ie08e2f318510d5a6a4bcb1c315f46791b5b77524
Gerrit-Change-Number: 465
Gerrit-PatchSet: 3
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: Fri, 15 Nov 2019 22:19:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment