Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use high-level library load API #217
base: main
Are you sure you want to change the base?
Use high-level library load API #217
Changes from 19 commits
d765df3
d877e54
1dd3250
e6fa1c6
61c787a
ca3313c
06121a9
f751f2b
c9b67b6
2497f24
f24f6d2
009a09f
e6e1b38
16e53be
f3b8f1e
11cfb01
678e3ba
5b0e460
11a708e
0260cbd
bc1091c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else block is not needed. This method is only used with .NET Framework 4.x, which only runs on Windows, so there is no need to consider other operating systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about Mono here - not sure if it's a target you might want to support though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not attempting to support Mono. There are several places in code already that assume anything within
#if NETFRAMEWORK
is Windows-only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssemblyDirectory
is listed as one of the unsafe options, so this may get flagged by a security scan. (I admit I do not understand why that location should be considered unsafe.)I think it would be better to allow the application to optionally specify the search path:
Then the application developer can decide whether some search paths are unsafe or not, depending on the application threat model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that, but after some research it looks like CA5393 might have some problems in it: It looks like "SafeDirectories" includes the application directory, but then "ApplicationDirectory" is considered unsafe. Maybe this rule hasn't been updated in a while, or maybe it was made with only Windows in mind? On UNIX it's quite common to load libraries from the assembly directory and if it really was insecure, wouldn't that mean that .NETs own approach of putting managed dependency DLLs next to the assembly is also insecure?
If you want I can still change this but to me, the rule looks a little weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned today that the
DllImportSearchPath
is entirely ignored on non-Windows platforms, and that is not documented: dotnet/dotnet-api-docs#3205In that case should we use your
FindLibrary()
code on those platforms? Or if not, will it even search in the right place(s)? It probably requires testing.Also I found that CA5393 is now disabled by default since it doesn't apply cross-platform, so maybe we don't need to worry about that. (I recalled dealing with this warning in other projects several years ago.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for the near future, loading
libnode
from a system directory is rarely going to be the right choice. The Node.js project does not publish official libnode shared-library binaries, and I believe most applications that do use it now are static-linked. Also, applications are likely to be sensitive to the major version of node. So any apps that do use a libnode shared-library will distribute it with the application.For this project, our plan is to publish libnode binaries via NuGet packages. Then when a .NET application that consumes the package is published, the libnode binary for the target runtime-identifier would get copied to the publish output directory. That would be the assembly directory, or at least relative to it. So I'm thinking the best default should probably be just
DllImportSearchPath.AssemblyDirectory
, and we can let the application override that with an optional parameter as I suggested above.We are working on setting up that infrastructure for building libnode, packaging multiple platforms in nuget packages, and publishing them. When that is further along, it might be necessary to make adjustments to the loading code here anyway.