RangeAttribute throws InvalidCastException when accessed from multiple threads

[bucolucas]

This happens intermittently if you use RangeAttribute for date values:

[Range(typeof(Date), "1900-01-01", "2019-06-06 23:59:00")]
public DateTime CreatedOn { get; set; }

It is reproducible 100% of the time from a debugger.

Construct a RangeAttribute object:

var range = new RangeAttribute(typeof(DateTime), "1900-01-01", "2019-06-06 23:59:00");

Set a breakpoint at line 166 in RangeAttribute.cs:

runtime/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/RangeAttribute.cs

Lines 162 to 167 in 4f9ae42

	private void SetupConversion()
	{
	if (Conversion == null)
	{
	object minimum = Minimum;
	object maximum = Maximum;

Execute .IsValid() from two or more threads:

Parallel.For(0, 2, new ParallelOptions { MaxDegreeOfParallelism = 2 }, (i) => { _ = range.IsValid(DateTime.Now); }

Freeze the first thread to hit RangeAttribute.cs line 166. Disable the breakpoint and allow the other thread to run. It will execute all of SetupConversion since Conversion is still null.

Once the second thread has completed validation, pause the debugger and unfreeze the first thread. An InvalidCastException will be thrown on line 207.

runtime/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/RangeAttribute.cs

Lines 203 to 209 in 4f9ae42

	TypeConverter converter = TypeDescriptor.GetConverter(type);
	IComparable min = (IComparable)(ParseLimitsInInvariantCulture
	? converter.ConvertFromInvariantString((string)minimum)
	: converter.ConvertFromString((string)minimum));
	IComparable max = (IComparable)(ParseLimitsInInvariantCulture
	? converter.ConvertFromInvariantString((string)maximum)
	: converter.ConvertFromString((string)maximum));

Unable to cast object of type 'System.DateTime' to type 'System.String'

System.ComponentModel.DataAnnotations.RangeAttribute.SetupConversion()
System.ComponentModel.DataAnnotations.RangeAttribute.IsValid(Object value)

[svick]

What makes you think that RangeAttribute is thread-safe? Or is there some specific reason why it should be thread-safe?

[bucolucas]

It's not thread safe, and I think it should be. The framework creates one instance of RangeAttribute for each property it's declared on, so if you have fifty instances of the following class:

public class Foo
{
    [RangeAttribute(typeof(DateTime), "2001-01-01", "2050-01-01")]
    public DateTime Bar { get; set; }
}

All instances of Foo.Bar share one instance of the RangeAttribute class.

The attributes are created per-type in a thread-safe manner, and stored in a Singleton collection:

runtime/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttributeStore.cs

Lines 113 to 124 in 4f9ae42

	private TypeStoreItem GetTypeStoreItem(Type type)
	{
	Debug.Assert(type != null);
	lock (_typeStoreItems)
	{
	if (!_typeStoreItems.TryGetValue(type, out TypeStoreItem item))
	{
	// use CustomAttributeExtensions.GetCustomAttributes() to get inherited attributes as well as direct ones
	var attributes = CustomAttributeExtensions.GetCustomAttributes(type, true);
	item = new TypeStoreItem(type, attributes);
	_typeStoreItems[type] = item;

The only reason it isn't thread-safe is because the date casting logic is outside the constructor, which is executed thread-safely.

I think a null check->lock->null check would solve this:

if (Conversion == null)
{
    lock(arbitraryObject)
    {
        if (Conversion == null)
        {
            // Existing conversion code
        }
    {
}

This would only contest the lock if Conversion hasn't been set, and prevent the Conversion logic from executing twice.

[danmoseley]

Thanks for the explanation. Are the other ValidationAttribute's already thread safe?

(I don't myself own this code)

[bucolucas]

I checked the other ValidationAttribute classes, I didn't find anything concerning. This is the only one that is giving my organization issues.

[ajcvickers]

This is not something we plan to change. Data annotations cannot run significant code in their constructors due to legacy compatibility, and making this thread safe in other ways would likely introduce locks or other sources of contention to the code.