[loader] Clean up locking around adding assemblies to domain/ALC (dot…

…net#32622)

This cleans up three components of assembly loading that could potentially cause races/crashes.

First, both `add_assemblies_to_domain` and `add_assembly_to_alc` now ignore resolved references on netcore. We were seeing a crash on CI related to an assertion in here that resulted from two threads using the same MonoImage simultaneously as one is being loaded in with LoadFile, and this should solve that. Fixes dotnet#2305 

Second, we take both the domain and ALC loaded assemblies locks simultaneously around the assembly being added to both lists, ensuring they stay in sync.

Finally, MonoImage now takes the image lock when writing to references instead of using the global `assemblies_mutex` in assembly.c for all images. This potentially helps reduce contention. Work here is not complete, but for this PR it's good enough. See dotnet#32889

src/mono/mono/metadata/appdomain.c

@@ -128,7 +128,7 @@ static gboolean
 mono_domain_asmctx_from_path (const char *fname, MonoAssembly *requesting_assembly, gpointer user_data, MonoAssemblyContextKind *out_asmctx);
 
 static void
-add_assemblies_to_domain (MonoDomain *domain, MonoAssembly *ass, GHashTable *hash);
+add_assemblies_to_domain (MonoDomain *domain, MonoAssembly *ass, GHashTable *ht);
 
 #if ENABLE_NETCORE
 
@@ -1520,7 +1520,6 @@ mono_domain_assembly_postload_search (MonoAssemblyLoadContext *alc, MonoAssembly
 static void
 add_assemblies_to_domain (MonoDomain *domain, MonoAssembly *ass, GHashTable *ht)
 {
-	gint i;
 	GSList *tmp;
 	gboolean destroy_ht = FALSE;
 
@@ -1533,49 +1532,49 @@ add_assemblies_to_domain (MonoDomain *domain, MonoAssembly *ass, GHashTable *ht)
 		ht = g_hash_table_new (mono_aligned_addr_hash, NULL);
 		destroy_ht = TRUE;
 		for (tmp = domain->domain_assemblies; tmp; tmp = tmp->next) {
-			g_hash_table_insert (ht, tmp->data, tmp->data);
+			g_hash_table_add (ht, tmp->data);
 		}
 	}
 
-	/* FIXME: handle lazy loaded assemblies */
-
 	if (!g_hash_table_lookup (ht, ass)) {
 		mono_assembly_addref (ass);
-		g_hash_table_insert (ht, ass, ass);
+		g_hash_table_add (ht, ass);
 		domain->domain_assemblies = g_slist_append (domain->domain_assemblies, ass);
 		mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_ASSEMBLY, "Assembly %s[%p] added to domain %s, ref_count=%d", ass->aname.name, ass, domain->friendly_name, ass->ref_count);
 	}
 
+#ifndef ENABLE_NETCORE
 	if (ass->image->references) {
-		for (i = 0; i < ass->image->nreferences; i++) {
-			if (ass->image->references[i] && ass->image->references [i] != REFERENCE_MISSING) {
-				if (!g_hash_table_lookup (ht, ass->image->references [i])) {
-					add_assemblies_to_domain (domain, ass->image->references [i], ht);
-				}
+		for (int i = 0; i < ass->image->nreferences; i++) {
+			MonoAssembly *ref = ass->image->references [i];
+			if (ref && ref != REFERENCE_MISSING) {
+				if (!g_hash_table_lookup (ht, ref))
+					add_assemblies_to_domain (domain, ref, ht);
 			}
 		}
 	}
+#endif
+
 	if (destroy_ht)
 		g_hash_table_destroy (ht);
 }
 
+/*
+ * LOCKING: assumes the ALC's assemblies lock is taken
+ */
 #ifdef ENABLE_NETCORE
 static void
 add_assembly_to_alc (MonoAssemblyLoadContext *alc, MonoAssembly *ass)
 {
-	gint i;
 	GSList *tmp;
 
 	g_assert (ass != NULL);
 
 	if (!ass->aname.name)
 		return;
 
-	mono_alc_assemblies_lock (alc);
-
 	for (tmp = alc->loaded_assemblies; tmp; tmp = tmp->next) {
 		if (tmp->data == ass) {
-			mono_alc_assemblies_unlock (alc);
 			return;
 		}
 	}
@@ -1585,14 +1584,6 @@ add_assembly_to_alc (MonoAssemblyLoadContext *alc, MonoAssembly *ass)
 	alc->loaded_assemblies = g_slist_append (alc->loaded_assemblies, ass);
 	mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_ASSEMBLY, "Assembly %s[%p] added to ALC (%p), ref_count=%d", ass->aname.name, ass, (gpointer)alc, ass->ref_count);
 
-	if (ass->image->references) {
-		for (i = 0; i < ass->image->nreferences; i++) {
-			// TODO: remove all this after we're confident this assert isn't hit
-			g_assertf (!ass->image->references [i], "Did not expect reference %d of %s to be resolved", i, ass->image->name);
-		}
-	}
-
-	mono_alc_assemblies_unlock (alc);
 }
 #endif
 
@@ -1672,11 +1663,15 @@ mono_domain_fire_assembly_load (MonoAssemblyLoadContext *alc, MonoAssembly *asse
 	mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_ASSEMBLY, "Loading assembly %s (%p) into domain %s (%p) and ALC %p", assembly->aname.name, assembly, domain->friendly_name, domain, alc);
 
 	mono_domain_assemblies_lock (domain);
+#ifdef ENABLE_NETCORE
+	mono_alc_assemblies_lock (alc);
+#endif
 	add_assemblies_to_domain (domain, assembly, NULL);
-	mono_domain_assemblies_unlock (domain);
 #ifdef ENABLE_NETCORE
 	add_assembly_to_alc (alc, assembly);
+	mono_alc_assemblies_unlock (alc);
 #endif
+	mono_domain_assemblies_unlock (domain);
 
 	mono_domain_fire_assembly_load_event (domain, assembly, error_out);
 

src/mono/mono/metadata/assembly.c

@@ -356,11 +356,21 @@ static MonoAssembly *corlib;
 
 static char* unquote (const char *str);
 
-/* This protects loaded_assemblies and image->references */
-#define mono_assemblies_lock() mono_os_mutex_lock (&assemblies_mutex)
-#define mono_assemblies_unlock() mono_os_mutex_unlock (&assemblies_mutex)
+// This protects loaded_assemblies
 static mono_mutex_t assemblies_mutex;
 
+static inline void
+mono_assemblies_lock ()
+{
+	mono_os_mutex_lock (&assemblies_mutex);
+}
+
+static inline void
+mono_assemblies_unlock ()
+{
+	mono_os_mutex_unlock (&assemblies_mutex);
+}
+
 /* If defined, points to the bundled assembly information */
 static const MonoBundledAssembly **bundles;
 
@@ -1783,15 +1793,15 @@ mono_assembly_load_reference (MonoImage *image, int index)
 	 * image->references is shared between threads, so we need to access
 	 * it inside a critical section.
 	 */
-	mono_assemblies_lock ();
+	mono_image_lock (image);
 	if (!image->references) {
 		MonoTableInfo *t = &image->tables [MONO_TABLE_ASSEMBLYREF];
 
 		image->references = g_new0 (MonoAssembly *, t->rows + 1);
 		image->nreferences = t->rows;
 	}
 	reference = image->references [index];
-	mono_assemblies_unlock ();
+	mono_image_unlock (image);
 	if (reference)
 		return;
 
@@ -1874,7 +1884,7 @@ mono_assembly_load_reference (MonoImage *image, int index)
 	}
 
 commit_reference:
-	mono_assemblies_lock ();
+	mono_image_lock (image);
 	if (reference == NULL) {
 		/* Flag as not found */
 		reference = (MonoAssembly *)REFERENCE_MISSING;
@@ -1894,7 +1904,7 @@ mono_assembly_load_reference (MonoImage *image, int index)
 
 		image->references [index] = reference;
 	}
-	mono_assemblies_unlock ();
+	mono_image_unlock (image);
 
 	if (image->references [index] != reference) {
 		/* Somebody loaded it before us */

src/mono/mono/metadata/loader-internals.h

@@ -33,6 +33,7 @@ struct _MonoAssemblyLoadContext {
 	MonoDomain *domain;
 	MonoLoadedImages *loaded_images;
 	GSList *loaded_assemblies;
+	// If taking this with the domain assemblies_lock, always take this second
 	MonoCoopMutex assemblies_lock;
 	/* Handle of the corresponding managed object.  If the ALC is
 	 * collectible, the handle is weak, otherwise it's strong.

src/mono/mono/metadata/metadata-internals.h

@@ -427,6 +427,8 @@ struct _MonoImage {
 	 * references is initialized only by using the mono_assembly_open
 	 * function, and not by using the lowlevel mono_image_open.
 	 *
+	 * Protected by the image lock.
+	 *
 	 * It is NULL terminated.
 	 */
 	MonoAssembly **references;