[review v4] 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 4: Code-Review+2

(4 comments)

Still looks good.
OK for master.

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

| --- elf/dl-open.c
| +++ elf/dl-open.c
| @@ -87,7 +115,20 @@ add_to_global_resize (struct link_map *new)
| +    add_to_global_resize_failure (new);
| +
|    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:

Done

| +    }
| +  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:

Done

| +
| +      /* 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;
| --- elf/dl-open.c
| +++ elf/dl-open.c
| @@ -171,34 +208,34 @@ _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);
|  
|  /* struct dl_init_args and call_dl_init are used to call _dl_init with
|     exception handling disabled.  */
|  struct dl_init_args
|  {
|    struct link_map *new;
|    int argc;
|    char **argv;
|    char **env;
|  };
|  
|  static void
|  call_dl_init (void *closure)
|  {
|    struct dl_init_args *args = closure;
|    _dl_init (args->new, args->argc, args->argv, args->env);
|  }

PS4, Line 232:

OK. Call _dl_init with a closure.

|  
|  static void
|  dl_open_worker (void *a)
|  {
|    struct dl_open_args *args = a;
|    const char *file = args->file;
|    int mode = args->mode;
|    struct link_map *call_map = NULL;
|  

 ...

| @@ -525,25 +569,27 @@ #endif
|  #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.  Temporarily
|       disable the exception handler, so that lazy binding failures are
|       fatal.  */
|    {
|      struct dl_init_args init_args =
|        {
|          .new = new,
|          .argc = args->argc,
|          .argv = args->argv,
|          .env = args->env
|        };
|      _dl_catch_exception (NULL, call_dl_init, &init_args);
|    }

PS4, Line 590:

OK, run with _dl_catch_exception to it throws.

|  
|    /* 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);
|  

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ie08e2f318510d5a6a4bcb1c315f46791b5b77524
Gerrit-Change-Number: 465
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 18:39:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: comment