From 620cf538206fe0f8cd63d76c502149b331f56f51 Mon Sep 17 00:00:00 2001 From: monojenkins Date: Tue, 25 Aug 2020 13:02:57 -0400 Subject: [PATCH] [2020-02] Avoid following invalid pointers in mono_w32process_get_modules on Darwin. (#20297) * Avoid following invalid pointers in mono_w32process_get_modules on Darwin. `_dyld_get_image_name` and `_dyld_get_image_header` can return pointers to data that will be freed or unmapped by concurrent calls to `dlclose`. Avoid this by using `_dyld_register_func_for_add_image` and `_dyld_register_func_for_remove_image` to install callbacks that maintain a runtime-internal set of loaded dynamic libraries. Some notes: - dyld doesn't provide a public "app store friendly" API (like `dl_iterate_phdr`) to traverse a snapshot of all loaded dynamic libraries in a process. There are two alternative private APIs that do this: one uses `_dyld_process_info_create` and friends and is present in macOS 10.12 and above, and the other depends on `_dyld_get_all_image_infos`, which is documented by Apple (... in a WWDC video) to "go away" at some point in dyld3. - `dladdr` linearly scans the dyld loaded image array (and each image's segment set) for a matching address, so `image_added` will get slower as more dynamic libraries are loaded in the process. But dyld3's implementation of dlopen already scales linearly in time with the number of loaded libraries. The use of `dladdr` can be avoided by using `_dyld_register_for_image_loads` or `_dyld_register_for_bulk_image_loads`, but those are both private APIs. - The callbacks registered with `_dyld_register_func_for_add_image` and `_dyld_register_func_for_remove_image` can never be unregistered, so this is yet another thing that makes it impossible to safely unload an embedded mono. - `MonoW32ProcessModule::inode` only seems to be used to avoid dupicates in other implementations of w32process; sorting the returned module list in `mono_w32process_get_modules` wouldn't be necessary if the `mono_dyld_image_info`s were also kept in an intrusive circular doubly linked list. - It looks like `mono_w32process_get_modules` in `w32process-unix-bsd.c` has a similar race condition. Also see: https://github.com/mono/mono/issues/20107 * Formatting. * Make `image_added` and `image_removed` non-blocking. It is theoretically possible for `mono_w32process_get_modules` to cause a deadlock if either `image_added` or `image_removed` is waiting for the image table mutex to be released and if `g_new0`, `g_strdup`, `g_hash_table_iter_next`, or any of the symbols they depend on are lazily bound and haven't already been resolved. Only one thread may iterate over the image table and only one thread may add/remove entries to/from the image table. The writer thread attemps to lock a mutex to block readers from iterating over the table. If the writer successfully acquires this lock, then no readers are concurrently iterating over the image table and it is safe to eagerly free stale data. If the writer doesn't successfully acquire this lock, then the reader thread is running concurrently with the writer; if this happens, the writer pushes stale data into a limbo list. Readers opportunistically clear this list. Use `mono_mutex_t` instead of `MonoCoopMutex`. Hand-roll the necessary GC state transitions inside `mono_w32process_get_modules`. Add `mono_conc_hashtable_foreach_snapshot` to mono-conc-hashtable.{h,c}. `MonoConcurrentHashTable` depends on the hazard pointer table (`hazard_table` inside hazard-pointer.c), which is initialized lazily on the first call to `mono_thread_small_id_alloc`. Perhaps this should be explicitly initialized early during `mono_init_internal`. * Revert "Make `image_added` and `image_removed` non-blocking." This reverts commit a22a4440df5f0f13da29b1b90e80a3420223c2b9. * Use mono_mutex_t instead of MonoCoopMutex * Explicitly load symbols not used by `image_added` before `mono_w32process_get_modules` runs Co-authored-by: Imran Hameed --- mono/metadata/w32process-unix-bsd.c | 5 + mono/metadata/w32process-unix-default.c | 5 + mono/metadata/w32process-unix-haiku.c | 5 + mono/metadata/w32process-unix-internals.h | 3 + mono/metadata/w32process-unix-osx.c | 153 ++++++++++++++++++++++-------- mono/metadata/w32process-unix.c | 4 + 6 files changed, 134 insertions(+), 41 deletions(-) diff --git a/mono/metadata/w32process-unix-bsd.c b/mono/metadata/w32process-unix-bsd.c index d09bc9f506e..e368c19bd8b 100644 --- a/mono/metadata/w32process-unix-bsd.c +++ b/mono/metadata/w32process-unix-bsd.c @@ -175,6 +175,11 @@ mono_w32process_get_modules (pid_t pid) return g_slist_reverse (ret); } +void +mono_w32process_platform_init_once (void) +{ +} + #else MONO_EMPTY_SOURCE_FILE (w32process_unix_bsd); diff --git a/mono/metadata/w32process-unix-default.c b/mono/metadata/w32process-unix-default.c index e2fda17fb5a..0f148aec286 100644 --- a/mono/metadata/w32process-unix-default.c +++ b/mono/metadata/w32process-unix-default.c @@ -360,4 +360,9 @@ mono_w32process_get_modules (pid_t pid) #endif } +void +mono_w32process_platform_init_once (void) +{ +} + #endif diff --git a/mono/metadata/w32process-unix-haiku.c b/mono/metadata/w32process-unix-haiku.c index 0b3c82436c7..dbd312c8d86 100644 --- a/mono/metadata/w32process-unix-haiku.c +++ b/mono/metadata/w32process-unix-haiku.c @@ -57,6 +57,11 @@ mono_w32process_get_modules (pid_t pid) return g_slist_reverse (ret); } +void +mono_w32process_platform_init_once (void) +{ +} + #else MONO_EMPTY_SOURCE_FILE (w32process_unix_haiku); diff --git a/mono/metadata/w32process-unix-internals.h b/mono/metadata/w32process-unix-internals.h index 7ba5f7e002e..056177dd22a 100644 --- a/mono/metadata/w32process-unix-internals.h +++ b/mono/metadata/w32process-unix-internals.h @@ -40,6 +40,9 @@ mono_w32process_get_name (pid_t pid); GSList* mono_w32process_get_modules (pid_t pid); +void +mono_w32process_platform_init_once (void); + static void G_GNUC_UNUSED mono_w32process_module_free (MonoW32ProcessModule *module) { diff --git a/mono/metadata/w32process-unix-osx.c b/mono/metadata/w32process-unix-osx.c index c687f6a70a1..2568a44223e 100644 --- a/mono/metadata/w32process-unix-osx.c +++ b/mono/metadata/w32process-unix-osx.c @@ -15,6 +15,7 @@ #include #include #include +#include /* sys/resource.h (for rusage) is required when using osx 10.3 (but not 10.4) */ #ifdef __APPLE__ @@ -117,58 +118,128 @@ mono_w32process_get_path (pid_t pid) #endif } -GSList* +struct mono_dyld_image_info +{ + const void *header_addr; + const void *data_section_start; + const void *data_section_end; + const char *name; + guint64 order; +}; + +static guint64 dyld_order = 0; +static GHashTable *images; +static mono_mutex_t images_mutex; + +static int +sort_modules_by_load_order (gconstpointer a, gconstpointer b) +{ + MonoW32ProcessModule *ma = (MonoW32ProcessModule *) a; + MonoW32ProcessModule *mb = (MonoW32ProcessModule *) b; + return ma->inode == mb->inode ? 0 : ma->inode < mb->inode ? -1 : 1; +} + +GSList * mono_w32process_get_modules (pid_t pid) { GSList *ret = NULL; - MonoW32ProcessModule *mod; - guint32 count; - int i = 0; - + MONO_ENTER_GC_SAFE; if (pid != getpid ()) - return NULL; + goto done; - count = _dyld_image_count (); - for (i = 0; i < count; i++) { -#if SIZEOF_VOID_P == 8 - const struct mach_header_64 *hdr; - const struct section_64 *sec; -#else - const struct mach_header *hdr; - const struct section *sec; -#endif - const char *name; - - name = _dyld_get_image_name (i); -#if SIZEOF_VOID_P == 8 - hdr = (const struct mach_header_64*)_dyld_get_image_header (i); - sec = getsectbynamefromheader_64 (hdr, SEG_DATA, SECT_DATA); -#else - hdr = _dyld_get_image_header (i); - sec = getsectbynamefromheader (hdr, SEG_DATA, SECT_DATA); -#endif + GHashTableIter it; + g_hash_table_iter_init (&it, images); - /* Some dynlibs do not have data sections on osx (#533893) */ - if (sec == 0) - continue; + gpointer val; - mod = g_new0 (MonoW32ProcessModule, 1); - mod->address_start = GINT_TO_POINTER (sec->addr); - mod->address_end = GINT_TO_POINTER (sec->addr+sec->size); + mono_os_mutex_lock (&images_mutex); + while (g_hash_table_iter_next (&it, NULL, &val)) { + struct mono_dyld_image_info *info = (struct mono_dyld_image_info *) val; + MonoW32ProcessModule *mod = g_new0 (MonoW32ProcessModule, 1); + mod->address_start = GINT_TO_POINTER (info->data_section_start); + mod->address_end = GINT_TO_POINTER (info->data_section_end); mod->perms = g_strdup ("r--p"); mod->address_offset = 0; - mod->device = makedev (0, 0); - mod->inode = i; - mod->filename = g_strdup (name); - - if (g_slist_find_custom (ret, mod, mono_w32process_module_equals) == NULL) { - ret = g_slist_prepend (ret, mod); - } else { - mono_w32process_module_free (mod); - } + mod->device = 0; + mod->inode = info->order; + mod->filename = g_strdup (info->name); + ret = g_slist_prepend (ret, mod); } + mono_os_mutex_unlock (&images_mutex); + ret = g_slist_sort (ret, &sort_modules_by_load_order); +done: + MONO_EXIT_GC_SAFE; + return ret; +} + +static void +mono_dyld_image_info_free (void *info) +{ + struct mono_dyld_image_info *dinfo = (struct mono_dyld_image_info *) info; + g_free ((void *) dinfo->name); + g_free (dinfo); +} + +static void +image_added (const struct mach_header *hdr32, intptr_t vmaddr_slide) +{ + #if SIZEOF_VOID_P == 8 + const struct mach_header_64 *hdr64 = (const struct mach_header_64 *)hdr32; + const struct section_64 *sec = getsectbynamefromheader_64 (hdr64, SEG_DATA, SECT_DATA); + #else + const struct section *sec = getsectbynamefromheader (hdr32, SEG_DATA, SECT_DATA); + #endif + Dl_info dlinfo; + if (!dladdr (hdr32, &dlinfo)) return; + if (sec == NULL) return; + + mono_os_mutex_lock (&images_mutex); + gpointer found = g_hash_table_lookup (images, (gpointer) hdr32); + mono_os_mutex_unlock (&images_mutex); + + if (found == NULL) { + struct mono_dyld_image_info *info = g_new0 (struct mono_dyld_image_info, 1); + info->header_addr = hdr32; + info->data_section_start = GINT_TO_POINTER (sec->addr); + info->data_section_end = GINT_TO_POINTER (sec->addr + sec->size); + info->name = g_strdup (dlinfo.dli_fname); + info->order = dyld_order; + ++dyld_order; + + mono_os_mutex_lock (&images_mutex); + g_hash_table_insert (images, (gpointer) hdr32, info); + mono_os_mutex_unlock (&images_mutex); + } +} + +static void +image_removed (const struct mach_header *hdr32, intptr_t vmaddr_slide) +{ + mono_os_mutex_lock (&images_mutex); + g_hash_table_remove (images, hdr32); + mono_os_mutex_unlock (&images_mutex); +} - return g_slist_reverse (ret); +void +mono_w32process_platform_init_once (void) +{ + mono_os_mutex_init (&images_mutex); + images = g_hash_table_new_full (NULL, NULL, NULL, &mono_dyld_image_info_free); + + /* Ensure that the functions used within the lock-protected region in + * mono_w32process_get_modules have been loaded, in case these symbols + * are lazily bound. g_new0 and g_strdup will be called by + * _dyld_register_func_for_add_image when it calls image_added with the + * current list of all loaded dynamic libraries + */ + GSList *dummy = g_slist_prepend (NULL, NULL); + g_slist_free (dummy); + GHashTableIter it; + g_hash_table_iter_init (&it, images); + g_hash_table_iter_next (&it, NULL, NULL); + + _dyld_register_func_for_add_image (&image_added); + _dyld_register_func_for_remove_image (&image_removed); } #else diff --git a/mono/metadata/w32process-unix.c b/mono/metadata/w32process-unix.c index b4fcc3016fe..426e12810c4 100644 --- a/mono/metadata/w32process-unix.c +++ b/mono/metadata/w32process-unix.c @@ -85,6 +85,7 @@ #include #include #include +#include #include #include #include "object-internals.h" @@ -594,6 +595,8 @@ process_set_name (MonoW32HandleProcess *process_handle) } } +static mono_once_t init_state = MONO_ONCE_INIT; + void mono_w32process_init (void) { @@ -615,6 +618,7 @@ mono_w32process_init (void) g_assert (current_process != INVALID_HANDLE_VALUE); mono_coop_mutex_init (&processes_mutex); + mono_once (&init_state, &mono_w32process_platform_init_once); } void -- 2.11.4.GIT