Calling TypeDescriptor.AddAttributes too many times leads to StackOverflowException

[ericstj]

Description

TypeDesciptor.AddAttributes is a static registration API and each time it's called it adds a new AttributeProvider for the instance or type specified. If this provider is not removed it will remain for the lifetime of the AppDomain/Process. If an application calls this API multiple times for the same instance/type - as may be the case if the call to AddAttributes is in an instance type/method - the providers will accumulate. That accumulation will cause issues wherever CustomTypeDescriptor walks the provider chain.

For example:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/CustomTypeDescriptor.cs

Lines 44 to 173 in 03acd23

	public virtual AttributeCollection GetAttributes()
	{
	if (_parent != null)
	{
	return _parent.GetAttributes();
	}
	return AttributeCollection.Empty;
	}
	/// <summary>
	/// The GetClassName method returns the fully qualified name of the
	/// class this type descriptor is representing. Returning null from
	/// this method causes the TypeDescriptor object to return the
	/// default class name.
	/// </summary>
	public virtual string? GetClassName() => _parent?.GetClassName();
	/// <summary>
	/// The GetComponentName method returns the name of the component instance
	/// this type descriptor is describing.
	/// </summary>
	public virtual string? GetComponentName() => _parent?.GetComponentName();
	/// <summary>
	/// The GetConverter method returns a type converter for the type this type
	/// descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(TypeConverter.RequiresUnreferencedCodeMessage)]
	public virtual TypeConverter? GetConverter()
	{
	if (_parent != null)
	{
	return _parent.GetConverter();
	}
	return new TypeConverter();
	}
	/// <summary>
	/// The GetDefaultEvent method returns the event descriptor for the default
	/// event on the object this type descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(EventDescriptor.RequiresUnreferencedCodeMessage)]
	public virtual EventDescriptor? GetDefaultEvent() => _parent?.GetDefaultEvent();
	/// <summary>
	/// The GetDefaultProperty method returns the property descriptor for the
	/// default property on the object this type descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(PropertyDescriptor.PropertyDescriptorPropertyTypeMessage)]
	public virtual PropertyDescriptor? GetDefaultProperty() => _parent?.GetDefaultProperty();
	/// <summary>
	/// The GetEditor method returns an editor of the given type that is
	/// to be associated with the class this type descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(TypeDescriptor.EditorRequiresUnreferencedCode)]
	public virtual object? GetEditor(Type editorBaseType) => _parent?.GetEditor(editorBaseType);
	/// <summary>
	/// The GetEvents method returns a collection of event descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is
	/// returned. If no parent is provided,this will return an empty
	/// event collection.
	/// </summary>
	public virtual EventDescriptorCollection GetEvents()
	{
	if (_parent != null)
	{
	return _parent.GetEvents();
	}
	return EventDescriptorCollection.Empty;
	}
	/// <summary>
	/// The GetEvents method returns a collection of event descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is
	/// returned. If no parent is provided,this will return an empty
	/// event collection.
	/// </summary>
	[RequiresUnreferencedCode(AttributeCollection.FilterRequiresUnreferencedCodeMessage)]
	public virtual EventDescriptorCollection GetEvents(Attribute[]? attributes)
	{
	if (_parent != null)
	{
	return _parent.GetEvents(attributes);
	}
	return EventDescriptorCollection.Empty;
	}
	/// <summary>
	/// The GetProperties method returns a collection of property descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is returned.
	/// If no parent is provided,this will return an empty
	/// property collection.
	/// </summary>
	[RequiresUnreferencedCode(PropertyDescriptor.PropertyDescriptorPropertyTypeMessage)]
	public virtual PropertyDescriptorCollection GetProperties()
	{
	if (_parent != null)
	{
	return _parent.GetProperties();
	}
	return PropertyDescriptorCollection.Empty;
	}
	/// <summary>
	/// The GetProperties method returns a collection of property descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is returned.
	/// If no parent is provided,this will return an empty
	/// property collection.
	/// </summary>
	[RequiresUnreferencedCode(PropertyDescriptor.PropertyDescriptorPropertyTypeMessage + " " + AttributeCollection.FilterRequiresUnreferencedCodeMessage)]
	public virtual PropertyDescriptorCollection GetProperties(Attribute[]? attributes)
	{
	if (_parent != null)
	{
	return _parent.GetProperties(attributes);
	}
	return PropertyDescriptorCollection.Empty;
	}

These methods recursively walk the _parent chain on the stack - which will eventually lead to StackOverflowException when that chain is too long.

Reproduction Steps

using System.ComponentModel;

for (int i = 0; i < 30000; i++)
    TypeDescriptor.AddAttributes(typeof(int), new TypeConverterAttribute(typeof(MyCoverter)));

// any of the following will overflow
TypeDescriptor.GetAttributes(typeof(int));
TypeDescriptor.GetConverter(typeof(int));
TypeDescriptor.GetProperties(typeof(int));
TypeDescriptor.GetEvents(typeof(int));

public class MyCoverter : TypeConverter
{ }

Expected behavior

The behavior is expected, but it could be better.

Seems like we could capture how many times we recurse and throw a better exception before hitting stack overflow.

We could also consider an analyzer to catch this problem. If someone calls TypeDescriptor.AddProvider without doing anything with the return value - raise a diagnostic. The user might decide to suppress that if they could garuntee the code would only ever run once.

Actual behavior

Stack overflow.
   at System.ComponentModel.TypeDescriptor+AttributeProvider.GetTypeDescriptor(System.Type, System.Object)
   at System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+AttributeProvider+AttributeTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+AttributeProvider+AttributeTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+AttributeProvider+AttributeTypeDescriptor.GetAttributes()
...

Depending on which method is called it may also overflow at any of the following:

System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetConverter()
System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetProperties()
System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetEvents()

Regression?

No, behavior exists in .NETFramework as well.

Known Workarounds

Make sure you remove any provider added with AddAttributes, either in a finally block if it's only needed for the method, or in the Dispose method if it's needed for the lifetime of the type. Alternatively you can add attributes with a singleton like the following:

// register
MyConverterRegistration<int>.EnsureRegistered();

public static class MyConverterRegistration<T>
{
    static MyConverterRegistration() => 
        TypeDescriptor.AddAttributes(typeof(T), new TypeConverterAttribute(typeof(MyCoverter)));
    public static void EnsureRegistered() { /* intentionally empty -- call to register coverter */}
}

Configuration

No response

Other information

No response

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

TypeDesciptor.AddAttributes is a static registration API and each time it's called it adds a new AttributeProvider for the instance or type specified. If this provider is not removed it will remain for the lifetime of the AppDomain/Process. If an application calls this API multiple times for the same instance/type - as may be the case if the call to AddAttributes is in an instance type/method - the providers will accumulate. That accumulation will cause issues wherever CustomTypeDescriptor walks the provider chain.

For example:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/CustomTypeDescriptor.cs

Lines 44 to 173 in 03acd23

	public virtual AttributeCollection GetAttributes()
	{
	if (_parent != null)
	{
	return _parent.GetAttributes();
	}
	return AttributeCollection.Empty;
	}
	/// <summary>
	/// The GetClassName method returns the fully qualified name of the
	/// class this type descriptor is representing. Returning null from
	/// this method causes the TypeDescriptor object to return the
	/// default class name.
	/// </summary>
	public virtual string? GetClassName() => _parent?.GetClassName();
	/// <summary>
	/// The GetComponentName method returns the name of the component instance
	/// this type descriptor is describing.
	/// </summary>
	public virtual string? GetComponentName() => _parent?.GetComponentName();
	/// <summary>
	/// The GetConverter method returns a type converter for the type this type
	/// descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(TypeConverter.RequiresUnreferencedCodeMessage)]
	public virtual TypeConverter? GetConverter()
	{
	if (_parent != null)
	{
	return _parent.GetConverter();
	}
	return new TypeConverter();
	}
	/// <summary>
	/// The GetDefaultEvent method returns the event descriptor for the default
	/// event on the object this type descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(EventDescriptor.RequiresUnreferencedCodeMessage)]
	public virtual EventDescriptor? GetDefaultEvent() => _parent?.GetDefaultEvent();
	/// <summary>
	/// The GetDefaultProperty method returns the property descriptor for the
	/// default property on the object this type descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(PropertyDescriptor.PropertyDescriptorPropertyTypeMessage)]
	public virtual PropertyDescriptor? GetDefaultProperty() => _parent?.GetDefaultProperty();
	/// <summary>
	/// The GetEditor method returns an editor of the given type that is
	/// to be associated with the class this type descriptor is representing.
	/// </summary>
	[RequiresUnreferencedCode(TypeDescriptor.EditorRequiresUnreferencedCode)]
	public virtual object? GetEditor(Type editorBaseType) => _parent?.GetEditor(editorBaseType);
	/// <summary>
	/// The GetEvents method returns a collection of event descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is
	/// returned. If no parent is provided,this will return an empty
	/// event collection.
	/// </summary>
	public virtual EventDescriptorCollection GetEvents()
	{
	if (_parent != null)
	{
	return _parent.GetEvents();
	}
	return EventDescriptorCollection.Empty;
	}
	/// <summary>
	/// The GetEvents method returns a collection of event descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is
	/// returned. If no parent is provided,this will return an empty
	/// event collection.
	/// </summary>
	[RequiresUnreferencedCode(AttributeCollection.FilterRequiresUnreferencedCodeMessage)]
	public virtual EventDescriptorCollection GetEvents(Attribute[]? attributes)
	{
	if (_parent != null)
	{
	return _parent.GetEvents(attributes);
	}
	return EventDescriptorCollection.Empty;
	}
	/// <summary>
	/// The GetProperties method returns a collection of property descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is returned.
	/// If no parent is provided,this will return an empty
	/// property collection.
	/// </summary>
	[RequiresUnreferencedCode(PropertyDescriptor.PropertyDescriptorPropertyTypeMessage)]
	public virtual PropertyDescriptorCollection GetProperties()
	{
	if (_parent != null)
	{
	return _parent.GetProperties();
	}
	return PropertyDescriptorCollection.Empty;
	}
	/// <summary>
	/// The GetProperties method returns a collection of property descriptors
	/// for the object this type descriptor is representing. An optional
	/// attribute array may be provided to filter the collection that is returned.
	/// If no parent is provided,this will return an empty
	/// property collection.
	/// </summary>
	[RequiresUnreferencedCode(PropertyDescriptor.PropertyDescriptorPropertyTypeMessage + " " + AttributeCollection.FilterRequiresUnreferencedCodeMessage)]
	public virtual PropertyDescriptorCollection GetProperties(Attribute[]? attributes)
	{
	if (_parent != null)
	{
	return _parent.GetProperties(attributes);
	}
	return PropertyDescriptorCollection.Empty;
	}

These methods recursively walk the _parent chain on the stack - which will eventually lead to StackOverflowException when that chain is too long.

Reproduction Steps

using System.ComponentModel;

for (int i = 0; i < 30000; i++)
    TypeDescriptor.AddAttributes(typeof(int), new TypeConverterAttribute(typeof(MyCoverter)));

// any of the following will overflow
TypeDescriptor.GetAttributes(typeof(int));
TypeDescriptor.GetConverter(typeof(int));
TypeDescriptor.GetProperties(typeof(int));
TypeDescriptor.GetEvents(typeof(int));

public class MyCoverter : TypeConverter
{ }

Expected behavior

The behavior is expected, but it could be better.

Seems like we could capture how many times we recurse and throw a better exception before hitting stack overflow.

We could also consider an analyzer to catch this problem. If someone calls TypeDescriptor.AddProvider without doing anything with the return value - raise a diagnostic. The user might decide to suppress that if they could garuntee the code would only ever run once.

Actual behavior

Stack overflow.
   at System.ComponentModel.TypeDescriptor+AttributeProvider.GetTypeDescriptor(System.Type, System.Object)
   at System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+AttributeProvider+AttributeTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+AttributeProvider+AttributeTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
   at System.ComponentModel.TypeDescriptor+AttributeProvider+AttributeTypeDescriptor.GetAttributes()
...

Depending on which method is called it may also overflow at any of the following:

System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetAttributes()
System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetConverter()
System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetProperties()
System.ComponentModel.TypeDescriptor+DefaultTypeDescriptor.GetEvents()

Regression?

No, behavior exists in .NETFramework as well.

Known Workarounds

Make sure you remove any provider added with AddAttributes, either in a finally block if it's only needed for the method, or in the Dispose method if it's needed for the lifetime of the type. Alternatively you can add attributes with a singleton like the following:

// register
MyConverterRegistration<int>.EnsureRegistered();

public static class MyConverterRegistration<T>
{
    static MyConverterRegistration() => 
        TypeDescriptor.AddAttributes(typeof(T), new TypeConverterAttribute(typeof(MyCoverter)));
    public static void EnsureRegistered() { /* intentionally empty -- call to register coverter */}
}

Configuration

No response

Other information

No response

Author:	ericstj
Assignees:	-
Labels:	area-System.ComponentModel
Milestone:	-