Avoid reading and writing global state when loading native library #1563

tmat · 2018-04-12T01:10:27Z

Instead of using PATH environment variable to locate the native DLL, call LoadLibrary explicitly on the path the DLL should be loaded. DllImport will use the already loaded DLL and won't attempt to load it again.

The reason why it is not safe to change PATH environment variable include race-conditions and security concerns in plugin environments, such as msbuild.
For example, if two independently developed msbuild tasks that use different versions of the libgit2 library are loaded to the same msbuild process adds path to the native library to the PATH environment variable there is a chance that one of the writes doesn't make it causing one of the tasks to fail to find the library.

AArnott · 2018-04-12T19:32:02Z

LibGit2Sharp/Properties/AssemblyInfo.cs

@@ -16,3 +17,5 @@
 // The following GUID is for the ID of the typelib if this project is exposed to COM

 [assembly: Guid("c6f71967-5be1-49f5-b48e-861bff498ea3")]
+
+[assembly: InternalsVisibleTo("LibGit2Sharp.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010013172CAC3D61EF825164EF8443ED2F97316D0C2A4A65D3B2F6E5C9175C6C589D6A0EAE803E3E7FC0DA9E6672B1DE036CF74E1D33E21DD83E1145E3A454F92E52107495082DCCD1D9F521592F79F41DF26ED727059F8A4E5D3C23ECC525306831A15F1E56B693FDE112137E973B599A13209A5B63E05EE00886DE594E70A993B5")]


Why? What's the problem with IVT to tests?

From another comment I left:

IVT allows unbounded and unchecked access to all internals which is decidedly against the pattern these tests use so far. There is great value (in my experience) in limiting tests to public API and having IVT will make that boundary much harder to preserve.

Value includes:

focusing on user-impacting behaviors rather than internal ones

no need to fix tests when you refactor code

justification to fix a failing test is straightforward since it generally ties directly to a user scenario instead of some arcane internal behavior that's hard to explain to your manager.

Should I instead add a new public API that returns the name of the native module, so that the tests can use it?

BTW, the tests already link a couple of files from the product as a workaround for not being able to use those internal APIs. This workaround invalidates some of your points, although I agree it's more limited.

BTW, the tests already link a couple of files from the product as a workaround for not being able to use those internal APIs.

That's unfortunate. Can you raise an issue pointing to those?

Linking a few source files with static types in both product and test assemblies isn't ideal, but in my experience it is far preferable to IVT because (as you point out) it severely limits the scope so we don't end up with half our tests written against internals.

OK, I'll try to make it work without IVT.

AArnott · 2018-04-12T19:35:03Z

Directory.Build.props

@@ -5,6 +5,8 @@
    <OutputPath>$(MSBuildThisFileDirectory)bin\$(MSBuildProjectName)\$(Configuration)\</OutputPath>
    <BaseIntermediateOutputPath>$(MSBuildThisFileDirectory)obj\$(MSBuildProjectName)\</BaseIntermediateOutputPath>
    <DefineConstants Condition=" '$(ExtraDefine)' != '' ">$(DefineConstants);$(ExtraDefine)</DefineConstants>
+    <SignAssembly>true</SignAssembly>
+    <AssemblyOriginatorKeyFile>..\libgit2sharp.snk</AssemblyOriginatorKeyFile>


It appears you moved this from the .csproj, which is a directory deeper. But you didn't modify the relative path, so I think this is pointing at a non-existent file now.

Oh, you are right. Strange it's passing. Will fix.

Oh, this is actually correct. Because the path is relative to the project file. I'm gonna normalize it to a full path instead.

Ah ya. Will you be using $(MSBuildThisFileDirectory) instead then?

AArnott · 2018-04-12T19:40:30Z

LibGit2Sharp.TestApp/TestApp.cs

+            var loadFromDirectory = args[1];
+            var expectedPath = Path.Combine(loadFromDirectory, (IntPtr.Size == 4) ? "x86" : "x64", moduleName + ".dll");
+
+            GlobalSettings.NativeLibraryPath = loadFromDirectory;


Is it required that the NetFx host app now help libgit2sharp find the right binary now? That's unfortunate. Is there no way to avoid it? Can't the library encode the logic for how to find the DLL?

No. it's not required. This is a test app that validates that setting GlobalSettings.NativeLibraryPath works.

So, at the very least, TestApp is not a very descriptive name. (What does it test? Is it actually running as part of our test gauntlet?)

Let's at least give it a better name. But having it actually run as part of the tests would be much more useful. If we can't do that, then I'm not sure we should be checking it in at all.

Ugh, sorry. You're consistently a step ahead of me. I do now see the test executing this.

Still, let's improve the name here a bit. NativeLoaderTest or something? If we add additional apps, it will be useful to disambiguate.

I'll rename it to NativeLoaderTestApp. It is used by the new test I added. To test library loading we need to spawn a process, the test app is used for that.

AArnott · 2018-04-12T19:42:15Z

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj

  </PropertyGroup>

  <ItemGroup>
    <ProjectReference Include="..\LibGit2Sharp\LibGit2Sharp.csproj" />
+    <ProjectReference Include="..\LibGit2Sharp.TestApp\LibGit2Sharp.TestApp.csproj" Condition="'$(TargetFramework)' == 'net461'" />


Should you add ReferenceOutputAssembly=false metadata to this project reference so it doesn't proffer Program from the .exe into the test project?

I want the .exe to be referenced by the test project. I find the .exe by typeof(TestApp).Assembly.Location.

Although, it'd probably be better to not add that reference and just assume it get's copied next to the test assembly.

I think a reference is good to have. If the test assembly requires the other assembly to be present at runtime, a project reference will ensure it gets built. Otherwise on a clean repo if I build the tests and run them, they'll fail.

I meant use ReferenceOutputAssembly=false

AArnott · 2018-04-12T19:43:05Z

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj

@@ -2,11 +2,11 @@

  <PropertyGroup>
    <TargetFrameworks>net461;netcoreapp2.0</TargetFrameworks>
-    <DefineConstants Condition=" '$(TargetFramework)' == 'net461' ">$(DefineConstants);DESKTOP</DefineConstants>


Are you sure no one is using the DESKTOP symbol?
Why remove it in favor of using NET461? Doing so means if later we target net462 that all the existing #if directives have to be changed when semantically they just need to know if they're targeting desktop.

Are you sure no one is using the DESKTOP symbol?

Yes. Of course I searched for it. It was used in one place.

Why remove it in favor of using NET461?

NET461 is a standard symbol. I can leave DESKTOP but imo it's better to use standard symbols rather then introducing new ones.

I respect the desire to use symbols that folks will recognize. I just do multi-targeting and retargeting so much and DESKTOP almost always covers my needs in a consistent, unchanging way, even when I have to target multiple desktop frameworks simultaneously.
I'm OK either way though.

AArnott · 2018-04-12T19:44:49Z

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj

@@ -19,8 +19,6 @@
  </ItemGroup>

  <ItemGroup>
-    <Compile Include="..\LibGit2Sharp\Core\Epoch.cs" Link="TestHelpers\Epoch.cs" />
-    <Compile Include="..\LibGit2Sharp\Core\Platform.cs" Link="TestHelpers\Platform.cs" />


Why can these be removed? Is it because you added InternalsVisibleTo? That seems irrelevant to this change and IMO should be omitted.
FWIW, I strongly prefer linking in the source for a few static types over using InternalsVisibleTo. IVT allows unbounded and unchecked access to all internals which is decidedly against the pattern these tests use so far. There is great value (in my experience) in limiting tests to public API and having IVT will make that boundary much harder to preserve.

Why can these be removed? Is it because you added InternalsVisibleTo? That seems irrelevant to this change and IMO should be omitted.

Yes. It's not irrelevant since leaving these creates duplicate types and compiler errors.

So it's solving a problem your IVT change created. Can we revert the IVT change instead?

AArnott · 2018-04-12T19:48:24Z

LibGit2Sharp.Tests/GlobalSettingsFixture.cs

@@ -49,5 +52,35 @@ public void TryingToResetNativeLibraryPathAfterLoadedThrows()

            Assert.Throws<LibGit2SharpException>(() => { GlobalSettings.NativeLibraryPath = "C:/Foo"; });
        }
+
+        [ConditionalFact(typeof(NetFramework))]


We already use xunit.skippablefact in this project. Can you use that instead of bringing in a new way to skip facts?

[SkippableFact] public void LoadFromSpecificPath() { Skip.IfNot(Platform.IsRunningOnNetFramework, ".NET Framework only test."); // ... }

Ok, i didn't see that.

AArnott · 2018-04-12T22:40:17Z

LibGit2Sharp.Tests/GlobalSettingsFixture.cs

-        public void LoadFromSpecifiedPath()
+        [SkippableTheory]
+        [InlineData(new object[] { "x86" })]
+        [InlineData(new object[] { "x64" })]


Nit: I think you can just say [InlineData("x86")] (and skip the array wrapping syntax).

You are right. Not sure why I thought it needs to be object.

tmat · 2018-04-13T23:22:37Z

@AArnott turns out that we'll need to explicitly load the native library on .NET Core as well. Currently .deps.json only works for the main app binary, so it won't be applied to msbuild task binaries :(.

bording · 2018-04-13T23:31:23Z

@tmat That's one of the reasons why this approach makes much more sense IMHO: https://www.natemcmaster.com/blog/2017/11/11/build-tools-in-nuget/

Make it a console app that gets called at build time via targets instead.

tmat · 2018-04-13T23:37:37Z

@bording That has bad performance.

tmat · 2018-04-13T23:39:07Z

Unless it's a server app, but that's much more complicated than loading the native dependency. I'll continue working on this PR to make it work for Core CLR as well.

bording · 2018-04-13T23:41:42Z

Unless it's a server app, but that's much more complicated than loading the native dependency. I'll continue working on this PR to make it work for Core CLR as well.

Given that we need to rely on the deps.json file to load the correct native library, which can vary by linux distro, how are you going to mimic that? You'd need knowledge of the RID to know which library to choose.

tmat · 2018-04-13T23:43:23Z

@bording I don't know yet. Will figure something out. Meanwhile, can we merge this PR? It's fixing it at least for .NET Framework.

bording · 2018-04-13T23:45:02Z

@tmat I'll do a review this weekend, and I know @ethomson will likely want to take a look as well before it gets merged.

tmat · 2018-04-13T23:45:16Z

@bording Excellent. Thanks!

AArnott · 2018-04-14T00:20:01Z

Ya, I have a few MSBuild tasks with dependencies (including Roslyn itself) that are very hard to manage, even without thinking about native libraries. I tried appdomains, load contexts, assembly resolvers, ... but getting the task to work on MSBuild Core and MSBuild (full) was virtually impossible. That's why CodeGeneration.Roslyn finally fell back to invoking a Core CLR console app that ships all its dependencies. But ya, perf is really bad (Roslyn JITing, rescanning all the assemblies repeatedly, etc.) compared to when it was working all in-proc with the MSBuild task. :(

tmat · 2018-04-14T00:20:08Z

@bording I think we can keep it simple for now: the RIDs libgit2sharp.nativebinaries uses are: linux-x64, osx, win7-x64 and win7-x86. These can be easily determined from info returned by RuntimeInformation API. More can be added as needed.

AArnott · 2018-04-14T00:20:56Z

Consider voting up any of my MSBuild issues requesting better support for this, BTW.

tmat · 2018-04-14T00:21:04Z

Future Core CLR runtimes will likely have better support for loading native libraries. Then we can remove the explicit handling.

bording · 2018-04-14T00:26:58Z

@bording I think we can keep it simple for now: the RIDs libgit2sharp.nativebinaries uses are: linux-x64, osx, win7-x64 and win7-x86. These can be easily determined from info returned by RuntimeInformation API. More can be added as needed.

@tmat That's true of the current version of LibGit2Sharp.NativeBinaries, but I'm just waiting for the RID changes I got into corefx to show up in a 2.0.x release before I can finish up libgit2/libgit2sharp.nativebinaries#51.

The problem right now is that the single "linux" library in the NativeBinaries package only really works on Ubuntu. I want to update it to include support for every linux distro that .NET Core runs on, so that will be coming soon.

bording · 2018-04-14T00:40:17Z

To provide a bit more info, at the very least, there's going to need to be a separate native binary for the CentOS family of distros since they use a completely different soname for their OpenSSL library.

tmat · 2018-04-14T00:43:07Z

@bording Will it not be possible (hard) to detect the RID for the distros at runtime?

bording · 2018-04-14T00:47:22Z

@tmat I've not spent time to determine how/if that is possible. Previously, the plan has always been to rely on .NET Core itself to figure that out vs. trying to re-implement that kind of logic directly. Aside from the MSBuild task scenario, I can't think of reason we'd want to.

tmat · 2018-04-14T00:53:42Z

@bording MSBuild is the scenario we want it for :)

bording · 2018-04-14T01:02:17Z

@bording MSBuild is the scenario we want it for :)

@tmat Yeah, I realize that. 😄

My point is, aside from MSBuild, is there a reason for LibGit2Sharp to need to have RID-aware, cross-platform, manual library loading for .NET Core? I can't think of one.

To me, that feels like instead of trying to add that sort of logic here, we should be looking at adding the missing features to MSBuild instead. That way everyone benefits from the effort.

tmat · 2018-04-14T01:03:54Z

Yes, the missing features should be added to Core CLR and MSBuild. That's hopefully gonna happen in future, but for now libgit2sharp is unusable in msbuild tasks. I'd rather do something that's not overly complicated (and I believe this isn't) that makes it work while waiting for the real fix.

AArnott · 2018-04-14T01:14:37Z

for now libgit2sharp is unusable in msbuild tasks

Why do you say that, @tmat? Nerdbank.GitVersioning contains an MSBuild task that invokes libgit2sharp right now and has for many months, and runs on full MSBuild as well as MSBuild Core on all OSs.

bording · 2018-04-14T01:15:03Z

Yes, the missing features should be added to Core CLR and MSBuild. That's hopefully gonna happen in future, but for now libgit2sharp is unusable in msbuild tasks.

I would disagree that it's "unusable" in MSBuild tasks. People are already doing that right now. GitVerson and @AArnott's own Nerdbank.GitVersioning uses it in MSBuild tasks. There's even a GitVersion PR open that's got it working under .NET Core: GitTools/GitVersion#1269

I'd rather do something that's not overly complicated (and I believe this isn't) that makes it work while waiting for the real fix.

Sure, if you think that's possible without having to completely reimplement all of .NET Core's internal logic around RID parsing, etc. and still be able to handle the scenario of choosing the correct distro-specifc RID, then I'd like to see what that looks like.

tmat · 2018-04-14T01:34:02Z

@bording Yeah, it "works" until you change the RIDs (or add more Linux flavors). See Nerdbank.GitVersioning:

https://github.com/AArnott/Nerdbank.GitVersioning/blob/master/src/NerdBank.GitVersioning/GitExtensions.cs#L357-L377

It'd be better if we had the RID logic in one place, where we know what RIDs do we need to support.

tmat · 2018-04-14T01:36:21Z

It also needs to change the PATH environment variable, which is not a good thing to do in msbuild process.

tmat · 2018-04-14T02:18:50Z

Added commit that implements the loading for .NET Core. Tested on Windows (in msbuild task). Not yet on Linux/MacOS.

tmat · 2018-04-16T17:40:24Z

I believe I addressed all feedback now.

ethomson · 2018-04-16T18:46:46Z

@AArnott, @bording do you have any final thoughts on this?

bording · 2018-04-16T20:42:49Z

@ethomson I'd like to do another review pass to take a look at the new changes. Should be able to get to that later today.

bording · 2018-04-17T00:51:31Z

@tmat Allowing the NativeLibraryPath to work with .NET Core but not have a default value seems like a good middle ground, though that does mean the "which library should I choose based on my RID?" problem is now something that has to be solved by the caller.

Also, regarding:

BTW, I noticed the unique id generator is generating a new guid every build. That's unfortuante since it makes the entire build non-deterministic. The guid seems to be only passed to the custom marshaller as a cookie, which doesn't use it for anything. So it seems unnecessary.

That does at least have a historical purpose. It's there to work around a CLR bug apparently: #438 (comment). If that caching bug has been fixed for all the frameworks that we run on now, then it seems like it could be removed.

tmat · 2018-04-17T00:54:09Z

though that does mean the "which library should I choose based on my RID?" problem is now something that has to be solved by the caller.

Yes, that's why I originally put it to this PR.

tmat · 2018-04-17T00:54:48Z

@bording Thanks for review!

bording · 2018-04-17T00:57:13Z

Yes, that's why I originally put it to this PR.

Are you aware of any way to determine the RID at runtime other than having to construct it manually the same way the host does? If that was possible, then it would be more feasible to have a reliable default value for .NET Core.

tmat · 2018-04-17T00:57:47Z

That does at least have a historical purpose. It's there to work around a CLR bug apparently: #438 (comment). If that caching bug has been fixed for all the frameworks that we run on now, then it seems like it could be removed.

Could the MarshalCookie be the commit sha instead of a random guid?

tmat · 2018-04-17T01:01:51Z

I don't think there is public API for RID currently. There is this:
https://github.com/dotnet/core-setup/blob/master/src/managed/Microsoft.DotNet.PlatformAbstractions/RuntimeEnvironment.cs

tmat · 2018-04-17T01:05:03Z

https://github.com/dotnet/corefx/issues/16629

bording · 2018-04-17T01:09:44Z

Could the MarshalCookie be the commit sha insted of a random guid?

It seems like that would work, at least for actually shipped assemblies. You could still run into a problem if you loaded two different versions of the assembly that you built locally, though.

However, what I'd really like to know is if it's even still a problem any more. It was mentioned in #430 (comment) that it was filed as a CLR bug. Do you by any chance have a way to find out if it ever got fixed?

eerhardt · 2018-04-17T02:47:59Z

I don't think there is public API for RID currently. There is this:
https://github.com/dotnet/core-setup/blob/master/src/managed/Microsoft.DotNet.PlatformAbstractions/RuntimeEnvironment.cs

That is the public API currently. You can get it here: https://www.nuget.org/packages/Microsoft.DotNet.PlatformAbstractions/

AArnott · 2018-04-17T04:42:11Z

Could the MarshalCookie be the commit sha insted of a random guid?

It seems like that would work, at least for actually shipped assemblies. You could still run into a problem if you loaded two different versions of the assembly that you built locally, though.

Even two versions built locally would have different commit IDs, except when you're building uncommitted changes multiple times, but how would that ever be a problem in practice?

Since you're using Nerdbank.GitVersioning, you should be able to get the commit ID from a build property, or a unique version from the ThisAssembly.AssemblyInformationalVersion at runtime.

ethomson · 2018-04-17T11:27:35Z

Thanks again, everyone, this is now published at https://www.nuget.org/packages/LibGit2Sharp/0.26.0-preview-0017

Avoid reading and writing global state when loading native library

tmat force-pushed the NativeLoad branch 3 times, most recently from 00d51ab to 9398e8b Compare April 12, 2018 02:06

AArnott suggested changes Apr 12, 2018

View reviewed changes

AArnott reviewed Apr 12, 2018

View reviewed changes

tmat force-pushed the NativeLoad branch from 5290a81 to 0e295fa Compare April 12, 2018 22:43

tmat added 2 commits April 16, 2018 10:36

Fix path

53e1fea

Do not set default value for NativeLibararyPath on .NET Core

e618156

tmat force-pushed the NativeLoad branch from 24016bf to e618156 Compare April 16, 2018 17:36

AArnott approved these changes Apr 16, 2018

View reviewed changes

bording added 2 commits April 16, 2018 20:35

Clean up trailing whitespace

d9496ba

Add code generation targets to Solution Items

497f91f

bording approved these changes Apr 17, 2018

View reviewed changes

ethomson merged commit 8f6ad7c into libgit2:master Apr 17, 2018

CyberSinh mentioned this pull request Apr 17, 2018

DllNotFoundException: Unable to load DLL 'git2-15e1193' when libgit2sharp is used under Desktop Bridge (Centennial) #1539

Closed

bording mentioned this pull request Apr 22, 2018

[WIP] Resolve native library location from runtimes folder #1571

Closed

felixfbecker mentioned this pull request Aug 24, 2018

Error under macOS in PowerShell Core: The type initializer for 'LibGit2Sharp.Core.NativeMethods' threw an exception #1583

Closed

This was referenced Aug 25, 2018

2.2.10 fails on Ubuntu 16 and 18 dotnet/Nerdbank.GitVersioning#217

Closed

Improve GenerateUniqueIdentifierCs #1603

Merged

bording mentioned this pull request Mar 17, 2019

libgit2sharp is not working on latest version of LinqPad #1670

Closed

bording mentioned this pull request Nov 16, 2019

Fix native library loading on Linux #1732

Closed

alex-weaver pushed a commit to alex-weaver/libgit2sharp that referenced this pull request Mar 23, 2023

Merge pull request libgit2#1563 from tmat/NativeLoad

42538f8

Avoid reading and writing global state when loading native library

Avoid reading and writing global state when loading native library #1563

Avoid reading and writing global state when loading native library #1563

Conversation

tmat commented Apr 12, 2018 • edited Loading

AArnott Apr 12, 2018 • 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

tmat Apr 14, 2018 • 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

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

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

tmat Apr 12, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Apr 12, 2018 • 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

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

tmat commented Apr 13, 2018

bording commented Apr 13, 2018

tmat commented Apr 13, 2018

tmat commented Apr 13, 2018

bording commented Apr 13, 2018

tmat commented Apr 13, 2018

bording commented Apr 13, 2018

tmat commented Apr 13, 2018

AArnott commented Apr 14, 2018

tmat commented Apr 14, 2018 • edited Loading

AArnott commented Apr 14, 2018

tmat commented Apr 14, 2018

bording commented Apr 14, 2018 • edited Loading

bording commented Apr 14, 2018

tmat commented Apr 14, 2018

bording commented Apr 14, 2018

tmat commented Apr 14, 2018

bording commented Apr 14, 2018

tmat commented Apr 14, 2018 • edited Loading

AArnott commented Apr 14, 2018

bording commented Apr 14, 2018 • edited Loading

tmat commented Apr 14, 2018 • edited Loading

tmat commented Apr 14, 2018

tmat commented Apr 14, 2018

tmat commented Apr 16, 2018

ethomson commented Apr 16, 2018

bording commented Apr 16, 2018

bording commented Apr 17, 2018

tmat commented Apr 17, 2018

tmat commented Apr 17, 2018

bording commented Apr 17, 2018

tmat commented Apr 17, 2018 • edited Loading

tmat commented Apr 17, 2018 • edited Loading

tmat commented Apr 17, 2018

bording commented Apr 17, 2018

eerhardt commented Apr 17, 2018

AArnott commented Apr 17, 2018

ethomson commented Apr 17, 2018

tmat commented Apr 12, 2018 •

edited

Loading

AArnott Apr 12, 2018 •

edited

Loading

tmat Apr 14, 2018 •

edited

Loading

tmat Apr 12, 2018 •

edited

Loading

tmat Apr 12, 2018 •

edited

Loading

tmat commented Apr 14, 2018 •

edited

Loading

bording commented Apr 14, 2018 •

edited

Loading

tmat commented Apr 14, 2018 •

edited

Loading

bording commented Apr 14, 2018 •

edited

Loading

tmat commented Apr 14, 2018 •

edited

Loading

tmat commented Apr 17, 2018 •

edited

Loading

tmat commented Apr 17, 2018 •

edited

Loading