Missing argument validation on 'LoggerFactory.AddProvider'

[julealgon]

Description

Microsoft.Extensions.Logging.LoggerFactory implements ILoggerFactory with a AddProvider(ILoggerProvider) method. However, it does not perform any argument validation on provider, meaning it will consider null as a "valid" provider and add it to the collection of providers internally.

Some operations inside the class will unnecessarily throw NullReferenceExceptions due to this behavior, like the Dispose method, which will capture these exceptions and do nothing.

One would expect that null providers are not accepted and the method should throw a ArgumentNullException.

Configuration

Net 5.0
Microsoft.Extensions.Logging latest (v5.0.0)

Other information

Here is where it loads up the provider:

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Lines 148 to 157 in fae3594

	public void AddProvider(ILoggerProvider provider)
	{
	if (CheckDisposed())
	{
	throw new ObjectDisposedException(nameof(LoggerFactory));
	}
	lock (_sync)
	{
	AddProviderRegistration(provider, dispose: true);

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Lines 174 to 180 in fae3594

	private void AddProviderRegistration(ILoggerProvider provider, bool dispose)
	{
	_providerRegistrations.Add(new ProviderRegistration
	{
	Provider = provider,
	ShouldDispose = dispose
	});

And here an example where exceptions will be generated and caught:

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Lines 254 to 264 in fae3594

	try
	{
	if (registration.ShouldDispose)
	{
	registration.Provider.Dispose();
	}
	}
	catch
	{
	// Swallow exceptions on dispose
	}

I'm not sure if this could cause further problems downstream when using the loggers from the factory.

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Microsoft.Extensions.Logging.LoggerFactory implements ILoggerFactory with a AddProvider(ILoggerProvider) method. However, it does not perform any argument validation on provider, meaning it will consider null as a "valid" provider and add it to the collection of providers internally.

Some operations inside the class will unnecessarily throw NullReferenceExceptions due to this behavior, like the Dispose method, which will capture these exceptions and do nothing.

One would expect that null providers are not accepted and the method should throw a ArgumentNullException.

Configuration

Net 5.0
Microsoft.Extensions.Logging latest (v5.0.0)

Other information

Here is where it loads up the provider:

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Lines 148 to 157 in fae3594

	public void AddProvider(ILoggerProvider provider)
	{
	if (CheckDisposed())
	{
	throw new ObjectDisposedException(nameof(LoggerFactory));
	}
	lock (_sync)
	{
	AddProviderRegistration(provider, dispose: true);

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Lines 174 to 180 in fae3594

	private void AddProviderRegistration(ILoggerProvider provider, bool dispose)
	{
	_providerRegistrations.Add(new ProviderRegistration
	{
	Provider = provider,
	ShouldDispose = dispose
	});

And here an example where exceptions will be generated and caught:

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Lines 254 to 264 in fae3594

	try
	{
	if (registration.ShouldDispose)
	{
	registration.Provider.Dispose();
	}
	}
	catch
	{
	// Swallow exceptions on dispose
	}

I'm not sure if this could cause further problems downstream when using the loggers from the factory.

Author:	julealgon
Assignees:	-
Labels:	area-Extensions-Logging, untriaged
Milestone:	-

[maryamariyan]

As part of the effort to enable nullable annotations for extensions libraries (tracked here), we just recently annotated Logging.Abstractions, and that means ILoggerFactory implemented under Logging.Abstractions would actually expect to take a non-nullable ILoggerProvider for ILoggerFactory.AddProvider(ILoggerProvider).

When we enable nullable annotations for Microsoft.Extensions.Logging, it should take this into account for the code snippets you share above as well.

So I think, throwing ArgumentNullException would be mainly beneficial for when nullable annotations are not enabled.

[julealgon]

So I think, throwing ArgumentNullException would be beneficial for when nullable annotations are not enabled.

I think this is ill-advised @maryamariyan : ArgumentNullExceptions should be thrown even in the case that correct nullable annotations are in place.

This is because as of right now, there is no runtime validation leveraging nullable annotations, so if consumers pass a null value, one would still see the same issues. Also keep in mind that people having nullable reference types disabled and also freely pass null into these methods without any warnings without using the ! operator.

I do hope to see C# and the runtime move into full-on runtime validation of these nullable constraints at some point, and then we could remove the manual argument validation from everywhere.

[Michael3161010]

I am looking for a good first issue to pick up, does anyone have an issue if I do that ?

[maryamariyan]

I am looking for a good first issue to pick up, does anyone have an issue if I do that ?

Seems like there is a PR for this now. @Michael3161010 you could perhaps pick another one from here and comment on the one you picked stating you picked it and would be creating a PR for it.

[maryamariyan]

ArgumentNullExceptions should be thrown even in the case that correct nullable annotations are in place.

Thanks for the pointer @julealgon. We should throw even if nullable annotations are enabled, since the consuming library may not have nullable enabled. Also, e.g. Foo(null!) could get past compiler, and then in runtime with this fix we'd receive an ArgumentNullException which is better than the previous NRE.

cc: @safern