From 1dc438307e2a890f582d7c2802d99ff7e7af645c Mon Sep 17 00:00:00 2001 From: Andi McClure Date: Mon, 19 Sep 2016 17:55:33 -0400 Subject: [PATCH] Safety check for "diamond dependencies" on netmodules This is something I noticed while following up on an issue with mcs/tests/test-418.exe. The ECMA-335 spec contains the text: "Usually, a module belongs only to one assembly, but it is possible to share it across assemblies. When assembly A is loaded at runtime, an instance of M3 will be loaded for it. When assembly B is loaded into the same application domain, possibly simultaneously with assembly A, M3 will be shared for both assemblies. Both assemblies also reference F2, for which similar rules apply." What happens in Mono when two assemblies both load the same module? We never planned for that possibility in our architecture. Looking at our current code, when an assembly image loads a module image into its modules[] or files[] array, it immediately sets the "assembly" field of that module to its own MonoAssembly. This field is a plain C pointer with no safety on it whatsoever, and is references in various places including by icalls. Consider the following scenario: - Assembly A is loaded, references module M3, sets M3->assembly to A->assembly. - Assembly B is loaded, references module M3, sets M3->assembly to b->assembly. - Assembly B is unloaded. At this point the image for B is unloaded, but M3 is still alive because A keeps it alive, and its image contains a C pointer to freed memory. There is likely some way to trigger a crash from this point. Because the above scenario is fairly exotic, I dealt with it by just causing the module loader to detect when multiple assemblies are referencing the same module and throwing an exception in this case. I also added a test to verify the exception is thrown. Allowing the module load to raise an exception required adding _checked versions of several functions. --- mono/metadata/assembly-internals.h | 12 ++++ mono/metadata/assembly.c | 9 +++ mono/metadata/assembly.h | 3 +- mono/metadata/class.c | 8 ++- mono/metadata/icall.c | 14 +++-- mono/metadata/image-internals.h | 6 ++ mono/metadata/image.c | 81 +++++++++++++++++++++++--- mono/metadata/image.h | 5 +- mono/tests/Makefile.am | 10 +++- mono/tests/test-multi-netmodule-1-netmodule.cs | 4 ++ mono/tests/test-multi-netmodule-2-dll1.cs | 5 ++ mono/tests/test-multi-netmodule-3-dll2.cs | 5 ++ mono/tests/test-multi-netmodule-4-exe.cs | 28 +++++++++ 13 files changed, 171 insertions(+), 19 deletions(-) create mode 100644 mono/metadata/assembly-internals.h create mode 100644 mono/tests/test-multi-netmodule-1-netmodule.cs create mode 100644 mono/tests/test-multi-netmodule-2-dll1.cs create mode 100644 mono/tests/test-multi-netmodule-3-dll2.cs create mode 100644 mono/tests/test-multi-netmodule-4-exe.cs diff --git a/mono/metadata/assembly-internals.h b/mono/metadata/assembly-internals.h new file mode 100644 index 00000000000..5af7bd6b9a3 --- /dev/null +++ b/mono/metadata/assembly-internals.h @@ -0,0 +1,12 @@ +/* + * Copyright 2015 Xamarin Inc + * Licensed under the MIT license. See LICENSE file in the project root for full license information. + */ +#ifndef __MONO_METADATA_ASSEMBLY_INTERNALS_H__ +#define __MONO_METADATA_ASSEMBLY_INTERNALS_H__ + +#include + +MONO_API MonoImage* mono_assembly_load_module_checked (MonoAssembly *assembly, uint32_t idx, MonoError *error); + +#endif /* __MONO_METADATA_ASSEMBLY_INTERNALS_H__ */ diff --git a/mono/metadata/assembly.c b/mono/metadata/assembly.c index 96025d9d06a..174d45d4a69 100644 --- a/mono/metadata/assembly.c +++ b/mono/metadata/assembly.c @@ -16,7 +16,9 @@ #include #include #include "assembly.h" +#include "assembly-internals.h" #include "image.h" +#include "image-internals.h" #include "object-internals.h" #include #include @@ -3450,6 +3452,13 @@ mono_assembly_load_module (MonoAssembly *assembly, guint32 idx) return mono_image_load_file_for_image (assembly->image, idx); } +MONO_API MonoImage* +mono_assembly_load_module_checked (MonoAssembly *assembly, uint32_t idx, MonoError *error) +{ + return mono_image_load_file_for_image_checked (assembly->image, idx, error); +} + + /** * mono_assembly_foreach: * @func: function to invoke for each assembly loaded diff --git a/mono/metadata/assembly.h b/mono/metadata/assembly.h index bdf3d75f62c..b414971e171 100644 --- a/mono/metadata/assembly.h +++ b/mono/metadata/assembly.h @@ -1,6 +1,7 @@ #ifndef _MONONET_METADATA_ASSEMBLY_H_ #define _MONONET_METADATA_ASSEMBLY_H_ +#include #include MONO_BEGIN_DECLS @@ -32,7 +33,7 @@ MONO_API MonoAssembly* mono_assembly_loaded_full (MonoAssemblyName *aname, mono_ MONO_API void mono_assembly_get_assemblyref (MonoImage *image, int index, MonoAssemblyName *aname); MONO_API void mono_assembly_load_reference (MonoImage *image, int index); MONO_API void mono_assembly_load_references (MonoImage *image, MonoImageOpenStatus *status); -MONO_API MonoImage* mono_assembly_load_module (MonoAssembly *assembly, uint32_t idx); +MONO_RT_EXTERNAL_ONLY MONO_API MonoImage* mono_assembly_load_module (MonoAssembly *assembly, uint32_t idx); MONO_API void mono_assembly_close (MonoAssembly *assembly); MONO_API void mono_assembly_setrootdir (const char *root_dir); MONO_API MONO_CONST_RETURN char *mono_assembly_getrootdir (void); diff --git a/mono/metadata/class.c b/mono/metadata/class.c index 7ec6ee9186e..a9b2b05af2a 100644 --- a/mono/metadata/class.c +++ b/mono/metadata/class.c @@ -18,7 +18,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -216,7 +218,7 @@ mono_class_from_typeref_checked (MonoImage *image, guint32 type_token, MonoError goto done; case MONO_RESOLUTION_SCOPE_MODULEREF: - module = mono_image_load_module (image, idx); + module = mono_image_load_module_checked (image, idx, error); if (module) res = mono_class_from_name_checked (module, nspace, name, error); goto done; @@ -7814,7 +7816,7 @@ search_modules (MonoImage *image, const char *name_space, const char *name, Mono if (cols [MONO_FILE_FLAGS] == FILE_CONTAINS_NO_METADATA) continue; - file_image = mono_image_load_file_for_image (image, i + 1); + file_image = mono_image_load_file_for_image_checked (image, i + 1, error); if (file_image) { klass = mono_class_from_name_checked (file_image, name_space, name, error); if (klass || !is_ok (error)) @@ -7910,7 +7912,7 @@ mono_class_from_name_checked_aux (MonoImage *image, const char* name_space, cons impl = cols [MONO_EXP_TYPE_IMPLEMENTATION]; if ((impl & MONO_IMPLEMENTATION_MASK) == MONO_IMPLEMENTATION_FILE) { - loaded_image = mono_assembly_load_module (image->assembly, impl >> MONO_IMPLEMENTATION_BITS); + loaded_image = mono_assembly_load_module_checked (image->assembly, impl >> MONO_IMPLEMENTATION_BITS, error); if (!loaded_image) return NULL; klass = mono_class_from_name_checked_aux (loaded_image, name_space, name, visited_images, error); diff --git a/mono/metadata/icall.c b/mono/metadata/icall.c index 95374177657..43cea3cda75 100644 --- a/mono/metadata/icall.c +++ b/mono/metadata/icall.c @@ -43,7 +43,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -4827,8 +4829,8 @@ ves_icall_System_Reflection_Assembly_GetManifestResourceInternal (MonoReflection g_assert ((impl & MONO_IMPLEMENTATION_MASK) == MONO_IMPLEMENTATION_FILE); file_idx = impl >> MONO_IMPLEMENTATION_BITS; - module = mono_image_load_file_for_image (assembly->assembly->image, file_idx); - if (!module) + module = mono_image_load_file_for_image_checked (assembly->assembly->image, file_idx, &error); + if (mono_error_set_pending_exception (&error) || !module) return NULL; } else @@ -5028,7 +5030,9 @@ ves_icall_System_Reflection_Assembly_GetModulesInternal (MonoReflectionAssembly mono_array_setref (res, j, rm); } else { - MonoImage *m = mono_image_load_file_for_image (image, i + 1); + MonoImage *m = mono_image_load_file_for_image_checked (image, i + 1, &error); + if (mono_error_set_pending_exception (&error)) + return NULL; if (!m) { MonoString *fname = mono_string_new (mono_domain_get (), mono_metadata_string_heap (image, cols [MONO_FILE_NAME])); mono_set_pending_exception (mono_get_exception_file_not_found2 (NULL, fname)); @@ -5552,7 +5556,9 @@ ves_icall_System_Reflection_Assembly_GetTypes (MonoReflectionAssembly *assembly, /* Append data from all modules in the assembly */ for (i = 0; i < table->rows; ++i) { if (!(mono_metadata_decode_row_col (table, i, MONO_FILE_FLAGS) & FILE_CONTAINS_NO_METADATA)) { - MonoImage *loaded_image = mono_assembly_load_module (image->assembly, i + 1); + MonoImage *loaded_image = mono_assembly_load_module_checked (image->assembly, i + 1, &error); + if (mono_error_set_pending_exception (&error)) + return NULL; if (loaded_image) { MonoArray *ex2; MonoArray *res2; diff --git a/mono/metadata/image-internals.h b/mono/metadata/image-internals.h index 959575f6a0a..6281e3b241c 100644 --- a/mono/metadata/image-internals.h +++ b/mono/metadata/image-internals.h @@ -10,4 +10,10 @@ MonoImage * mono_find_image_owner (void *ptr); +MonoImage* +mono_image_load_file_for_image_checked (MonoImage *image, int fileidx, MonoError *error); + +MonoImage* +mono_image_load_module_checked (MonoImage *image, int idx, MonoError *error); + #endif /* __MONO_METADATA_IMAGE_INTERNALS_H__ */ diff --git a/mono/metadata/image.c b/mono/metadata/image.c index 8f213278e8b..4724a2b2bea 100644 --- a/mono/metadata/image.c +++ b/mono/metadata/image.c @@ -66,18 +66,43 @@ enum { }; static GHashTable *loaded_images_hashes [4] = {NULL, NULL, NULL, NULL}; -static GHashTable *get_loaded_images_hash (gboolean refonly) +static GHashTable * +get_loaded_images_hash (gboolean refonly) { int idx = refonly ? IMAGES_HASH_PATH_REFONLY : IMAGES_HASH_PATH; return loaded_images_hashes [idx]; } -static GHashTable *get_loaded_images_by_name_hash (gboolean refonly) +static GHashTable * +get_loaded_images_by_name_hash (gboolean refonly) { int idx = refonly ? IMAGES_HASH_NAME_REFONLY : IMAGES_HASH_NAME; return loaded_images_hashes [idx]; } +// Change the assembly set in `image` to the assembly set in `assemblyImage`. Halt if overwriting is attempted. +// Can be used on modules loaded through either the "file" or "module" mechanism +static gboolean +assign_assembly_parent_for_netmodule (MonoImage *image, MonoImage *assemblyImage, MonoError *error) +{ + // Assembly to assign + MonoAssembly *assembly = assemblyImage->assembly; + + while (1) { + // Assembly currently assigned + MonoAssembly *assemblyOld = image->assembly; + if (assemblyOld) { + if (assemblyOld == assembly) + return TRUE; + mono_error_set_bad_image (error, assemblyImage, "Attempted to load module %s which has already been loaded by assembly %s. This is not supported in Mono.", image->name, assemblyOld->image->name); + return FALSE; + } + gpointer result = InterlockedExchangePointer((gpointer *)&image->assembly, assembly); + if (result == assembly) + return TRUE; + } +} + static gboolean debug_assembly_unload = FALSE; #define mono_images_lock() if (mutex_inited) mono_os_mutex_lock (&images_mutex) @@ -632,13 +657,13 @@ load_modules (MonoImage *image) } /** - * mono_image_load_module: + * mono_image_load_module_checked: * * Load the module with the one-based index IDX from IMAGE and return it. Return NULL if - * it cannot be loaded. + * it cannot be loaded. NULL without MonoError being set will be interpreted as "not found". */ MonoImage* -mono_image_load_module (MonoImage *image, int idx) +mono_image_load_module_checked (MonoImage *image, int idx, MonoError *error) { MonoTableInfo *t; MonoTableInfo *file_table; @@ -683,9 +708,21 @@ mono_image_load_module (MonoImage *image, int idx) } if (valid) { module_ref = g_build_filename (base_dir, name, NULL); - image->modules [idx - 1] = mono_image_open_full (module_ref, &status, refonly); + MonoImage *moduleImage = mono_image_open_full (module_ref, &status, refonly); if (image->modules [idx - 1]) { + g_assert(!image->modules [idx - 1]->assembly || image->modules [idx - 1]->assembly == image->assembly); image->modules [idx - 1]->assembly = image->assembly; + + if (!assign_assembly_parent_for_netmodule (moduleImage, image, error)) { + mono_image_close (moduleImage); + g_free (module_ref); + g_free (base_dir); + g_list_free (valid_modules); + return NULL; + } + + image->modules [idx - 1] = image; + #ifdef HOST_WIN32 if (image->modules [idx - 1]->is_module_handle) mono_image_fixup_vtable (image->modules [idx - 1]); @@ -704,6 +741,17 @@ mono_image_load_module (MonoImage *image, int idx) return image->modules [idx - 1]; } +MonoImage* +mono_image_load_module (MonoImage *image, int idx) +{ + MonoError error; + mono_error_init (&error); + MonoImage *result = mono_image_load_module_checked (image, idx, &error); + mono_error_assert_ok (&error); + mono_error_cleanup (&error); + return result; +} + static gpointer class_key_extract (gpointer value) { @@ -2165,8 +2213,9 @@ mono_image_get_resource (MonoImage *image, guint32 offset, guint32 *size) return data; } +// Returning NULL with no error set will be interpeted as "not found" MonoImage* -mono_image_load_file_for_image (MonoImage *image, int fileidx) +mono_image_load_file_for_image_checked (MonoImage *image, int fileidx, MonoError *error) { char *base_dir, *name; MonoImage *res; @@ -2201,7 +2250,12 @@ mono_image_load_file_for_image (MonoImage *image, int fileidx) } else { int i; /* g_print ("loaded file %s from %s (%p)\n", name, image->name, image->assembly); */ - res->assembly = image->assembly; + if (!assign_assembly_parent_for_netmodule (res, image, error)) { + mono_image_unlock (image); + mono_image_close (res); + return NULL; + } + for (i = 0; i < res->module_count; ++i) { if (res->modules [i] && !res->modules [i]->assembly) res->modules [i]->assembly = image->assembly; @@ -2226,6 +2280,17 @@ done: return res; } +MonoImage* +mono_image_load_file_for_image (MonoImage *image, int fileidx) +{ + MonoError error; + mono_error_init (&error); + MonoImage *result = mono_image_load_file_for_image_checked (image, fileidx, &error); + mono_error_assert_ok (&error); + mono_error_cleanup (&error); + return result; +} + /** * mono_image_get_strong_name: * @image: a MonoImage diff --git a/mono/metadata/image.h b/mono/metadata/image.h index c3ad58b3431..b158ec26834 100644 --- a/mono/metadata/image.h +++ b/mono/metadata/image.h @@ -3,6 +3,7 @@ #include #include +#include MONO_BEGIN_DECLS @@ -50,9 +51,9 @@ MONO_API int mono_image_ensure_section_idx (MonoImage *image, MONO_API uint32_t mono_image_get_entry_point (MonoImage *image); MONO_API const char *mono_image_get_resource (MonoImage *image, uint32_t offset, uint32_t *size); -MONO_API MonoImage* mono_image_load_file_for_image (MonoImage *image, int fileidx); +MONO_RT_EXTERNAL_ONLY MONO_API MonoImage* mono_image_load_file_for_image (MonoImage *image, int fileidx); -MONO_API MonoImage* mono_image_load_module (MonoImage *image, int idx); +MONO_RT_EXTERNAL_ONLY MONO_API MonoImage* mono_image_load_module (MonoImage *image, int idx); MONO_API const char* mono_image_get_name (MonoImage *image); MONO_API const char* mono_image_get_filename (MonoImage *image); diff --git a/mono/tests/Makefile.am b/mono/tests/Makefile.am index 41e4a9b9a4c..62e875eec28 100644 --- a/mono/tests/Makefile.am +++ b/mono/tests/Makefile.am @@ -6,7 +6,7 @@ else FEATUREFUL_RUNTIME_TEST = test-appdomain-unload endif -check-local: assemblyresolve/test/asm.dll testjit test-generic-sharing test-type-load test-cattr-type-load test-reflection-load-with-context test_platform \ +check-local: assemblyresolve/test/asm.dll testjit test-generic-sharing test-type-load test-multi-netmodule test-cattr-type-load test-reflection-load-with-context test_platform \ test-console-output test-messages test-env-options test-unhandled-exception-2 $(FEATUREFUL_RUNTIME_TEST) test-process-stress rm-empty-logs check-full: test-sgen check-local check-parallel: compile-tests check-full @@ -1076,6 +1076,14 @@ test-type-load: $(TEST_DRIVER_DEPEND) @echo "Testing load-exception.exe..." @$(RUNTIME) load-exceptions.exe > load-exceptions.exe.stdout 2> load-exceptions.exe.stderr +EXTRA_DIST += test-multi-netmodule-1-netmodule.cs test-multi-netmodule-2-dll1.cs test-multi-netmodule-3-dll2.cs test-multi-netmodule-4-exe.cs +test-multi-netmodule: + @$(MCS) -t:module test-multi-netmodule-1-netmodule.cs + @$(MCS) -addmodule:test-multi-netmodule-1-netmodule.netmodule -t:library test-multi-netmodule-2-dll1.cs + @$(MCS) -addmodule:test-multi-netmodule-1-netmodule.netmodule -t:library test-multi-netmodule-3-dll2.cs + @$(MCS) -r:test-multi-netmodule-2-dll1.dll test-multi-netmodule-4-exe.cs + $(RUNTIME) test-multi-netmodule-4-exe.exe > test-multi-netmodule-4-exe.exe.stdout 2> test-multi-netmodule-4-exe.exe.stderr + EXTRA_DIST += custom-attr-errors.cs custom-attr-errors-lib.cs test-cattr-type-load: $(TEST_DRIVER_DEPEND) custom-attr-errors.cs custom-attr-errors-lib.cs $(MCS) -D:WITH_MEMBERS /t:library $(srcdir)/custom-attr-errors-lib.cs diff --git a/mono/tests/test-multi-netmodule-1-netmodule.cs b/mono/tests/test-multi-netmodule-1-netmodule.cs new file mode 100644 index 00000000000..2abdc39d627 --- /dev/null +++ b/mono/tests/test-multi-netmodule-1-netmodule.cs @@ -0,0 +1,4 @@ +// Compiler options: -t:module + +public class M1 { +} diff --git a/mono/tests/test-multi-netmodule-2-dll1.cs b/mono/tests/test-multi-netmodule-2-dll1.cs new file mode 100644 index 00000000000..018db84323a --- /dev/null +++ b/mono/tests/test-multi-netmodule-2-dll1.cs @@ -0,0 +1,5 @@ +// Compiler options: -addmodule:test-multi-netmodule-1-netmodule.netmodule -t:library + +public class M2 { + public M1 m1 = new M1(); +} diff --git a/mono/tests/test-multi-netmodule-3-dll2.cs b/mono/tests/test-multi-netmodule-3-dll2.cs new file mode 100644 index 00000000000..4229b80e101 --- /dev/null +++ b/mono/tests/test-multi-netmodule-3-dll2.cs @@ -0,0 +1,5 @@ +// Compiler options: -addmodule:test-multi-netmodule-1-netmodule.netmodule -t:library + +public class M3 { + public M1 m1 = new M1(); +} diff --git a/mono/tests/test-multi-netmodule-4-exe.cs b/mono/tests/test-multi-netmodule-4-exe.cs new file mode 100644 index 00000000000..03cd2425921 --- /dev/null +++ b/mono/tests/test-multi-netmodule-4-exe.cs @@ -0,0 +1,28 @@ +// Compiler options: -r:test-multi-netmodule-2-dll1.dll + +using System; +using System.Reflection; + +public class M4 { + public static int Main () { + M2 m2 = new M2(); + + // Expecting failure + try { + var DLL = Assembly.LoadFile(@"test-multi-netmodule-3-dll2.dll"); + var m3Type = DLL.GetType("M3"); + var m3 = Activator.CreateInstance(m3Type); + var m3m1Field = m3Type.GetField("m1"); + + Console.WriteLine("M3 assembly:" + m3Type.Assembly); + Console.WriteLine("M3.M1 assembly:" + m3m1Field.DeclaringType.Assembly); + } catch (System.TypeLoadException) { + return 0; + } + + Console.WriteLine("M2 assembly:" + typeof (M2).Assembly); + Console.WriteLine("M2.M1 assembly:" + m2.m1.GetType().Assembly); + + return 1; + } +} -- 2.11.4.GIT