Race condition in DependencyProperty registration mechanism
- Dominant language
- C#
- Stars
- 7.7k
- Forks
- 1.3k
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 61
Description
### 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:
https://github.com/dotnet/wpf/blob/662423d3614158380ba77d710212ac6bb890f8ac/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/DependencyProperty.cs#L253-L258
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.
https://github.com/dotnet/wpf/blob/662423d3614158380ba77d710212ac6bb890f8ac/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/DependencyProperty.cs#L1060-L1065
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.)
https://github.com/dotnet/wpf/blob/662423d3614158380ba77d710212ac6bb890f8ac/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/DependencyProperty.cs#L296-L299
### 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.
Contributor guide
Assessment
This issue has not been assessed yet.