-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Description
Prior to #116677, did ConfigurationBinder directly reference IConfigurationRoot.Providers for anything? Or did it just use the IConfiguration indexer to read values? I'm a little worried that we've introduced a race condition when combining this new binding logic with ConfigurationManager which is mutable and implements both IConfigurationSource and IConfigurationRoot meaning that the IConfigurationProvider in the list might be disposed by the time you access it leading to an ODE. This is the type returned by WebApplicationBuilder.Configuration and WebApplication.Configuration, so it's pretty widely used.
The indexer works around this issue via reference counting. It prevents the disposal of the IConfigurationProvider until the indexing operation completes. Unfortunately, it's impossible for ConfigurationManager to guarantee any IConfigurationProvider returned by IConfigurationRoot.Providers will never be disposed.
runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs
Lines 53 to 65 in bdf59c6
public string? this[string key] | |
{ | |
get | |
{ | |
using ReferenceCountedProviders reference = _providerManager.GetReference(); | |
return ConfigurationRoot.GetConfiguration(reference.Providers, key); | |
} | |
set | |
{ | |
using ReferenceCountedProviders reference = _providerManager.GetReference(); | |
ConfigurationRoot.SetConfiguration(reference.Providers, key, value); | |
} | |
} |
runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs
Lines 78 to 81 in bdf59c6
// We cannot track the duration of the reference to the providers if this property is used. | |
// If a configuration source is removed after this is accessed but before it's completely enumerated, | |
// this may allow access to a disposed provider. | |
IEnumerable<IConfigurationProvider> IConfigurationRoot.Providers => _providerManager.NonReferenceCountedProviders; |
ConfigurationManager does at least create a new List<IConfigurationProvider>
after any modification to the configuration sources, so the reverse looping itself should be safe. Long term, I think we need to fix the ConfigurationBinder logic so that it iterates over an immutable reference counted list of providers like the indexer does.
Originally posted by @halter73 in #117474 (comment)