Deadlock in loader when using bundling. (mono/mono#15061)

We can hit the following deadlock when loading assemblies (using reflection)
together with bundling:

One thread calls mono_assembly_open_from_bundle, that will take assemblies lock
and then try to load assembly. If this race with a different load of the
same assemblies, one will lose, and that method will call mono_image_close that
in turn will call unload hook (mono_class_unregister_image_generic_subclasses)
that will take loader lock.

If we at the same time have a different thread that calls mono_class_create_from_typedef
, it will take loader lock and then it might end up calling mono_assembly_load_reference
that will take assemblies lock, but since that thread takes the locks
in different order compare to first thread, they could deadlock.

Looking into the use of assemblies lock in mono_assembly_open_from_bundle it
uses a static variable, bundles, that is not protected in other scenarios
and only set by mono_register_bundled_assemblies, normally called during
boot. The method called in the loop mono_image_open_from_data_internal is called
at several places without locks (and looks internal using image lock), so that
doesn't need to be protected. The rest is local data access and the only thing the
lock could do is serialize the whole loop between threads (but that shouldn't be needed).

Fix will remove the locking and also eliminating the incorrect lock order
causing the deadlock.

Commit migrated from https://github.com/mono/mono/commit/a382edc9e1106dd0ab2af87bd6a981f3b08916ec

diff --git a/src/mono/mono/metadata/assembly.c b/src/mono/mono/metadata/assembly.c
index f94bf49..a72f7ed 100644 (file)
--- a/src/mono/mono/metadata/assembly.c
+++ b/src/mono/mono/metadata/assembly.c
@@ -2168,14 +2168,12 @@ mono_assembly_open_from_bundle (const char *filename, MonoImageOpenStatus *statu
        is_satellite = g_str_has_suffix (lowercase_filename, ".resources.dll");
        g_free (lowercase_filename);
        name = g_path_get_basename (filename);
-       mono_assemblies_lock ();
        for (i = 0; !image && bundles [i]; ++i) {
                if (strcmp (bundles [i]->name, is_satellite ? filename : name) == 0) {
                        image = mono_image_open_from_data_internal ((char*)bundles [i]->data, bundles [i]->size, FALSE, status, refonly, FALSE, name);
                        break;
                }
        }
-       mono_assemblies_unlock ();
        if (image) {
                mono_image_addref (image);
                mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_ASSEMBLY, "Assembly Loader loaded assembly from bundle: '%s'.", is_satellite ? filename : name);