Merge pull request #5156 from lambdageek/bug-57850+57851

[sre] Deal with ResolveEventHandler returning an AssemblyBuilder (Fixes #57850, #57851)

Fix a couple of bugs in the situation where a `ResolveEventHandler` happens to return an `AssemblyBuilder`.

* [#57850](https://bugzilla.xamarin.com/show_bug.cgi?id=57850) - if the `ResolveEventHandler` returns a reflection only assembly, throw a `FileNotFoundException`
* [#57851](https://bugzilla.xamarin.com/show_bug.cgi?id=57851) - don't look for a `ReferenceAssemblyAttribute` on assemblies returned by a `ResolveEventHandler` if they happen to be an `AssemblyBuilder`. (the native code path to get there is to try and instantiate a custom attribute in some other assembly, hence the fairly convoluted test in `assemblyresolve_event5` and its helper files)

mcs/class/corlib/System.Reflection.Emit/AssemblyBuilderAccess.cs

@@ -35,11 +35,13 @@ namespace System.Reflection.Emit
 	[ComVisible (true)]
 	[Serializable]
 	[Flags]
+	#region Sync with sre-internals.h
 	public enum AssemblyBuilderAccess {
 		Run = 1,
 		Save = 2,
 		RunAndSave = 3,
 		ReflectionOnly = 6,
 		RunAndCollect = 9
   }
+	#endregion
 }

mcs/class/corlib/Test/System.Reflection.Emit/AssemblyBuilderTest.cs

@@ -1844,5 +1844,68 @@ public void CannotCreateInstanceOfSaveOnlyAssembly ()
 		} catch (NotSupportedException e) {
 		}
      }
+
+	class AssemblyBuilderResolver {
+		private Assembly mock;
+		private ResolveEventHandler d;
+		private string theName;
+
+		public AssemblyBuilderResolver (string theName) {
+			mock = CreateMock (theName);
+			d = new ResolveEventHandler (HandleResolveEvent);
+			this.theName = theName;
+		}
+
+		public void StartHandling () {
+			AppDomain.CurrentDomain.AssemblyResolve += d;
+		}
+
+		public void StopHandling () {
+			AppDomain.CurrentDomain.AssemblyResolve -= d;
+		}
+
+		public Assembly HandleResolveEvent (Object sender, ResolveEventArgs args) {
+			if (args.Name.StartsWith (theName))
+				return mock;
+			else
+				return null;
+		}
+
+		private static Assembly CreateMock (string name) {
+			var an = new AssemblyName (name);
+			var ab = AssemblyBuilder.DefineDynamicAssembly (an, AssemblyBuilderAccess.ReflectionOnly);
+			var mb = ab.DefineDynamicModule (an.Name);
+
+			// Just make some content for the assembly
+			var tb = mb.DefineType ("Foo", TypeAttributes.Public);
+			tb.DefineDefaultConstructor (MethodAttributes.Public);
+
+			tb.CreateType ();
+
+			return ab;
+		}
+	}
+
+	[Test]
+	public void ResolveEventHandlerReflectionOnlyError ()
+	{
+		// Regression test for 57850.
+
+		// If a ResolveEventHandler returns a reflection-only
+		// AssemblyBuilder, we should throw a FileNotFoundException.
+		var s = "ResolveEventHandlerReflectionOnlyErrorAssembly";
+		var h = new AssemblyBuilderResolver (s);
+		Assert.Throws<FileNotFoundException>(() => {
+				h.StartHandling ();
+				var aName = new AssemblyName (s);
+				try {
+					AppDomain.CurrentDomain.Load (aName);
+				} finally {
+					h.StopHandling ();
+				}
+			});
+	}
+
+
 }
 }

mono/metadata/appdomain.c

@@ -1176,6 +1176,13 @@ mono_try_assembly_resolve_handle (MonoDomain *domain, MonoStringHandle fname, Mo
 	params [2] = &isrefonly;
 	MonoReflectionAssemblyHandle result = MONO_HANDLE_NEW (MonoReflectionAssembly, mono_runtime_invoke_checked (method, domain->domain, params, error));
 	ret = !MONO_HANDLE_IS_NULL (result) ? MONO_HANDLE_GETVAL (result, assembly) : NULL;
+
+	if (ret && !refonly && ret->ref_only) {
+		/* .NET Framework throws System.IO.FileNotFoundException in this case */
+		mono_error_set_file_not_found (error, "AssemblyResolveEvent handlers cannot return Assemblies loaded for reflection only");
+		ret = NULL;
+		goto leave;
+	}
 leave:
 	HANDLE_FUNCTION_RETURN_VAL (ret);
 }

mono/metadata/assembly.c

@@ -2108,6 +2108,10 @@ has_reference_assembly_attribute_iterator (MonoImage *image, guint32 typeref_sco
 gboolean
 mono_assembly_has_reference_assembly_attribute (MonoAssembly *assembly, MonoError *error)
 {
+	g_assert (assembly && assembly->image);
+	/* .NET Framework appears to ignore the attribute on dynamic
+	 * assemblies, so don't call this function for dynamic assemblies. */
+	g_assert (!image_is_dynamic (assembly->image));
 	error_init (error);
 
 	/*
@@ -3513,8 +3517,11 @@ prevent_reference_assembly_from_running (MonoAssembly* candidate, gboolean refon
 {
 	MonoError refasm_error;
 	error_init (&refasm_error);
-	if (candidate && !refonly && mono_assembly_has_reference_assembly_attribute (candidate, &refasm_error)) {
-		candidate = NULL;
+	if (candidate && !refonly) {
+		/* .NET Framework seems to not check for ReferenceAssemblyAttribute on dynamic assemblies */
+		if (!image_is_dynamic (candidate->image) &&
+		    mono_assembly_has_reference_assembly_attribute (candidate, &refasm_error))
+			candidate = NULL;
 	}
 	mono_error_cleanup (&refasm_error);
 	return candidate;

mono/metadata/custom-attrs.c

@@ -1950,6 +1950,9 @@ mono_assembly_metadata_foreach_custom_attr (MonoAssembly *assembly, MonoAssembly
 	 */
 
 	image = assembly->image;
+	/* Dynamic images would need to go through the AssemblyBuilder's
+	 * CustomAttributeBuilder array.  Going through the tables below
+	 * definitely won't work. */
 	g_assert (!image_is_dynamic (image));
 	idx = 1; /* there is only one assembly */
 	idx <<= MONO_CUSTOM_ATTR_BITS;

mono/metadata/sre-internals.h

@@ -8,6 +8,15 @@
 
 #include <mono/metadata/object-internals.h>
 
+/* Keep in sync with System.Reflection.Emit.AssemblyBuilderAccess */
+enum MonoAssemblyBuilderAccess {
+	MonoAssemblyBuilderAccess_Run = 1,                /* 0b0001 */
+	MonoAssemblyBuilderAccess_Save = 2,               /* 0b0010 */
+	MonoAssemblyBuilderAccess_RunAndSave = 3,         /* Run | Save */
+	MonoAssemblyBuilderAccess_ReflectionOnly = 6,     /* Refonly | Save */
+	MonoAssemblyBuilderAccess_RunAndCollect = 9,      /* Collect | Run */
+};
+
 typedef struct _ArrayMethod ArrayMethod;
 
 typedef struct {

mono/metadata/sre.c

@@ -1204,6 +1204,25 @@ mono_image_create_token (MonoDynamicImage *assembly, MonoObjectHandle obj,
 
 #ifndef DISABLE_REFLECTION_EMIT
 
+static gboolean
+assemblybuilderaccess_can_refonlyload (guint32 access)
+{
+	return (access & 0x4) != 0;
+}
+
+static gboolean
+assemblybuilderaccess_can_run (guint32 access)
+{
+	return (access & MonoAssemblyBuilderAccess_Run) != 0;
+}
+
+static gboolean
+assemblybuilderaccess_can_save (guint32 access)
+{
+	return (access & MonoAssemblyBuilderAccess_Save) != 0;
+}
+
+
 /*
  * mono_reflection_dynimage_basic_init:
  * @assembly: an assembly builder object
@@ -1260,8 +1279,9 @@ mono_reflection_dynimage_basic_init (MonoReflectionAssemblyBuilder *assemblyb)
 			assembly->assembly.aname.revision = 0;
         }
 
-	assembly->run = assemblyb->access != 2;
-	assembly->save = assemblyb->access != 1;
+	assembly->assembly.ref_only = assemblybuilderaccess_can_refonlyload (assemblyb->access);
+	assembly->run = assemblybuilderaccess_can_run (assemblyb->access);
+	assembly->save = assemblybuilderaccess_can_save (assemblyb->access);
 	assembly->domain = domain;
 
 	char *assembly_name = mono_string_to_utf8_checked (assemblyb->name, &error);

mono/tests/Makefile.am

@@ -126,6 +126,7 @@ TESTS_CS_SRC=		\
 	assemblyresolve_event.cs	\
 	assemblyresolve_event3.cs	\
 	assemblyresolve_event4.cs	\
+	assemblyresolve_event5.cs	\
 	checked.cs		\
 	char-isnumber.cs	\
 	field-layout.cs		\
@@ -826,7 +827,8 @@ PROFILE_DISABLED_TESTS += \
 	bug-389886-3.exe	\
 	constant-division.exe	\
 	dynamic-method-resurrection.exe	\
-	assembly_append_ordering.exe
+	assembly_append_ordering.exe \
+	assemblyresolve_event5.exe
 
 # Test which needs System.Web support
 PROFILE_DISABLED_TESTS += \
@@ -1704,6 +1706,7 @@ assemblyresolve_asm.dll$(PLATFORM_AOT_SUFFIX): assemblyresolve_deps/Test.dll$(PL
 	MONO_PATH="assemblyresolve_deps:$(CLASS)" $(top_builddir)/runtime/mono-wrapper $(AOT_BUILD_FLAGS) assemblyresolve_asm.dll
 assemblyresolve_deps/Test.dll$(PLATFORM_AOT_SUFFIX): assemblyresolve_deps/TestBase.dll$(PLATFORM_AOT_SUFFIX)
 
+EXTRA_DIST += assemblyresolve_TestBase.cs assemblyresolve_Test.cs assemblyresolve_asm.cs 
 assemblyresolve_deps:
 	mkdir -p assemblyresolve_deps
 assemblyresolve_deps/TestBase.dll: assemblyresolve_deps $(srcdir)/assemblyresolve_TestBase.cs
@@ -1719,6 +1722,13 @@ assemblyresolve_event3.exe: assemblyresolve_asm.dll assemblyresolve_deps/Test.dl
 assemblyresolve_event4.exe$(PLATFORM_AOT_SUFFIX): assemblyresolve_deps/Test.dll$(PLATFORM_AOT_SUFFIX) assemblyresolve_deps/TestBase.dll$(PLATFORM_AOT_SUFFIX)
 assemblyresolve_event4.exe: assemblyresolve_deps/Test.dll assemblyresolve_deps/TestBase.dll
 
+EXTRA_DIST += assemblyresolve_event5_label.cs assemblyresolve_event5_helper.cs
+assemblyresolve_deps/assemblyresolve_event5_label.dll: assemblyresolve_event5_label.cs assemblyresolve_deps 
+	$(MCS) -target:library -out:assemblyresolve_deps/assemblyresolve_event5_label.dll $(srcdir)/assemblyresolve_event5_label.cs
+assemblyresolve_event5_helper.dll: assemblyresolve_event5_helper.cs assemblyresolve_deps/assemblyresolve_event5_label.dll
+	$(MCS) -target:library -out:assemblyresolve_event5_helper.dll -r:assemblyresolve_deps/assemblyresolve_event5_label.dll $(srcdir)/assemblyresolve_event5_helper.cs
+assemblyresolve_event5.exe: assemblyresolve_event5_helper.dll
+
 # We use 'test-support-files' to handle an ordering issue between the 'mono/' and 'runtime/' directories
 bug-80307.exe: $(srcdir)/bug-80307.cs
 	$(MCS) -r:$(CLASS)/System.Web.dll -out:$@ $(srcdir)/bug-80307.cs

mono/tests/assemblyresolve_event5.cs

@@ -0,0 +1,63 @@
+using System;
+using System.Reflection;
+using System.Reflection.Emit;
+
+
+public class TestAssemblyResolveEvent {
+	public static int Main (String[] args) {
+		// Regression test for https://bugzilla.xamarin.com/show_bug.cgi?id=57851
+
+		// If the custom attributes of an assembly trigger a
+		// ResolveEventHandler, and that handler returns an
+		// AssemblyBuilder, don't crash.
+		var h = new MockResolver ("assemblyresolve_event5_label");
+		var aName = new AssemblyName ("assemblyresolve_event5_helper");
+		var a = AppDomain.CurrentDomain.Load (aName);
+		var t = a.GetType ("MyClass");
+		h.StartHandling ();
+		var cas = t.GetCustomAttributes (true);
+		h.StopHandling ();
+		return 0;
+	}
+}
+
+
+public class MockResolver {
+	private Assembly mock;
+	private ResolveEventHandler d;
+	private string theName;
+
+	public MockResolver (string theName) {
+		mock = CreateMock (theName);
+		d = new ResolveEventHandler (HandleResolveEvent);
+		this.theName = theName;
+	}
+
+	public void StartHandling () {
+		AppDomain.CurrentDomain.AssemblyResolve += d;
+	}
+
+	public void StopHandling () {
+		AppDomain.CurrentDomain.AssemblyResolve -= d;
+	}
+
+	public Assembly HandleResolveEvent (Object sender, ResolveEventArgs args) {
+		Console.Error.WriteLine ("handling load of {0}", args.Name);
+		if (args.Name.StartsWith (theName))
+			return mock;
+		else
+			return null;
+	}
+
+	private static Assembly CreateMock (string s) {
+		var an = new AssemblyName (s);
+		var ab = AssemblyBuilder.DefineDynamicAssembly (an, AssemblyBuilderAccess.Run);
+		var mb = ab.DefineDynamicModule (an.Name);
+
+		var tb = mb.DefineType ("Foo", TypeAttributes.Public);
+		tb.DefineDefaultConstructor (MethodAttributes.Public);
+		tb.CreateType ();
+
+		return ab;
+	}
+}

mono/tests/assemblyresolve_event5_helper.cs

@@ -0,0 +1,10 @@
+using System;
+
+public class SimpleTypedAttribute : Attribute {
+	public SimpleTypedAttribute (Type t) { }
+}
+
+[SimpleTypedAttribute(typeof(Foo))] /* Foo defined in the assemblyresolve_event5_label assembly */
+public class MyClass {
+	public MyClass () { }
+}

mono/tests/assemblyresolve_event5_label.cs

@@ -0,0 +1,5 @@
+using System;
+
+public class Foo {
+	public Foo () { }
+}

mono/utils/mono-error-internals.h

@@ -123,6 +123,9 @@ mono_error_set_not_supported (MonoError *error, const char *msg_format, ...) MON
 void
 mono_error_set_invalid_operation (MonoError *error, const char *msg_format, ...) MONO_ATTR_FORMAT_PRINTF(2,3);
 
+void
+mono_error_set_file_not_found (MonoError *error, const char *msg_format, ...) MONO_ATTR_FORMAT_PRINTF(2,3);
+
 void
 mono_error_set_exception_instance (MonoError *error, MonoException *exc);
 

mono/utils/mono-error.c

@@ -459,6 +459,20 @@ mono_error_set_invalid_operation (MonoError *oerror, const char *msg_format, ...
 	va_end (args);
 }
 
+/**
+ * mono_error_set_file_not_found:
+ *
+ * System.IO.FileNotFoundException
+ */
+void
+mono_error_set_file_not_found (MonoError *oerror, const char *msg_format, ...)
+{
+	va_list args;
+	va_start (args, msg_format);
+	mono_error_set_generic_errorv (oerror, "System.IO", "FileNotFoundException", msg_format, args);
+	va_end (args);
+}
+
 void
 mono_error_set_invalid_program (MonoError *oerror, const char *msg_format, ...)
 {