Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConfigurationReader is attempting to load assemblies not associated with the project but found by the DllScanningAssemblyLoader #406

Open
yankun opened this issue Jan 4, 2024 · 7 comments

Comments

@yankun
Copy link

yankun commented Jan 4, 2024

I've found following bug:

The DllScanningAssemlyFinder is scanning the Application's directory for dlls containing .serilog in its name. Later on the ConfigurationReader attempts to load the results with the Assembly.Load function.

Unfortunately Assembly.Load only works with assemblies referenced by the application and does no actual disk lookup. So a user attempted to add a new sink to an application, copying manually the sink's dll into the folder will have no success, as the Dll scanner will find the dll but the ConfigurationReader is unable to load it and the application will crash with a FileNotFoundException.

The behavior of Assembly.Load has changed since in the .NET5 framework: dotnet/runtime#62522

See my other ticket here, how this error will sho up: #407

@yankun yankun changed the title ConfigurationReader is attempting to load assemblies not associated with the project provided by DllScanningAssemblyLoader ConfigurationReader is attempting to load assemblies not associated with the project but found by DllScanningAssemblyLoader Jan 4, 2024
@yankun yankun changed the title ConfigurationReader is attempting to load assemblies not associated with the project but found by DllScanningAssemblyLoader ConfigurationReader is attempting to load assemblies not associated with the project but found by the DllScanningAssemblyLoader Jan 4, 2024
@nblumhardt
Copy link
Member

Thanks for the note. Dealing with LoadFrom() isn't always straightforward, so I'm not sure we'll want to attempt it directly in this project.

Your best bet is to use the ConfigurationReaderOptions(Assembly[]) constructor, and pass through a list of assemblies you've found and loaded via LoadFrom() in the application code. This could be as direct as running over all Serilog.Sinks.*.dll files in your deployment directory and loading them up, depending on your environment. HTH!

@yankun
Copy link
Author

yankun commented Jan 5, 2024

As a work around I've just added references to all sinks we are using to all projects. But I think it clearly should be considered to not do a file based lookup of dlls, as there is the potential, that they can't be loaded. The DllScanningAssemblyFinder will only work on the classic .NET framework and will break newer applications, if it finds a file not referenced by the project.

@0xced
Copy link
Member

0xced commented Jan 5, 2024

As a work around I've just added references to all sinks we are using to all projects.

You should probably do what @nblumhardt suggested instead, i.e. passing explicit assemblies to the ConfigurationReaderOptions. This is safer and will work for single-file deployments.

var configurationAssemblies = new[]
{
    typeof(ILogger).Assembly,                               // Serilog
    typeof(SerilogExpression).Assembly,                     // Serilog.Expressions
    typeof(ProcessLoggerConfigurationExtensions).Assembly,  // Serilog.Enrichers.Process
    typeof(ThreadLoggerConfigurationExtensions).Assembly,   // Serilog.Enrichers.Thread
    typeof(Log4NetTextFormatter).Assembly,                  // Serilog.Formatting.Log4Net
    typeof(ConsoleLoggerConfigurationExtensions).Assembly,  // Serilog.Sinks.Console
    typeof(LoggerConfigurationEventLogExtensions).Assembly, // Serilog.Sinks.EventLog
    typeof(FileLoggerConfigurationExtensions).Assembly,     // Serilog.Sinks.File
    typeof(NotepadLoggerConfigurationExtensions).Assembly,  // Serilog.Sinks.Notepad
    typeof(UdpClientFactory).Assembly,                      // Serilog.Sinks.Udp
};
var readerOptions = new ConfigurationReaderOptions(configurationAssemblies);
configuration.ReadFrom.Configuration(context.Configuration, readerOptions);

But I think it clearly should be considered to not do a file based lookup of dlls, as there is the potential, that they can't be loaded.

This is exactly what happens when you explicitly pass assemblies as in the example above: no file based lookup occurs at all.

@yankun
Copy link
Author

yankun commented Jan 7, 2024

Of course I can do that, but isn't that simply a work around? As I see it, currently the ConfigurationReader is able to execute a code path which might lead to a crash due to an unsupported combination of file based lookup of dlls and the later Assembly.Load. Further more, somebody unaware of the fact that the assemblies have been defined manually, could have a hard time adding a new sink because he first must find this declaration and extend it. No blocker but imho not intuitiv too.

@sommmen
Copy link

sommmen commented Jan 15, 2024

As a work around I've just added references to all sinks we are using to all projects.

You should probably do what @nblumhardt suggested instead, i.e. passing explicit assemblies to the ConfigurationReaderOptions. This is safer and will work for single-file deployments.

var configurationAssemblies = new[]
{
    typeof(ILogger).Assembly,                               // Serilog
    typeof(SerilogExpression).Assembly,                     // Serilog.Expressions
    typeof(ProcessLoggerConfigurationExtensions).Assembly,  // Serilog.Enrichers.Process
    typeof(ThreadLoggerConfigurationExtensions).Assembly,   // Serilog.Enrichers.Thread
    typeof(Log4NetTextFormatter).Assembly,                  // Serilog.Formatting.Log4Net
    typeof(ConsoleLoggerConfigurationExtensions).Assembly,  // Serilog.Sinks.Console
    typeof(LoggerConfigurationEventLogExtensions).Assembly, // Serilog.Sinks.EventLog
    typeof(FileLoggerConfigurationExtensions).Assembly,     // Serilog.Sinks.File
    typeof(NotepadLoggerConfigurationExtensions).Assembly,  // Serilog.Sinks.Notepad
    typeof(UdpClientFactory).Assembly,                      // Serilog.Sinks.Udp
};
var readerOptions = new ConfigurationReaderOptions(configurationAssemblies);
configuration.ReadFrom.Configuration(context.Configuration, readerOptions);

But I think it clearly should be considered to not do a file based lookup of dlls, as there is the potential, that they can't be loaded.

This is exactly what happens when you explicitly pass assemblies as in the example above: no file based lookup occurs at all.

Does specifying a list of assemblies disable the dynamic lookup of dll's?

I'm running into this issue:

Hosting net8 app on IIS.
I remove a reference to Serilog.UI and run msdeploy to publish the app.
msdeploy updates all files, but does not perform a file clean beforehand (default settings).
3 dlls then are left behind in:
image
Then msdeploy fails to start the app:
image

Reproducing this locally (just pasting the Serilog.UI files in the output) gives me a FileNotFoundException:
image

In my case it took a long time to figure out why it was loading Serilog.UI after i removed the reference since it was not used in a Usings block in any configs, and the error message was vague. After inspecting the code online i found that it tries to dynamically load any serilog dll in addition to looking at the usings.

Also i'm now puzzled why it fails to load the DLL, because it should load just fine, its in the output directory it was the right version etc.

Anyhow in my case the 'fix' should be to just remove the extra dll's - but this seems like it could happen again in the future so i thought i'd bring it up.

@yankun
Copy link
Author

yankun commented Jan 15, 2024

Also i'm now puzzled why it fails to load the DLL, because it should load just fine, its in the output directory it was the right version etc.

@sommmen: Because Assembly.Load doesn't load files from the disk which are not referenced by the project in anyway, for security reasons. And that's the issue with serilogy, because it uses a file based lookup and attempting to load the found files, if referenced or not.

@simonthum
Copy link

I think I stumbled on a closely related problem. In a .net 4.8 application, I get an unhandled exception on startup because it loads (or attempts to load) some DLL. But the problem is, it fails one one specific system and works just fine elsewhere. As you might guess it's the customer's system.

I don't have regular access to the system, but I got this from the ETW application log:

Description: The process was terminated due to an unhandled exception.
Exception Info: System.NotSupportedException
Exception Info: System.IO.FileLoadException
   at System.Reflection.RuntimeAssembly._nLoad(System.Reflection.AssemblyName, System.String, System.Security.Policy.Evidence, System.Reflection.RuntimeAssembly, System.Threading.StackCrawlMark ByRef, IntPtr, Boolean, Boolean, Boolean)
   at System.Reflection.RuntimeAssembly.InternalLoadAssemblyName(System.Reflection.AssemblyName, System.Security.Policy.Evidence, System.Reflection.RuntimeAssembly, System.Threading.StackCrawlMark ByRef, IntPtr, Boolean, Boolean, Boolean)
   at System.Reflection.Assembly.Load(System.Reflection.AssemblyName)
   at Serilog.Settings.Configuration.ConfigurationReader.LoadConfigurationAssemblies(Microsoft.Extensions.Configuration.IConfiguration, Serilog.Settings.Configuration.Assemblies.AssemblyFinder)

So it seems that on this one system, the logic fails even before .net 5. My wild guess is some 32 Bit 3rd-party DLL in the GAC or somewhere found by the Auto resolver. I will try the workarounds suggested here next me I get the chance.

It would be nice if this behaviour could be disabled in the config file though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants