[reflection] Don't crash when building dynamic custom attributes on dynamic types (Fixes #43291) (#3400)mono-4.6.0.150

* [corlib] Regression test for non-visible custom attributes.

See https://bugzilla.xamarin.com/show_bug.cgi?id=43291

* [reflection] Don't crash when building dynamic custom attributes on dynamic types (Fixes #43291)

1. Don't dereference a NULL custom attribute ctor, throw a type load
   exception.
2. When building MonoCustomAttrInfo from an array of
   MonoReflectinoCustomAttr*, compute the number of
   non-visible (non-public) attributes correctly, and make sure to
   iterate over all the attributes when populating the result array.

Fixes [#43291](https://bugzilla.xamarin.com/show_bug.cgi?id=43291)

* [reflection] Marginally better TLE for custom attrs

At least include a message about what went wrong when trying to
construct a custom attribute from a type that isn't finished yet.

2 files changed, 92 insertions, 18 deletions

diff --git a/mcs/class/corlib/Test/System.Reflection.Emit/CustomAttributeBuilderTest.cs b/mcs/class/corlib/Test/System.Reflection.Emit/CustomAttributeBuilderTest.cs
index d2a00c76909..eb6eff866c6 100644
--- a/mcs/class/corlib/Test/System.Reflection.Emit/CustomAttributeBuilderTest.cs
+++ b/mcs/class/corlib/Test/System.Reflection.Emit/CustomAttributeBuilderTest.cs
@@ -727,6 +727,69 @@ namespace MonoTests.System.Reflection.Emit
 			Assert.AreEqual ("foo", res [0][0]);
 			Assert.AreEqual ("bar", res [1][0]);
 		}
+
+		[AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
+		internal class NonVisibleCustomAttribute : Attribute
+		{
+			public NonVisibleCustomAttribute () {}
+		}
+
+		[AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
+		public class PublicVisibleCustomAttribute : Attribute
+		{
+			public PublicVisibleCustomAttribute () {}
+		}
+
+		private static void AddCustomClassAttribute (TypeBuilder typeBuilder, Type customAttrType)
+		{
+			var attribCtorParams = new Type[] {};
+			var attribCtorInfo = customAttrType.GetConstructor(attribCtorParams);
+			var attribBuilder = new CustomAttributeBuilder(attribCtorInfo, new object[] { });
+			typeBuilder.SetCustomAttribute(attribBuilder);
+		}
+
+		[Test]
+		public void NonvisibleCustomAttribute () {
+			//
+			// We build:
+			//  [VisiblePublicCustom]
+			//  [VisiblePublicCustom]
+			//  [NonVisibleCustom]
+			//  [VisiblePublicCustom]
+			//  class BuiltType { public BuiltType () { } }
+			//
+			// And then we try to get all the attributes.
+			//
+			// Regression test for https://bugzilla.xamarin.com/show_bug.cgi?id=43291
+						var assemblyName = new AssemblyName("Repro43291Asm");
+			var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Run);
+			var moduleBuilder = assemblyBuilder.DefineDynamicModule("Repro43291Mod");
+
+			var typeBuilder = moduleBuilder.DefineType("BuiltType",
+				TypeAttributes.Public | TypeAttributes.Class | TypeAttributes.BeforeFieldInit);
+
+			AddCustomClassAttribute (typeBuilder, typeof (PublicVisibleCustomAttribute));
+			AddCustomClassAttribute (typeBuilder, typeof (PublicVisibleCustomAttribute));
+			AddCustomClassAttribute (typeBuilder, typeof (NonVisibleCustomAttribute));
+			AddCustomClassAttribute (typeBuilder, typeof (PublicVisibleCustomAttribute));
+
+			var createdType = typeBuilder.CreateType ();
+
+			Assert.IsNotNull (createdType);
+
+			var obj = Activator.CreateInstance (createdType);
+
+			Assert.IsNotNull (obj);
+
+			var attrs = obj.GetType ().GetCustomAttributes (typeof (Attribute), true);
+
+			Assert.IsNotNull (attrs);
+
+			Assert.AreEqual (3, attrs.Length);
+			Assert.IsInstanceOfType (typeof (PublicVisibleCustomAttribute), attrs[0]);
+			Assert.IsInstanceOfType (typeof (PublicVisibleCustomAttribute), attrs[1]);
+			Assert.IsInstanceOfType (typeof (PublicVisibleCustomAttribute), attrs[2]);
+		}
 	}
 }
 
diff --git a/mono/metadata/reflection.c b/mono/metadata/reflection.c
index 86ddead705b..9fbac024ad6 100644
--- a/mono/metadata/reflection.c
+++ b/mono/metadata/reflection.c
@@ -1478,12 +1478,12 @@ mono_custom_attrs_from_builders (MonoImage *alloc_img, MonoImage *image, MonoArr
 		if (!custom_attr_visible (image, cattr))
 			not_visible ++;
 	}
-	count -= not_visible;
 
-	ainfo = (MonoCustomAttrInfo *)image_g_malloc0 (alloc_img, MONO_SIZEOF_CUSTOM_ATTR_INFO + sizeof (MonoCustomAttrEntry) * count);
+	int num_attrs = count - not_visible;
+	ainfo = (MonoCustomAttrInfo *)image_g_malloc0 (alloc_img, MONO_SIZEOF_CUSTOM_ATTR_INFO + sizeof (MonoCustomAttrEntry) * num_attrs);
 
 	ainfo->image = image;
-	ainfo->num_attrs = count;
+	ainfo->num_attrs = num_attrs;
 	ainfo->cached = alloc_img != NULL;
 	index = 0;
 	for (i = 0; i < count; ++i) {
@@ -1492,11 +1492,13 @@ mono_custom_attrs_from_builders (MonoImage *alloc_img, MonoImage *image, MonoArr
 			unsigned char *saved = (unsigned char *)mono_image_alloc (image, mono_array_length (cattr->data));
 			memcpy (saved, mono_array_addr (cattr->data, char, 0), mono_array_length (cattr->data));
 			ainfo->attrs [index].ctor = cattr->ctor->method;
+			g_assert (cattr->ctor->method);
 			ainfo->attrs [index].data = saved;
 			ainfo->attrs [index].data_size = mono_array_length (cattr->data);
 			index ++;
 		}
 	}
+	g_assert (index == num_attrs && count == num_attrs + not_visible);
 
 	return ainfo;
 }
@@ -9877,24 +9879,35 @@ mono_custom_attrs_construct_by_type (MonoCustomAttrInfo *cinfo, MonoClass *attr_
 
 	mono_error_init (error);
 
-	n = 0;
 	for (i = 0; i < cinfo->num_attrs; ++i) {
-		if (!attr_klass || mono_class_is_assignable_from (attr_klass, cinfo->attrs [i].ctor->klass))
-			n ++;
+		MonoCustomAttrEntry *centry = &cinfo->attrs[i];
+		if (!centry->ctor) {
+			/* The cattr type is not finished yet */
+			/* We should include the type name but cinfo doesn't contain it */
+			mono_error_set_type_load_name (error, NULL, NULL, "Custom attribute constructor is null because the custom attribute type is not finished yet.");
+			return NULL;
+		}
+	}
+
+	n = 0;
+	if (attr_klass) {
+		for (i = 0; i < cinfo->num_attrs; ++i) {
+			MonoMethod *ctor = cinfo->attrs[i].ctor;
+			g_assert (ctor);
+			if (mono_class_is_assignable_from (attr_klass, ctor->klass))
+				n++;
+		}
+	} else {
+		n = cinfo->num_attrs;
 	}
 
 	result = mono_array_new_cached (mono_domain_get (), mono_defaults.attribute_class, n, error);
 	return_val_if_nok (error, NULL);
 	n = 0;
 	for (i = 0; i < cinfo->num_attrs; ++i) {
-		if (!cinfo->attrs [i].ctor) {
-			/* The cattr type is not finished yet */
-			/* We should include the type name but cinfo doesn't contain it */
-			mono_error_set_type_load_name (error, NULL, NULL, "");
-			return NULL;
-		}
-		if (!attr_klass || mono_class_is_assignable_from (attr_klass, cinfo->attrs [i].ctor->klass)) {
-			attr = create_custom_attr (cinfo->image, cinfo->attrs [i].ctor, cinfo->attrs [i].data, cinfo->attrs [i].data_size, error);
+		MonoCustomAttrEntry *centry = &cinfo->attrs [i];
+		if (!attr_klass || mono_class_is_assignable_from (attr_klass, centry->ctor->klass)) {
+			attr = create_custom_attr (cinfo->image, centry->ctor, centry->data, centry->data_size, error);
 			if (!mono_error_ok (error))
 				return result;
 			mono_array_setref (result, n, attr);
@@ -10302,9 +10315,8 @@ gboolean
 mono_custom_attrs_has_attr (MonoCustomAttrInfo *ainfo, MonoClass *attr_klass)
 {
 	int i;
-	MonoClass *klass;
 	for (i = 0; i < ainfo->num_attrs; ++i) {
-		klass = ainfo->attrs [i].ctor->klass;
+		MonoClass *klass = ainfo->attrs [i].ctor->klass;
 		if (mono_class_has_parent (klass, attr_klass) || (MONO_CLASS_IS_INTERFACE (attr_klass) && mono_class_is_assignable_from (attr_klass, klass)))
 			return TRUE;
 	}
@@ -10324,14 +10336,13 @@ MonoObject*
 mono_custom_attrs_get_attr_checked (MonoCustomAttrInfo *ainfo, MonoClass *attr_klass, MonoError *error)
 {
 	int i, attr_index;
-	MonoClass *klass;
 	MonoArray *attrs;
 
 	mono_error_init (error);
 
 	attr_index = -1;
 	for (i = 0; i < ainfo->num_attrs; ++i) {
-		klass = ainfo->attrs [i].ctor->klass;
+		MonoClass *klass = ainfo->attrs [i].ctor->klass;
 		if (mono_class_has_parent (klass, attr_klass)) {
 			attr_index = i;
 			break;