From patchwork Thu Oct 31 19:20:40 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Simon Marchi (Code Review)" X-Patchwork-Id: 35523 Received: (qmail 120421 invoked by alias); 31 Oct 2019 19:20:53 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 120345 invoked by uid 89); 31 Oct 2019 19:20:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=OLD X-HELO: mx1.osci.io X-Gerrit-PatchSet: 1 Date: Thu, 31 Oct 2019 15:20:40 -0400 From: "Florian Weimer (Code Review)" To: libc-alpha@sourceware.org Cc: Florian Weimer Message-ID: Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange Subject: [review] Avoid late dlopen failure due to scope, TLS slotinfo updates [BZ #25112] X-Gerrit-Change-Id: I240c58387dabda3ca1bcab48b02115175fa83d6c X-Gerrit-Change-Number: 468 X-Gerrit-ChangeURL: X-Gerrit-Commit: b48ed0a3be64f6522c1a20010db5941ce2b08dea References: Reply-To: fweimer@redhat.com, fweimer@redhat.com, libc-alpha@sourceware.org MIME-Version: 1.0 Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/468 ...................................................................... Avoid late dlopen failure due to scope, TLS slotinfo updates [BZ #25112] This change splits the scope and TLS slotinfo updates in dlopen into two parts: one to resize the data structures, and one to actually apply the update. The call to add_to_global_resize in dl_open_worker is moved before the demarcation point at which no further memory allocations are allowed. _dl_add_to_slotinfo is adjusted to make the list update optional. There is some optimization possibility here because we could grow the slotinfo list of arrays in a single call, one the largest TLS modid is known. This commit does not fix the fatal meory allocation failure in _dl_update_slotinfo. Ideally, this error during dlopen should be recoverable. The update order of scopes and TLS data structures is retained, although it appears to be more correct to fully initialize TLS first, and then expose symbols in the newly loaded objects via the scope update. Tested on x86_64-linux-gnu. Change-Id: I240c58387dabda3ca1bcab48b02115175fa83d6c Reviewed-by: Carlos O'Donell --- M elf/dl-open.c M elf/dl-tls.c M elf/rtld.c M sysdeps/generic/ldsodefs.h 4 files changed, 240 insertions(+), 128 deletions(-) diff --git a/elf/dl-open.c b/elf/dl-open.c index aad22ef..a305fef 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -214,6 +215,201 @@ } 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 }; + +/* 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) +{ + 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 + can be added later without further allocation of memory. This + function can raise an exceptions due to malloc failure. */ +static void +resize_scopes (struct link_map *new) +{ + /* If the file is not loaded now as a dependency, add the search + list of the newly loaded object to the scope. */ + 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; + + if (__glibc_unlikely (cnt + 1 >= imap->l_scope_max)) + { + /* The 'r_scope' array is too small. Allocate a new one + dynamically. */ + 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; + } + else + { + new_size = imap->l_scope_max * 2; + 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, + (cnt + 1) * sizeof (imap->l_scope[0])); + struct r_scope_elem **old = imap->l_scope; + + imap->l_scope = newp; + + if (old != imap->l_scope_mem) + _dl_scope_free (old); + + imap->l_scope_max = new_size; + } + } + } +} + +/* Second stage of resize_scopes: Add NEW to the scopes. Also print + debugging information about scopes if requested. + + This function cannot raise an exception because all required memory + has been allocated by a previous call to resize_scopes. */ +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; + + assert (cnt + 1 < imap->l_scope_max); + + /* 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. */ +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 + therefore is not yet initialized. */ + if (! imap->l_init_called && imap->l_tls_blocksize > 0) + { + _dl_add_to_slotinfo (imap, false); + any_tls = true; + } + } + return any_tls; +} + +/* Second stage of TLS update, after resize_tls_slotinfo. This + function does not raise any exception. It should only be called if + resize_tls_slotinfo returned true. */ +static void +update_tls_slotinfo (struct link_map *new) +{ + unsigned int first_static_tls = new->l_searchlist.r_nlist; + 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 + therefore is not yet initialized. */ + if (! imap->l_init_called && imap->l_tls_blocksize > 0) + { + _dl_add_to_slotinfo (imap, true); + + if (imap->l_need_tls_init + && first_static_tls == new->l_searchlist.r_nlist) + first_static_tls = i; + } + } + + if (__builtin_expect (++GL(dl_tls_generation) == 0, 0)) + _dl_fatal_printf (N_("\ +TLS generation counter wrapped! Please report this.")); + + /* We need a second pass for static tls data, because + _dl_update_slotinfo must not be run while calls to + _dl_add_to_slotinfo are still pending. */ + for (unsigned int i = first_static_tls; i < new->l_searchlist.r_nlist; ++i) + { + struct link_map *imap = new->l_searchlist.r_list[i]; + + if (imap->l_need_tls_init + && ! imap->l_init_called + && 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. */ + _dl_update_slotinfo (imap->l_tls_modid); +#endif + + GL(dl_init_static_tls) (imap); + assert (imap->l_need_tls_init == 0); + } + } +} + /* struct dl_init_args and call_dl_init are used to call _dl_init with exception handling disabled. */ struct dl_init_args @@ -431,133 +627,39 @@ relocation. */ _dl_open_check (new); - /* If the file is not loaded now as a dependency, add the search - list of the newly loaded object to the scope. */ - bool any_tls = false; - unsigned int first_static_tls = new->l_searchlist.r_nlist; - 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; + /* This only performs the memory allocations. The actual update of + the scopes happens below, after failure is impossible. */ + resize_scopes (new); - /* 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) - { - struct r_scope_elem **runp = imap->l_scope; - size_t cnt = 0; + /* Increase the size of the GL (dl_tls_dtv_slotinfo_list) data + structure. */ + bool any_tls = resize_tls_slotinfo (new); - while (*runp != NULL) - { - if (*runp == &new->l_searchlist) - break; - ++cnt; - ++runp; - } + /* Perform the necessary allocations for adding new global objects + to the global scope below. */ + if (mode & RTLD_GLOBAL) + add_to_global_resize (new); - if (*runp != NULL) - /* Avoid duplicates. */ - continue; + /* Demarcation point: After this, no recoverable errors are allowed. + All memory allocations for new objects must have happened + before. */ - if (__glibc_unlikely (cnt + 1 >= imap->l_scope_max)) - { - /* The 'r_scope' array is too small. Allocate a new one - dynamically. */ - size_t new_size; - struct r_scope_elem **newp; + /* Second stage after resize_scopes: Actually perform the scope + update. After this, dlsym and lazy binding can bind to new + objects. */ + update_scopes (new); -#define SCOPE_ELEMS(imap) \ - (sizeof (imap->l_scope_mem) / sizeof (imap->l_scope_mem[0])) + /* 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? */ - if (imap->l_scope != imap->l_scope_mem - && imap->l_scope_max < SCOPE_ELEMS (imap)) - { - new_size = SCOPE_ELEMS (imap); - newp = imap->l_scope_mem; - } - else - { - new_size = imap->l_scope_max * 2; - 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")); - } - - memcpy (newp, imap->l_scope, cnt * sizeof (imap->l_scope[0])); - struct r_scope_elem **old = imap->l_scope; - - imap->l_scope = newp; - - if (old != imap->l_scope_mem) - _dl_scope_free (old); - - imap->l_scope_max = new_size; - } - - /* 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; - - /* Print only new scope information. */ - from_scope = cnt; - } - /* Only add TLS memory if this object is loaded now and - therefore is not yet initialized. */ - else if (! imap->l_init_called - /* Only if the module defines thread local data. */ - && __builtin_expect (imap->l_tls_blocksize > 0, 0)) - { - /* Now that we know the object is loaded successfully add - modules containing TLS data to the slot info table. We - might have to increase its size. */ - _dl_add_to_slotinfo (imap); - - if (imap->l_need_tls_init - && first_static_tls == new->l_searchlist.r_nlist) - first_static_tls = i; - - /* We have to bump the generation counter. */ - any_tls = true; - } - - /* Print scope information. */ - if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES)) - _dl_show_scope (imap, from_scope); - } - - /* Bump the generation number if necessary. */ - if (any_tls && __builtin_expect (++GL(dl_tls_generation) == 0, 0)) - _dl_fatal_printf (N_("\ -TLS generation counter wrapped! Please report this.")); - - /* We need a second pass for static tls data, because _dl_update_slotinfo - must not be run while calls to _dl_add_to_slotinfo are still pending. */ - for (unsigned int i = first_static_tls; i < new->l_searchlist.r_nlist; ++i) - { - struct link_map *imap = new->l_searchlist.r_list[i]; - - if (imap->l_need_tls_init - && ! imap->l_init_called - && 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. */ - _dl_update_slotinfo (imap->l_tls_modid); -#endif - - GL(dl_init_static_tls) (imap); - assert (imap->l_need_tls_init == 0); - } - } + /* 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); /* Notify the debugger all new objects have been relocated. */ if (relocation_in_progress) diff --git a/elf/dl-tls.c b/elf/dl-tls.c index a4b0529..65d3520 100644 --- a/elf/dl-tls.c +++ b/elf/dl-tls.c @@ -883,7 +883,7 @@ void -_dl_add_to_slotinfo (struct link_map *l) +_dl_add_to_slotinfo (struct link_map *l, bool do_add) { /* Now that we know the object is loaded successfully add modules containing TLS data to the dtv info table. We @@ -939,6 +939,9 @@ } /* Add the information into the slotinfo data structure. */ - listp->slotinfo[idx].map = l; - listp->slotinfo[idx].gen = GL(dl_tls_generation) + 1; + if (do_add) + { + listp->slotinfo[idx].map = l; + listp->slotinfo[idx].gen = GL(dl_tls_generation) + 1; + } } diff --git a/elf/rtld.c b/elf/rtld.c index 8a6e1a1..0f185e7 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -2214,7 +2214,7 @@ /* Add object to slot information data if necessasy. */ if (l->l_tls_blocksize != 0 && tls_init_tp_called) - _dl_add_to_slotinfo (l); + _dl_add_to_slotinfo (l, true); } } else @@ -2259,7 +2259,7 @@ /* Add object to slot information data if necessasy. */ if (l->l_tls_blocksize != 0 && tls_init_tp_called) - _dl_add_to_slotinfo (l); + _dl_add_to_slotinfo (l, true); } rtld_timer_stop (&relocate_time, start); diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index ed0aed7..c546757 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -1140,8 +1140,15 @@ old scope, OLD can't be freed until no thread is using it. */ extern int _dl_scope_free (void *) attribute_hidden; -/* Add module to slot information data. */ -extern void _dl_add_to_slotinfo (struct link_map *l) attribute_hidden; + +/* Add module to slot information data. If DO_ADD is false, only the + required memory is allocated. Must be called with GL + (dl_load_lock) acquired. If the function has already been called + for the link map L with !do_add, then this function will not raise + an exception, otherwise it is possible that it encounters a memory + allocation failure. */ +extern void _dl_add_to_slotinfo (struct link_map *l, bool do_add) + attribute_hidden; /* Update slot information data for at least the generation of the module with the given index. */