From d1e43bd3505a7a84b06317392383b56c38846e39 Mon Sep 17 00:00:00 2001 From: J Wyman Date: Tue, 23 Jun 2015 12:26:46 -0700 Subject: [PATCH 1/2] Fixes potential NRE conditions --- LibGit2Sharp/Core/EncodingMarshaler.cs | 2 +- LibGit2Sharp/Core/Ensure.cs | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/LibGit2Sharp/Core/EncodingMarshaler.cs b/LibGit2Sharp/Core/EncodingMarshaler.cs index ad85c5ec9..08dc8c529 100644 --- a/LibGit2Sharp/Core/EncodingMarshaler.cs +++ b/LibGit2Sharp/Core/EncodingMarshaler.cs @@ -59,7 +59,7 @@ public virtual Object MarshalNativeToManaged(IntPtr pNativeData) public static unsafe IntPtr FromManaged(Encoding encoding, String value) { - if (value == null) + if (encoding == null || value == null) { return IntPtr.Zero; } diff --git a/LibGit2Sharp/Core/Ensure.cs b/LibGit2Sharp/Core/Ensure.cs index 04303ca3d..b3fc3e1e9 100644 --- a/LibGit2Sharp/Core/Ensure.cs +++ b/LibGit2Sharp/Core/Ensure.cs @@ -133,7 +133,13 @@ private static readonly Dictionary Date: Tue, 23 Jun 2015 12:28:27 -0700 Subject: [PATCH 2/2] Resolve racy NRE via rearchitect filter logic. Moves core registration logic into `FilterRegistration` while leaving legacy methods in `GlobalSettings`. Methods in `GlobalSettings` now forward to methods in `FilterRegistration`. Retains handles on all filter and registration values in static `GlobalSettings` class to prevent native code calling disposed managed objects resulting in NRE. Manages register, deregister, and dispose logic coherently. Added a new test to verify improved features and survivability. --- LibGit2Sharp.Tests/FilterFixture.cs | 86 ++++++++++++++++++----------- LibGit2Sharp/Core/Proxy.cs | 6 +- LibGit2Sharp/Filter.cs | 19 +++++-- LibGit2Sharp/FilterRegistration.cs | 66 +++++++++++++++++++--- LibGit2Sharp/GlobalSettings.cs | 86 ++++++++++++++++++++++++++--- 5 files changed, 207 insertions(+), 56 deletions(-) diff --git a/LibGit2Sharp.Tests/FilterFixture.cs b/LibGit2Sharp.Tests/FilterFixture.cs index 10461d8c9..5b8918064 100644 --- a/LibGit2Sharp.Tests/FilterFixture.cs +++ b/LibGit2Sharp.Tests/FilterFixture.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.IO; using LibGit2Sharp.Tests.TestHelpers; using Xunit; @@ -26,7 +27,7 @@ public void CanRegisterFilterWithSingleAttribute() [Fact] public void CanRegisterAndUnregisterTheSameFilter() { - var filter = new EmptyFilter(FilterName + 1, attributes); + var filter = new EmptyFilter(FilterName, attributes); var registration = GlobalSettings.RegisterFilter(filter); GlobalSettings.DeregisterFilter(registration); @@ -38,8 +39,7 @@ public void CanRegisterAndUnregisterTheSameFilter() [Fact] public void CanRegisterAndDeregisterAfterGarbageCollection() { - var filter = new EmptyFilter(FilterName + 2, attributes); - var filterRegistration = GlobalSettings.RegisterFilter(filter); + var filterRegistration = GlobalSettings.RegisterFilter(new EmptyFilter(FilterName, attributes)); GC.Collect(); @@ -49,7 +49,7 @@ public void CanRegisterAndDeregisterAfterGarbageCollection() [Fact] public void SameFilterIsEqual() { - var filter = new EmptyFilter(FilterName + 3, attributes); + var filter = new EmptyFilter(FilterName, attributes); Assert.Equal(filter, filter); } @@ -62,16 +62,16 @@ public void InitCallbackNotMadeWhenFilterNeverUsed() called = true; }; - var filter = new FakeFilter(FilterName + 11, attributes, - successCallback, - successCallback, - initializeCallback); - - var filterRegistration = GlobalSettings.RegisterFilter(filter); + var filter = new FakeFilter(FilterName, + attributes, + successCallback, + successCallback, + initializeCallback); + var registration = GlobalSettings.RegisterFilter(filter); Assert.False(called); - GlobalSettings.DeregisterFilter(filterRegistration); + GlobalSettings.DeregisterFilter(registration); } [Fact] @@ -83,12 +83,12 @@ public void InitCallbackMadeWhenUsingTheFilter() called = true; }; - var filter = new FakeFilter(FilterName + 12, attributes, - successCallback, - successCallback, - initializeCallback); - - var filterRegistration = GlobalSettings.RegisterFilter(filter); + var filter = new FakeFilter(FilterName, + attributes, + successCallback, + successCallback, + initializeCallback); + var registration = GlobalSettings.RegisterFilter(filter); Assert.False(called); string repoPath = InitNewRepository(); @@ -98,7 +98,7 @@ public void InitCallbackMadeWhenUsingTheFilter() Assert.True(called); } - GlobalSettings.DeregisterFilter(filterRegistration); + GlobalSettings.DeregisterFilter(registration); } [Fact] @@ -112,9 +112,9 @@ public void WhenStagingFileApplyIsCalledWithCleanForCorrectPath() called = true; reader.CopyTo(writer); }; - var filter = new FakeFilter(FilterName + 15, attributes, clean); - var filterRegistration = GlobalSettings.RegisterFilter(filter); + var filter = new FakeFilter(FilterName, attributes, clean); + var registration = GlobalSettings.RegisterFilter(filter); using (var repo = CreateTestRepository(repoPath)) { @@ -122,7 +122,7 @@ public void WhenStagingFileApplyIsCalledWithCleanForCorrectPath() Assert.True(called); } - GlobalSettings.DeregisterFilter(filterRegistration); + GlobalSettings.DeregisterFilter(registration); } [Fact] @@ -135,22 +135,20 @@ public void CleanFilterWritesOutputToObjectTree() Action cleanCallback = SubstitutionCipherFilter.RotateByThirteenPlaces; - var filter = new FakeFilter(FilterName + 16, attributes, cleanCallback); - - var filterRegistration = GlobalSettings.RegisterFilter(filter); + var filter = new FakeFilter(FilterName, attributes, cleanCallback); + var registration = GlobalSettings.RegisterFilter(filter); using (var repo = CreateTestRepository(repoPath)) { FileInfo expectedFile = StageNewFile(repo, decodedInput); var commit = repo.Commit("Clean that file"); - var blob = (Blob)commit.Tree[expectedFile.Name].Target; var textDetected = blob.GetContentText(); Assert.Equal(encodedInput, textDetected); } - GlobalSettings.DeregisterFilter(filterRegistration); + GlobalSettings.DeregisterFilter(registration); } [Fact] @@ -164,8 +162,8 @@ public void WhenCheckingOutAFileFileSmudgeWritesCorrectFileToWorkingDirectory() Action smudgeCallback = SubstitutionCipherFilter.RotateByThirteenPlaces; - var filter = new FakeFilter(FilterName + 17, attributes, null, smudgeCallback); - var filterRegistration = GlobalSettings.RegisterFilter(filter); + var filter = new FakeFilter(FilterName, attributes, null, smudgeCallback); + var registration = GlobalSettings.RegisterFilter(filter); FileInfo expectedFile = CheckoutFileForSmudge(repoPath, branchName, encodedInput); @@ -173,7 +171,7 @@ public void WhenCheckingOutAFileFileSmudgeWritesCorrectFileToWorkingDirectory() string readAllText = File.ReadAllText(combine); Assert.Equal(decodedInput, readAllText); - GlobalSettings.DeregisterFilter(filterRegistration); + GlobalSettings.DeregisterFilter(registration); } [Fact] @@ -186,8 +184,8 @@ public void CanFilterLargeFiles() string repoPath = InitNewRepository(); - var filter = new FileExportFilter("exportFilter", attributes); - var filterRegistration = GlobalSettings.RegisterFilter(filter); + var filter = new FileExportFilter(FilterName, attributes); + var registration = GlobalSettings.RegisterFilter(filter); string filePath = Path.Combine(Directory.GetParent(repoPath).Parent.FullName, Guid.NewGuid().ToString() + ".blob"); FileInfo contentFile = new FileInfo(filePath); @@ -233,7 +231,31 @@ public void CanFilterLargeFiles() contentFile.Delete(); - GlobalSettings.DeregisterFilter(filterRegistration); + GlobalSettings.DeregisterFilter(registration); + } + + [Fact] + public void DoubleRegistrationFailsButDoubleDeregistrationDoesNot() + { + Assert.Equal(0, GlobalSettings.GetRegisteredFilters().Count()); + + var filter = new EmptyFilter(FilterName, attributes); + var registration = GlobalSettings.RegisterFilter(filter); + + Assert.Throws(() => { GlobalSettings.RegisterFilter(filter); }); + Assert.Equal(1, GlobalSettings.GetRegisteredFilters().Count()); + + Assert.True(registration.IsValid, "FilterRegistration.IsValid should be true."); + + GlobalSettings.DeregisterFilter(registration); + Assert.Equal(0, GlobalSettings.GetRegisteredFilters().Count()); + + Assert.False(registration.IsValid, "FilterRegistration.IsValid should be false."); + + GlobalSettings.DeregisterFilter(registration); + Assert.Equal(0, GlobalSettings.GetRegisteredFilters().Count()); + + Assert.False(registration.IsValid, "FilterRegistration.IsValid should be false."); } private unsafe bool CharArrayAreEqual(char[] array1, char[] array2, int count) diff --git a/LibGit2Sharp/Core/Proxy.cs b/LibGit2Sharp/Core/Proxy.cs index a581725e6..09ad8a1fb 100644 --- a/LibGit2Sharp/Core/Proxy.cs +++ b/LibGit2Sharp/Core/Proxy.cs @@ -809,12 +809,12 @@ public static GitDiffDelta git_diff_get_delta(DiffSafeHandle diff, int idx) #region git_filter_ - public static void git_filter_register(string name, FilterRegistration filterRegistration, int priority) + public static void git_filter_register(string name, IntPtr filterPtr, int priority) { - int res = NativeMethods.git_filter_register(name, filterRegistration.FilterPointer, priority); + int res = NativeMethods.git_filter_register(name, filterPtr, priority); if (res == (int)GitErrorCode.Exists) { - var message = string.Format("A filter with the name '{0}' is already registered", name); + var message = String.Format("A filter with the name '{0}' is already registered", name); throw new EntryExistsException(message); } Ensure.ZeroResult(res); diff --git a/LibGit2Sharp/Filter.cs b/LibGit2Sharp/Filter.cs index 56cee570a..23814e3ba 100644 --- a/LibGit2Sharp/Filter.cs +++ b/LibGit2Sharp/Filter.cs @@ -15,15 +15,10 @@ namespace LibGit2Sharp public abstract class Filter : IEquatable { private static readonly LambdaEqualityHelper equalityHelper = - new LambdaEqualityHelper(x => x.Name, x => x.Attributes); + new LambdaEqualityHelper(x => x.Name, x => x.Attributes); // 64K is optimal buffer size per https://technet.microsoft.com/en-us/library/cc938632.aspx private const int BufferSize = 64 * 1024; - private readonly string name; - private readonly IEnumerable attributes; - - private readonly GitFilter gitFilter; - /// /// Initializes a new instance of the class. /// And allocates the filter natively. @@ -46,6 +41,18 @@ protected Filter(string name, IEnumerable attributes) stream = StreamCreateCallback, }; } + /// + /// Finalizer called by the , deregisters and frees native memory associated with the registered filter in libgit2. + /// + ~Filter() + { + GlobalSettings.DeregisterFilter(this); + } + + private readonly string name; + private readonly IEnumerable attributes; + private readonly GitFilter gitFilter; + private readonly object @lock = new object(); private GitWriteStream thisStream; private GitWriteStream nextStream; diff --git a/LibGit2Sharp/FilterRegistration.cs b/LibGit2Sharp/FilterRegistration.cs index 9b4ea4e0f..710f4f69c 100644 --- a/LibGit2Sharp/FilterRegistration.cs +++ b/LibGit2Sharp/FilterRegistration.cs @@ -9,26 +9,78 @@ namespace LibGit2Sharp /// public sealed class FilterRegistration { - internal FilterRegistration(Filter filter) + /// + /// Maximum priority value a filter can have. A value of 200 will be run last on checkout and first on checkin. + /// + public const int FilterPriorityMax = 200; + /// + /// Minimum priority value a filter can have. A value of 0 will be run first on checkout and last on checkin. + /// + public const int FilterPriorityMin = 0; + + /// + /// + /// + /// + /// + internal FilterRegistration(Filter filter, int priority) { - Ensure.ArgumentNotNull(filter, "filter"); - Name = filter.Name; + System.Diagnostics.Debug.Assert(filter != null); + System.Diagnostics.Debug.Assert(priority >= FilterPriorityMin && priority <= FilterPriorityMax); + + Filter = filter; + Priority = priority; + // marshal the git_filter strucutre into native memory FilterPointer = Marshal.AllocHGlobal(Marshal.SizeOf(filter.GitFilter)); Marshal.StructureToPtr(filter.GitFilter, FilterPointer, false); + + // register the filter with the native libary + Proxy.git_filter_register(filter.Name, FilterPointer, priority); + } + /// + /// Finalizer called by the , deregisters and frees native memory associated with the registered filter in libgit2. + /// + ~FilterRegistration() + { + // deregister the filter + GlobalSettings.DeregisterFilter(this); + // clean up native allocations + Free(); } + /// + /// Gets if the registration and underlying filter are valid. + /// + public bool IsValid { get { return !freed; } } + /// + /// The registerd filters + /// + public readonly Filter Filter; /// /// The name of the filter in the libgit2 registry /// - public string Name { get; private set; } + public string Name { get { return Filter.Name; } } + /// + /// The priority of the registered filter + /// + public readonly int Priority; + + private readonly IntPtr FilterPointer; - internal IntPtr FilterPointer { get; private set; } + private bool freed; internal void Free() { - Marshal.FreeHGlobal(FilterPointer); - FilterPointer = IntPtr.Zero; + if (!freed) + { + // unregister the filter with the native libary + Proxy.git_filter_unregister(Filter.Name); + // release native memory + Marshal.FreeHGlobal(FilterPointer); + // remember to not do this twice + freed = true; + } } } } diff --git a/LibGit2Sharp/GlobalSettings.cs b/LibGit2Sharp/GlobalSettings.cs index d312b3a43..2689fea05 100644 --- a/LibGit2Sharp/GlobalSettings.cs +++ b/LibGit2Sharp/GlobalSettings.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.IO; using System.Reflection; using LibGit2Sharp.Core; @@ -11,6 +12,7 @@ namespace LibGit2Sharp public static class GlobalSettings { private static readonly Lazy version = new Lazy(Version.Build); + private static readonly Dictionary registeredFilters; private static LogConfiguration logConfiguration = LogConfiguration.None; @@ -24,6 +26,8 @@ static GlobalSettings() string managedPath = new Uri(Assembly.GetExecutingAssembly().EscapedCodeBase).LocalPath; nativeLibraryPath = Path.Combine(Path.GetDirectoryName(managedPath), "NativeBinaries"); } + + registeredFilters = new Dictionary(); } /// @@ -170,6 +174,20 @@ internal static string GetAndLockNativeLibraryPath() return nativeLibraryPath; } + /// + /// Takes a snapshot of the currently registered filters. + /// + /// An array of . + public static IEnumerable GetRegisteredFilters() + { + lock (registeredFilters) + { + FilterRegistration[] array = new FilterRegistration[registeredFilters.Count]; + registeredFilters.Values.CopyTo(array, 0); + return array; + } + } + /// /// Register a filter globally with a default priority of 200 allowing the custom filter /// to imitate a core Git filter driver. It will be run last on checkout and first on checkin. @@ -180,28 +198,80 @@ public static FilterRegistration RegisterFilter(Filter filter) } /// - /// Register a filter globally with given priority for execution. - /// A filter with the priority of 200 will be run last on checkout and first on checkin. - /// A filter with the priority of 0 will be run first on checkout and last on checkin. + /// Registers a to be invoked when matches .gitattributes 'filter=name' /// + /// The filter to be invoked at run time. + /// The priroty of the filter to invoked. + /// A value of 0 () will be run first on checkout and last on checkin. + /// A value of 200 () will be run last on checkout and first on checkin. + /// + /// A object used to manage the lifetime of the registration. public static FilterRegistration RegisterFilter(Filter filter, int priority) { - var registration = new FilterRegistration(filter); + Ensure.ArgumentNotNull(filter, "filter"); + if (priority < FilterRegistration.FilterPriorityMin || priority > FilterRegistration.FilterPriorityMax) + { + throw new ArgumentOutOfRangeException("priority", + priority, + String.Format(System.Globalization.CultureInfo.InvariantCulture, + "Filter priorities must be within the inclusive range of [{0}, {1}].", + FilterRegistration.FilterPriorityMin, + FilterRegistration.FilterPriorityMax)); + } + + FilterRegistration registration = null; - Proxy.git_filter_register(filter.Name, registration, priority); + lock (registeredFilters) + { + // if the filter has already been registered + if (registeredFilters.ContainsKey(filter)) + { + throw new EntryExistsException("The filter has already been registered.", GitErrorCode.Exists, GitErrorCategory.Filter); + } + + // allocate the registration object + registration = new FilterRegistration(filter, priority); + // add the filter and registration object to the global tracking list + registeredFilters.Add(filter, registration); + } return registration; } /// - /// Remove the filter from the registry, and frees the native heap allocation. + /// Unregisters the associated filter. /// + /// Registration object with an associated filter. public static void DeregisterFilter(FilterRegistration registration) { Ensure.ArgumentNotNull(registration, "registration"); - Proxy.git_filter_unregister(registration.Name); - registration.Free(); + lock (registeredFilters) + { + var filter = registration.Filter; + + // do nothing if the filter isn't registered + if (registeredFilters.ContainsKey(filter)) + { + // remove the register from the global tracking list + registeredFilters.Remove(filter); + // clean up native allocations + registration.Free(); + } + } + } + + internal static void DeregisterFilter(Filter filter) + { + System.Diagnostics.Debug.Assert(filter != null); + + // do nothing if the filter isn't registered + if (registeredFilters.ContainsKey(filter)) + { + var registration = registeredFilters[filter]; + // unregister the filter + DeregisterFilter(registration); + } } } }