Race condition in DependencyProperty registration mechanism

[h3xds1nz]

Description

There is a race condition in DependencyProperty registration mechanism which can cause a successful DependencyProperty registration for the same type, with the same name. What makes it worse is that this property will exist everywhere but in the property name map.

While there are in total 3 attempts to synchronize the access around the collections, it may happen that this will simply not be enough.

Here we check under a lock whether property exists:

wpf/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/DependencyProperty.cs

Lines 253 to 258 in 662423d

	lock (Synchronized)
	{
	if (PropertyFromName.ContainsKey(key))
	{
	throw new ArgumentException(SR.Format(SR.PropertyAlreadyRegistered, name, ownerType.Name));
	}

Afterwards, under the same lock, DependencyProperty gets an unique index and is added into the list of registered properties. Before this lock is taken, it was already possible for another thread to pass the check for property name map registration successfully.

wpf/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/DependencyProperty.cs

Lines 1060 to 1065 in 662423d

	lock (Synchronized)
	{
	packedData = (Flags) GetUniqueGlobalIndex(ownerType, name);
	RegisteredPropertyList.Add(this);
	}

Finally, under the same lock, a property is written without checking whether it exists in the map. That means the previous one that was added into the map, the newer one can replace it (because it has managed to get past the ContainsKey check meanwhile.)

wpf/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/DependencyProperty.cs

Lines 296 to 299 in 662423d

	lock (Synchronized)
	{
	PropertyFromName[key] = dp;
	}

Reproduction Steps

As with all race conditions, hit registration overloads for DependencyProperty with the right timing on multiple threads. It is sometimes replicable by running the current tests in a lucky order even. It is not easy but it ain't impossible.

Expected behavior

A dependency property for a single ownerType with an identical name is not created and partially registered, exception is thrown instead.

Actual behavior

A property is created, returned, the newer one replaces the older one in PropertyFromName and the older one ceases to exist in this map without any notice.

Regression?

No.

Known Workarounds

No response

Impact

In the real application world/usage, given that developers follow guidelines of declaring dependency properties as static readonly, there's almost no impact as it requires a grave human error combined with great amount of luck to hit this one since most of the time this will actually properly fail with ArgumentException so you'd find out rather quickly you've made the mistake of declaring two dependency properties for the same type with the same name; then again, you may not always control all of your dependencies.

Configuration

No response

Other information

This also applies to AddOwner instance method where it may cause a metadata override but that's way harder to trigger.

To be honest, I'm not sure this is worth fixing on its own as it might negatively impact perf but I think it's worth to have this issue existing.