Skip to content

Commit cf1d405

Browse files
author
J Wyman
committed
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.
1 parent d1e43bd commit cf1d405

File tree

5 files changed

+207
-56
lines changed

5 files changed

+207
-56
lines changed

‎LibGit2Sharp.Tests/FilterFixture.cs

Lines changed: 54 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using System;
22
using System.Collections.Generic;
3+
using System.Linq;
34
using System.IO;
45
using LibGit2Sharp.Tests.TestHelpers;
56
using Xunit;
@@ -26,7 +27,7 @@ public void CanRegisterFilterWithSingleAttribute()
2627
[Fact]
2728
public void CanRegisterAndUnregisterTheSameFilter()
2829
{
29-
var filter = new EmptyFilter(FilterName + 1, attributes);
30+
var filter = new EmptyFilter(FilterName, attributes);
3031

3132
var registration = GlobalSettings.RegisterFilter(filter);
3233
GlobalSettings.DeregisterFilter(registration);
@@ -38,8 +39,7 @@ public void CanRegisterAndUnregisterTheSameFilter()
3839
[Fact]
3940
public void CanRegisterAndDeregisterAfterGarbageCollection()
4041
{
41-
var filter = new EmptyFilter(FilterName + 2, attributes);
42-
var filterRegistration = GlobalSettings.RegisterFilter(filter);
42+
var filterRegistration = GlobalSettings.RegisterFilter(new EmptyFilter(FilterName, attributes));
4343

4444
GC.Collect();
4545

@@ -49,7 +49,7 @@ public void CanRegisterAndDeregisterAfterGarbageCollection()
4949
[Fact]
5050
public void SameFilterIsEqual()
5151
{
52-
var filter = new EmptyFilter(FilterName + 3, attributes);
52+
var filter = new EmptyFilter(FilterName, attributes);
5353
Assert.Equal(filter, filter);
5454
}
5555

@@ -62,16 +62,16 @@ public void InitCallbackNotMadeWhenFilterNeverUsed()
6262
called = true;
6363
};
6464

65-
var filter = new FakeFilter(FilterName + 11, attributes,
66-
successCallback,
67-
successCallback,
68-
initializeCallback);
69-
70-
var filterRegistration = GlobalSettings.RegisterFilter(filter);
65+
var filter = new FakeFilter(FilterName,
66+
attributes,
67+
successCallback,
68+
successCallback,
69+
initializeCallback);
70+
var registration = GlobalSettings.RegisterFilter(filter);
7171

7272
Assert.False(called);
7373

74-
GlobalSettings.DeregisterFilter(filterRegistration);
74+
GlobalSettings.DeregisterFilter(registration);
7575
}
7676

7777
[Fact]
@@ -83,12 +83,12 @@ public void InitCallbackMadeWhenUsingTheFilter()
8383
called = true;
8484
};
8585

86-
var filter = new FakeFilter(FilterName + 12, attributes,
87-
successCallback,
88-
successCallback,
89-
initializeCallback);
90-
91-
var filterRegistration = GlobalSettings.RegisterFilter(filter);
86+
var filter = new FakeFilter(FilterName,
87+
attributes,
88+
successCallback,
89+
successCallback,
90+
initializeCallback);
91+
var registration = GlobalSettings.RegisterFilter(filter);
9292
Assert.False(called);
9393

9494
string repoPath = InitNewRepository();
@@ -98,7 +98,7 @@ public void InitCallbackMadeWhenUsingTheFilter()
9898
Assert.True(called);
9999
}
100100

101-
GlobalSettings.DeregisterFilter(filterRegistration);
101+
GlobalSettings.DeregisterFilter(registration);
102102
}
103103

104104
[Fact]
@@ -112,17 +112,17 @@ public void WhenStagingFileApplyIsCalledWithCleanForCorrectPath()
112112
called = true;
113113
reader.CopyTo(writer);
114114
};
115-
var filter = new FakeFilter(FilterName + 15, attributes, clean);
116115

117-
var filterRegistration = GlobalSettings.RegisterFilter(filter);
116+
var filter = new FakeFilter(FilterName, attributes, clean);
117+
var registration = GlobalSettings.RegisterFilter(filter);
118118

119119
using (var repo = CreateTestRepository(repoPath))
120120
{
121121
StageNewFile(repo);
122122
Assert.True(called);
123123
}
124124

125-
GlobalSettings.DeregisterFilter(filterRegistration);
125+
GlobalSettings.DeregisterFilter(registration);
126126
}
127127

128128
[Fact]
@@ -135,22 +135,20 @@ public void CleanFilterWritesOutputToObjectTree()
135135

136136
Action<Stream, Stream> cleanCallback = SubstitutionCipherFilter.RotateByThirteenPlaces;
137137

138-
var filter = new FakeFilter(FilterName + 16, attributes, cleanCallback);
139-
140-
var filterRegistration = GlobalSettings.RegisterFilter(filter);
138+
var filter = new FakeFilter(FilterName, attributes, cleanCallback);
139+
var registration = GlobalSettings.RegisterFilter(filter);
141140

142141
using (var repo = CreateTestRepository(repoPath))
143142
{
144143
FileInfo expectedFile = StageNewFile(repo, decodedInput);
145144
var commit = repo.Commit("Clean that file");
146-
147145
var blob = (Blob)commit.Tree[expectedFile.Name].Target;
148146

149147
var textDetected = blob.GetContentText();
150148
Assert.Equal(encodedInput, textDetected);
151149
}
152150

153-
GlobalSettings.DeregisterFilter(filterRegistration);
151+
GlobalSettings.DeregisterFilter(registration);
154152
}
155153

156154
[Fact]
@@ -164,16 +162,16 @@ public void WhenCheckingOutAFileFileSmudgeWritesCorrectFileToWorkingDirectory()
164162

165163
Action<Stream, Stream> smudgeCallback = SubstitutionCipherFilter.RotateByThirteenPlaces;
166164

167-
var filter = new FakeFilter(FilterName + 17, attributes, null, smudgeCallback);
168-
var filterRegistration = GlobalSettings.RegisterFilter(filter);
165+
var filter = new FakeFilter(FilterName, attributes, null, smudgeCallback);
166+
var registration = GlobalSettings.RegisterFilter(filter);
169167

170168
FileInfo expectedFile = CheckoutFileForSmudge(repoPath, branchName, encodedInput);
171169

172170
string combine = Path.Combine(repoPath, "..", expectedFile.Name);
173171
string readAllText = File.ReadAllText(combine);
174172
Assert.Equal(decodedInput, readAllText);
175173

176-
GlobalSettings.DeregisterFilter(filterRegistration);
174+
GlobalSettings.DeregisterFilter(registration);
177175
}
178176

179177
[Fact]
@@ -186,8 +184,8 @@ public void CanFilterLargeFiles()
186184

187185
string repoPath = InitNewRepository();
188186

189-
var filter = new FileExportFilter("exportFilter", attributes);
190-
var filterRegistration = GlobalSettings.RegisterFilter(filter);
187+
var filter = new FileExportFilter(FilterName, attributes);
188+
var registration = GlobalSettings.RegisterFilter(filter);
191189

192190
string filePath = Path.Combine(Directory.GetParent(repoPath).Parent.FullName, Guid.NewGuid().ToString() + ".blob");
193191
FileInfo contentFile = new FileInfo(filePath);
@@ -233,7 +231,31 @@ public void CanFilterLargeFiles()
233231

234232
contentFile.Delete();
235233

236-
GlobalSettings.DeregisterFilter(filterRegistration);
234+
GlobalSettings.DeregisterFilter(registration);
235+
}
236+
237+
[Fact]
238+
public void DoubleRegistrationFailsButDoubleDeregistrationDoesNot()
239+
{
240+
Assert.Equal(0, GlobalSettings.GetRegisteredFilters().Count());
241+
242+
var filter = new EmptyFilter(FilterName, attributes);
243+
var registration = GlobalSettings.RegisterFilter(filter);
244+
245+
Assert.Throws<EntryExistsException>(() => { GlobalSettings.RegisterFilter(filter); });
246+
Assert.Equal(1, GlobalSettings.GetRegisteredFilters().Count());
247+
248+
Assert.True(registration.IsValid, "FilterRegistration.IsValid should be true.");
249+
250+
GlobalSettings.DeregisterFilter(registration);
251+
Assert.Equal(0, GlobalSettings.GetRegisteredFilters().Count());
252+
253+
Assert.False(registration.IsValid, "FilterRegistration.IsValid should be false.");
254+
255+
GlobalSettings.DeregisterFilter(registration);
256+
Assert.Equal(0, GlobalSettings.GetRegisteredFilters().Count());
257+
258+
Assert.False(registration.IsValid, "FilterRegistration.IsValid should be false.");
237259
}
238260

239261
private unsafe bool CharArrayAreEqual(char[] array1, char[] array2, int count)

‎LibGit2Sharp/Core/Proxy.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -809,12 +809,12 @@ public static GitDiffDelta git_diff_get_delta(DiffSafeHandle diff, int idx)
809809

810810
#region git_filter_
811811

812-
public static void git_filter_register(string name, FilterRegistration filterRegistration, int priority)
812+
public static void git_filter_register(string name, IntPtr filterPtr, int priority)
813813
{
814-
int res = NativeMethods.git_filter_register(name, filterRegistration.FilterPointer, priority);
814+
int res = NativeMethods.git_filter_register(name, filterPtr, priority);
815815
if (res == (int)GitErrorCode.Exists)
816816
{
817-
var message = string.Format("A filter with the name '{0}' is already registered", name);
817+
var message = String.Format("A filter with the name '{0}' is already registered", name);
818818
throw new EntryExistsException(message);
819819
}
820820
Ensure.ZeroResult(res);

‎LibGit2Sharp/Filter.cs

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,10 @@ namespace LibGit2Sharp
1515
public abstract class Filter : IEquatable<Filter>
1616
{
1717
private static readonly LambdaEqualityHelper<Filter> equalityHelper =
18-
new LambdaEqualityHelper<Filter>(x => x.Name, x => x.Attributes);
18+
new LambdaEqualityHelper<Filter>(x => x.Name, x => x.Attributes);
1919
// 64K is optimal buffer size per https://technet.microsoft.com/en-us/library/cc938632.aspx
2020
private const int BufferSize = 64 * 1024;
2121

22-
private readonly string name;
23-
private readonly IEnumerable<FilterAttributeEntry> attributes;
24-
25-
private readonly GitFilter gitFilter;
26-
2722
/// <summary>
2823
/// Initializes a new instance of the <see cref="Filter"/> class.
2924
/// And allocates the filter natively.
@@ -46,6 +41,18 @@ protected Filter(string name, IEnumerable<FilterAttributeEntry> attributes)
4641
stream = StreamCreateCallback,
4742
};
4843
}
44+
/// <summary>
45+
/// Finalizer called by the <see cref="GC"/>, deregisters and frees native memory associated with the registered filter in libgit2.
46+
/// </summary>
47+
~Filter()
48+
{
49+
GlobalSettings.DeregisterFilter(this);
50+
}
51+
52+
private readonly string name;
53+
private readonly IEnumerable<FilterAttributeEntry> attributes;
54+
private readonly GitFilter gitFilter;
55+
private readonly object @lock = new object();
4956

5057
private GitWriteStream thisStream;
5158
private GitWriteStream nextStream;

‎LibGit2Sharp/FilterRegistration.cs

Lines changed: 59 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,26 +9,78 @@ namespace LibGit2Sharp
99
/// </summary>
1010
public sealed class FilterRegistration
1111
{
12-
internal FilterRegistration(Filter filter)
12+
/// <summary>
13+
/// Maximum priority value a filter can have. A value of 200 will be run last on checkout and first on checkin.
14+
/// </summary>
15+
public const int FilterPriorityMax = 200;
16+
/// <summary>
17+
/// Minimum priority value a filter can have. A value of 0 will be run first on checkout and last on checkin.
18+
/// </summary>
19+
public const int FilterPriorityMin = 0;
20+
21+
/// <summary>
22+
///
23+
/// </summary>
24+
/// <param name="filter"></param>
25+
/// <param name="priority"></param>
26+
internal FilterRegistration(Filter filter, int priority)
1327
{
14-
Ensure.ArgumentNotNull(filter, "filter");
15-
Name = filter.Name;
28+
System.Diagnostics.Debug.Assert(filter != null);
29+
System.Diagnostics.Debug.Assert(priority >= FilterPriorityMin && priority <= FilterPriorityMax);
30+
31+
Filter = filter;
32+
Priority = priority;
1633

34+
// marshal the git_filter strucutre into native memory
1735
FilterPointer = Marshal.AllocHGlobal(Marshal.SizeOf(filter.GitFilter));
1836
Marshal.StructureToPtr(filter.GitFilter, FilterPointer, false);
37+
38+
// register the filter with the native libary
39+
Proxy.git_filter_register(filter.Name, FilterPointer, priority);
40+
}
41+
/// <summary>
42+
/// Finalizer called by the <see cref="GC"/>, deregisters and frees native memory associated with the registered filter in libgit2.
43+
/// </summary>
44+
~FilterRegistration()
45+
{
46+
// deregister the filter
47+
GlobalSettings.DeregisterFilter(this);
48+
// clean up native allocations
49+
Free();
1950
}
2051

52+
/// <summary>
53+
/// Gets if the registration and underlying filter are valid.
54+
/// </summary>
55+
public bool IsValid { get { return !freed; } }
56+
/// <summary>
57+
/// The registerd filters
58+
/// </summary>
59+
public readonly Filter Filter;
2160
/// <summary>
2261
/// The name of the filter in the libgit2 registry
2362
/// </summary>
24-
public string Name { get; private set; }
63+
public string Name { get { return Filter.Name; } }
64+
/// <summary>
65+
/// The priority of the registered filter
66+
/// </summary>
67+
public readonly int Priority;
68+
69+
private readonly IntPtr FilterPointer;
2570

26-
internal IntPtr FilterPointer { get; private set; }
71+
private bool freed;
2772

2873
internal void Free()
2974
{
30-
Marshal.FreeHGlobal(FilterPointer);
31-
FilterPointer = IntPtr.Zero;
75+
if (!freed)
76+
{
77+
// unregister the filter with the native libary
78+
Proxy.git_filter_unregister(Filter.Name);
79+
// release native memory
80+
Marshal.FreeHGlobal(FilterPointer);
81+
// remember to not do this twice
82+
freed = true;
83+
}
3284
}
3385
}
3486
}

0 commit comments

Comments
 (0)