Use high-level library load API #217

samardzicneo · 2024-03-07T13:07:02Z

.NET provides multiple APIs for loading libraries. One of them is NativeLibrary.Load(string), which imports the library directly from the specified path. However, this requires the caller to search for the path and specify the proper extension for the OS. NativeLibrary.Load(string,Assembly,DllImportSearchPath) searches for the library in the usual library paths, like /lib, and adds extensions automatically.

samardzicneo · 2024-03-07T18:40:59Z

Interesting to see that the CI fails, will mark as draft and look into it, @jasongin thanks for the pipeline run.

~~worked on my machine~~

jasongin · 2024-03-07T18:44:44Z

Thanks for the PR, but I have a couple concerns:

For now, we need the ability to explicitly specify a path to libnode. The plan is eventually to consume it via a nuget package (Build libnode and publish nuget packages #107) which will take care of placing the libnode binary in the same directory as the assembly. But until then, test code loads it directly from a fixed path in the repo.
This code change doesn't build on all supported target frameworks. See the CI build errors. It is because there is a NativeLibrary.cs in the repo that implements the missing APIs for older versions of .NET, including .NET Framework 4.x.

samardzicneo · 2024-03-07T19:59:21Z

@jasongin As for your first concern, this is still possible when using the "better" Load API. If you give it a path, it'll still load it like it did before. For your second concern, I've just pushed a commit which resolves the failing pipeline, and sorry for not running it in my forked repo before PR'ing.

jasongin · 2024-03-07T22:06:47Z

src/NodeApi/Runtime/NodejsPlatform.cs

        if (Current != null)
        {
            throw new InvalidOperationException(
                "Only one Node.js platform instance per process is allowed.");
        }

-        nint libnodeHandle = NativeLibrary.Load(libnodePath);
+#if NET7_0_OR_GREATER


Can this be !NETFRAMEWORK? The 3-parameter overload of NativeLibrary.Load() is present on .NET 6.

This code was removed in the newest revision.

jasongin · 2024-03-07T22:07:30Z

src/NodeApi/Runtime/NodejsPlatform.cs

+
+        nint libnodeHandle =
+            entryAssembly != null
+                ? NativeLibrary.Load(libnode, entryAssembly, null)


I'm a little concerned about the lack of functional parity on .NET Framework 4.x. Would it be feasible to add code in this repo's NativeLibrary.cs to at least replicate the logic for loading from the same directory as the assembly?

I've implemented a partially-featured replica of the original method, but I can't test it because I don't have access to a Windows machine.

jasongin · 2024-03-07T22:09:38Z

src/NodeApi/Runtime/NodejsPlatform.cs

        if (Current != null)
        {
            throw new InvalidOperationException(
                "Only one Node.js platform instance per process is allowed.");
        }

-        nint libnodeHandle = NativeLibrary.Load(libnodePath);
+#if NET7_0_OR_GREATER
+        var entryAssembly = System.Reflection.Assembly.GetEntryAssembly();


Did you consider using DllImportSearchPath.AssemblyDirectory instead? I think technically that would use the current assembly rather than the entry assembly, but that might actually be better.

Even using AssemblyDirectory, you still need to pass an assembly. I'd rather use the system default but I can change the assembly to get the current assembly instead of the entry.

jasongin · 2024-03-07T22:10:39Z

I see the build issues are fixed, thanks.

@samardzicneo, I'm curious what is your motivation for submitting this change? I see you're aware in #218 of the current unfinished state of libnode support.

samardzicneo · 2024-03-07T22:22:09Z

@jasongin Thanks for the review, I'll have to look at your comments tomorrow.

# Conflicts: # src/NodeApi/Runtime/NodejsPlatform.cs

jasongin

Sorry this got more complicated than you probably anticipated, but we have to consider security.

jasongin · 2024-03-13T01:57:00Z

src/NodeApi/Runtime/NodejsPlatform.cs

+
+        nint libnodeHandle =
+            entryAssembly != null
+                ? NativeLibrary.Load(libnode, entryAssembly!, null)


I think we need to specify DllImportSearchPath.SafeDirectories here, because the default search behavior is considered unsafe.

If your goal was to load libnode from the same directory as the application, I think that will work, though the documentation for SafeDirectories is not entirely clear.

In that case the back-compat code in NativeLibrary should search the application directory, which is not necessarily the same as the assembly directory.

SafeDirectories won't use the assembly directory, so I'm ORing SafeDirectories and AssemblyDirectory now.

jasongin · 2024-03-13T01:57:15Z

src/NodeApi/Runtime/NativeLibrary.cs

+        string?[] tryDirectories =
+        [
+            Path.GetDirectoryName(assembly.Location),
+            Environment.CurrentDirectory,


Do not search the current directory; that is very unsafe.

jasongin · 2024-03-13T01:59:57Z

src/NodeApi/Runtime/NativeLibrary.cs

+    /// <returns>Library path if found, otherwise false.</returns>
+    private static string? FindLibrary(string libraryName, Assembly assembly)
+    {
+        if (File.Exists(libraryName))


If libraryName is not an absolute path this searches the current directory (or a relative path from it), which is unsafe.

Added a check to ensure it's an absolute path.

samardzicneo · 2024-03-13T14:00:15Z

@jasongin Thanks for the review. I wasn't aware that loading from cwd is unsafe.

Side note, this is my first time contributing to open source so I came into it fully expecting a load of comments. Especially if it concerns security, I'd rather have more than less comments.

jasongin · 2024-03-22T00:19:08Z

src/NodeApi/Runtime/NodejsPlatform.cs

+                ? NativeLibrary.Load(
+                    libnode,
+                    entryAssembly!,
+                    DllImportSearchPath.AssemblyDirectory | DllImportSearchPath.SafeDirectories


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:

public NodejsPlatform(string libnode, string[]? args = null, DllImportSearchPath? searchPath = null)

Then the application developer can decide whether some search paths are unsafe or not, depending on the application threat model.

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.

I learned today that the DllImportSearchPath is entirely ignored on non-Windows platforms, and that is not documented: dotnet/dotnet-api-docs#3205

In 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.)

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.

src/NodeApi/Runtime/NativeLibrary.cs

jasongin · 2024-03-22T00:57:03Z

src/NodeApi/Runtime/NativeLibrary.cs

+                $"{libraryName}.{libraryExtension}",
+                $"lib{libraryName}.{libraryExtension}"
+            ];
+        }


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.

I was thinking about Mono here - not sure if it's a target you might want to support though.

We're not attempting to support Mono. There are several places in code already that assume anything within #if NETFRAMEWORK is Windows-only.

src/NodeApi/Runtime/NativeLibrary.cs

samardzicneo force-pushed the high-level-library-load branch from b369101 to 0708d44 Compare March 7, 2024 13:51

use high-level library loading

d765df3

samardzicneo force-pushed the high-level-library-load branch from 0708d44 to d765df3 Compare March 7, 2024 13:52

samardzicneo marked this pull request as draft March 7, 2024 18:39

maybe fix build

d877e54

samardzicneo added 4 commits March 7, 2024 21:03

undo brainfart moment

1dd3250

this will fix it

e6fa1c6

i need more sleep

61c787a

remove using directive and reference Assembly by full name

ca3313c

samardzicneo marked this pull request as ready for review March 7, 2024 20:24

jasongin reviewed Mar 7, 2024

View reviewed changes

samardzicneo added 9 commits March 9, 2024 14:04

Merge branch 'main' into high-level-library-load

06121a9

# Conflicts: # src/NodeApi/Runtime/NodejsPlatform.cs

try to add search

f751f2b

update platform file

c9b67b6

use path combine

2497f24

fix build

f24f6d2

fix nullability

009a09f

add null check

e6e1b38

fix loading call

16e53be

simplify null check

f3b8f1e

samardzicneo requested a review from jasongin March 9, 2024 14:21

jasongin requested changes Mar 13, 2024

View reviewed changes

fix safety concerns

11cfb01

different search path

678e3ba

samardzicneo added 2 commits March 13, 2024 15:26

make search path more specific

5b0e460

Merge branch 'microsoft:main' into high-level-library-load

11a708e

samardzicneo requested a review from jasongin March 13, 2024 14:27

jasongin requested changes Mar 22, 2024

View reviewed changes

samardzicneo added 2 commits March 27, 2024 13:59

Merge branch 'main' into high-level-library-load

0260cbd

threads

bc1091c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use high-level library load API #217

Use high-level library load API #217

samardzicneo commented Mar 7, 2024

samardzicneo commented Mar 7, 2024

jasongin commented Mar 7, 2024

samardzicneo commented Mar 7, 2024

jasongin Mar 7, 2024

samardzicneo Mar 9, 2024

jasongin Mar 7, 2024

samardzicneo Mar 9, 2024

jasongin Mar 7, 2024

samardzicneo Mar 9, 2024

jasongin commented Mar 7, 2024

samardzicneo commented Mar 7, 2024 •

edited

Loading

jasongin left a comment

jasongin Mar 13, 2024

samardzicneo Mar 13, 2024

jasongin Mar 13, 2024

samardzicneo Mar 13, 2024

jasongin Mar 13, 2024

samardzicneo Mar 13, 2024

samardzicneo commented Mar 13, 2024

jasongin Mar 22, 2024

samardzicneo Mar 24, 2024

jasongin Apr 6, 2024

jasongin Apr 6, 2024 •

edited

Loading

jasongin Mar 22, 2024

samardzicneo Mar 24, 2024

jasongin Apr 6, 2024

Use high-level library load API #217

Are you sure you want to change the base?

Use high-level library load API #217

Conversation

samardzicneo commented Mar 7, 2024

samardzicneo commented Mar 7, 2024

jasongin commented Mar 7, 2024

samardzicneo commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongin commented Mar 7, 2024

samardzicneo commented Mar 7, 2024 • edited Loading

jasongin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samardzicneo commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongin Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samardzicneo commented Mar 7, 2024 •

edited

Loading

jasongin Apr 6, 2024 •

edited

Loading