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

Use high-level library load API #217

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions src/NodeApi/Runtime/NativeLibrary.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#if !NET7_0_OR_GREATER

using System;
using System.IO;
using System.Reflection;
using System.Runtime.InteropServices;
#if !NETFRAMEWORK
using SysNativeLibrary = System.Runtime.InteropServices.NativeLibrary;
Expand Down Expand Up @@ -50,6 +52,25 @@ public static nint Load(string libraryName)
#endif
}

/// <summary>
/// Loads a native library using the high-level API.
/// </summary>
/// <param name="libraryName">The name of the native library to be loaded.</param>
/// <param name="assembly">The assembly loading the native library.</param>
/// <param name="searchPath">The search path.</param>
/// <returns>The OS handle for the loaded native library.</returns>
public static nint Load(string libraryName, Assembly assembly, DllImportSearchPath? searchPath)
{
#if NETFRAMEWORK
string libraryPath = FindLibrary(libraryName, assembly, searchPath)
?? throw new ArgumentNullException(nameof(libraryName));
samardzicneo marked this conversation as resolved.
Show resolved Hide resolved

return LoadLibrary(libraryPath);
#else
return SysNativeLibrary.Load(libraryName, assembly, searchPath);
#endif
}

/// <summary>
/// Gets the address of an exported symbol.
/// </summary>
Expand All @@ -75,6 +96,77 @@ public static bool TryGetExport(nint handle, string name, out nint procAddress)
#endif
}

/// <summary>
/// Searches various well-known paths for a library and returns the first result.
/// </summary>
/// <param name="libraryName">Name of the library to search for.</param>
/// <param name="assembly">Assembly to search relative from.</param>
/// <param name="searchPath">The search path.</param>
/// <returns>Library path if found, otherwise false.</returns>
private static string? FindLibrary(string libraryName, Assembly assembly, DllImportSearchPath? searchPath)
samardzicneo marked this conversation as resolved.
Show resolved Hide resolved
{
if (Path.IsPathRooted(libraryName) && File.Exists(libraryName))
{
return Path.GetFullPath(libraryName);
}

string[] tryLibraryNames;

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
tryLibraryNames =
[
libraryName,
$"{libraryName}.dll"
];
}
else
{
string libraryExtension = RuntimeInformation.IsOSPlatform(OSPlatform.OSX)
? "dylib"
: "so";

tryLibraryNames =
[
libraryName,
$"lib{libraryName}",
$"{libraryName}.{libraryExtension}",
$"lib{libraryName}.{libraryExtension}"
];
}
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.


string?[] tryDirectories =
[
searchPath == null || (searchPath & DllImportSearchPath.AssemblyDirectory) > 0
? Path.GetDirectoryName(assembly.Location)
: null,

searchPath == null || (searchPath & DllImportSearchPath.SafeDirectories) > 0
? Environment.SystemDirectory
: null,
];

foreach (string? tryDirectory in tryDirectories)
{
if (tryDirectory == null)
{
continue;
}

foreach (string tryLibraryName in tryLibraryNames)
{
string tryLibraryPath = Path.Combine(tryDirectory, tryLibraryName);

if (File.Exists(tryLibraryPath))
{
return tryLibraryPath;
}
}
}

return null;
}

#pragma warning disable CA2101 // Specify marshaling for P/Invoke string arguments

[DllImport("kernel32")]
Expand Down
22 changes: 17 additions & 5 deletions src/NodeApi/Runtime/NodejsPlatform.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System;
using System.Reflection;
using System.Runtime.InteropServices;

namespace Microsoft.JavaScript.NodeApi.Runtime;
Expand All @@ -25,23 +26,34 @@ public sealed class NodejsPlatform : IDisposable
/// <summary>
/// Initializes the Node.js platform.
/// </summary>
/// <param name="libnodePath">Path to the `libnode` shared library, including extension.</param>
/// <param name="libnode">
/// Name of the `libnode` shared library.
/// Has to be a full file path when using .NET Framework.
/// </param>
/// <param name="args">Optional platform arguments.</param>
/// <exception cref="InvalidOperationException">A Node.js platform instance has already been
/// loaded in the current process.</exception>
public NodejsPlatform(
string libnodePath,
string libnode,
string[]? args = null)
{
if (string.IsNullOrEmpty(libnodePath)) throw new ArgumentNullException(nameof(libnodePath));

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

nint libnodeHandle = NativeLibrary.Load(libnodePath);
var entryAssembly = Assembly.GetAssembly(typeof(NodejsPlatform));

nint libnodeHandle =
entryAssembly != null
? NativeLibrary.Load(
libnode,
entryAssembly!,
DllImportSearchPath.AssemblyDirectory | DllImportSearchPath.SafeDirectories
Copy link
Member

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:

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.

Copy link
Author

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.

Copy link
Member

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

Copy link
Member

@jasongin jasongin Apr 6, 2024

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.

)
: NativeLibrary.Load(libnode);

Runtime = new NodejsRuntime(libnodeHandle);

Runtime.CreatePlatform(args, (error) => Console.WriteLine(error), out _platform)
Expand Down
Loading