dotnet / dotnet/wpf

Race condition in DependencyProperty registration mechanism

Open
#10,324 0 comments 0 reactions 0 assignees View on GitHub
Investigate
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.