Skip to content

Commit 62ad2c6

Browse files
Close TSFN post-then-release race in JSSynchronizationContext
JSTsfnSynchronizationContext.Post/Send checked IsDisposed and then called the thread-safe function via NonBlockingCall. The environment cleanup hook could run Dispose (releasing the TSFN) between the check and the call, causing a native use-after-release that a managed try/catch cannot guard. Introduce TsfnCallGate, an interlocked gate that reference-counts in-flight NonBlockingCall invocations. Post/Send wrap each native call in TryEnter/Exit; Dispose calls Close before releasing the TSFN. Close atomically rejects new entrants and then waits for in-flight calls to drain, guaranteeing no native call is in progress and none can start once the TSFN is released. The gate is held only around the native call, not around Send's completion wait, so TSFN release cannot deadlock against a callback running on the JS thread. Expose internals to the test assembly so the gate's concurrency behavior can be unit-tested directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7
1 parent ee3e750 commit 62ad2c6

4 files changed

Lines changed: 257 additions & 8 deletions

File tree

‎src/NodeApi/Interop/JSSynchronizationContext.cs‎

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,10 @@ internal sealed unsafe class JSTsfnSynchronizationContext : JSSynchronizationCon
254254
private readonly JSRuntime _runtime;
255255
private readonly napi_env _env;
256256
private readonly JSThreadSafeFunction _tsfn;
257+
258+
// Coordinates in-flight NonBlockingCall invocations with TSFN release during teardown, so that
259+
// a poster can never call into the TSFN after (or while) it is being released.
260+
private readonly TsfnCallGate _callGate = new();
257261
private GCHandle _cleanupHandle;
258262

259263
public JSTsfnSynchronizationContext()
@@ -298,6 +302,11 @@ public override void Dispose()
298302

299303
base.Dispose();
300304

305+
// Prevent any new NonBlockingCall from starting and wait for in-flight calls to finish
306+
// before releasing the TSFN. This closes the post-then-release race in which a poster could
307+
// call into the TSFN after the environment cleanup hook has released it.
308+
_callGate.Close();
309+
301310
// Destroy TSFN by releasing last thread use count.
302311
// TSFN is deleted after this point and must not be used.
303312
_tsfn.Release();
@@ -348,9 +357,16 @@ public override void CloseAsyncScope()
348357

349358
public override void Post(SendOrPostCallback callback, object? state)
350359
{
351-
if (IsDisposed) return;
360+
if (!_callGate.TryEnter()) return;
352361

353-
_tsfn.NonBlockingCall(() => callback(state));
362+
try
363+
{
364+
_tsfn.NonBlockingCall(() => callback(state));
365+
}
366+
finally
367+
{
368+
_callGate.Exit();
369+
}
354370
}
355371

356372
public override void Send(SendOrPostCallback callback, object? state)
@@ -361,14 +377,25 @@ public override void Send(SendOrPostCallback callback, object? state)
361377
return;
362378
}
363379

364-
if (IsDisposed) return;
365-
366380
using ManualResetEvent syncEvent = new(false);
367-
_tsfn.NonBlockingCall(() =>
381+
382+
// The gate is held only around the native NonBlockingCall, not around the wait below: the
383+
// posted callback runs on the JS thread and could otherwise deadlock TSFN release.
384+
if (!_callGate.TryEnter()) return;
385+
386+
try
368387
{
369-
callback(state);
370-
syncEvent.Set();
371-
});
388+
_tsfn.NonBlockingCall(() =>
389+
{
390+
callback(state);
391+
syncEvent.Set();
392+
});
393+
}
394+
finally
395+
{
396+
_callGate.Exit();
397+
}
398+
372399
syncEvent.WaitOne();
373400
}
374401
}
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
using System.Threading;
5+
6+
namespace Microsoft.JavaScript.NodeApi.Interop;
7+
8+
/// <summary>
9+
/// Coordinates in-flight native thread-safe-function (TSFN) calls with TSFN release, so that a
10+
/// caller cannot invoke the TSFN after (or while) it is being released.
11+
/// </summary>
12+
/// <remarks>
13+
/// <para>
14+
/// <see cref="JSTsfnSynchronizationContext"/> posts work to the JS thread by calling into a native
15+
/// TSFN, and releases that TSFN from an environment cleanup hook during teardown. Without
16+
/// coordination there is a race: a poster can observe the context as not-yet-disposed, then the
17+
/// cleanup hook can release the TSFN, and then the poster calls into the released TSFN &mdash; a
18+
/// native use-after-release that a managed <c>try/catch</c> cannot guard.
19+
/// </para>
20+
/// <para>
21+
/// This gate closes that window. A caller wraps each native TSFN call in
22+
/// <see cref="TryEnter"/>/<see cref="Exit"/>; the owner calls <see cref="Close"/> before releasing
23+
/// the TSFN. <see cref="Close"/> atomically prevents any further <see cref="TryEnter"/> from
24+
/// succeeding and then waits for all in-flight calls to finish, guaranteeing that no native call
25+
/// is in progress and none can start once <see cref="Close"/> returns.
26+
/// </para>
27+
/// </remarks>
28+
internal sealed class TsfnCallGate
29+
{
30+
// High bit marks the gate as closed; the remaining bits count in-flight calls. Because Enter
31+
// never increments once the closed bit is set, and Exit is only called after a successful
32+
// Enter, the count part never borrows into the closed bit.
33+
private const int ClosedFlag = unchecked((int)0x80000000);
34+
private const int CountMask = 0x7FFFFFFF;
35+
36+
private int _state;
37+
38+
/// <summary>
39+
/// Gets a value indicating whether the gate has been closed.
40+
/// </summary>
41+
public bool IsClosed => (Volatile.Read(ref _state) & ClosedFlag) != 0;
42+
43+
/// <summary>
44+
/// Attempts to enter the gate for a single native call. When this returns true the caller must
45+
/// pair it with exactly one call to <see cref="Exit"/> once the native call has returned.
46+
/// </summary>
47+
/// <returns>True if the call may proceed; false if the gate is closed and the call must be
48+
/// skipped.</returns>
49+
public bool TryEnter()
50+
{
51+
int state = Volatile.Read(ref _state);
52+
while ((state & ClosedFlag) == 0)
53+
{
54+
int updated = Interlocked.CompareExchange(ref _state, state + 1, state);
55+
if (updated == state)
56+
{
57+
return true;
58+
}
59+
60+
state = updated;
61+
}
62+
63+
return false;
64+
}
65+
66+
/// <summary>
67+
/// Marks completion of a native call previously admitted by <see cref="TryEnter"/>.
68+
/// </summary>
69+
public void Exit() => Interlocked.Decrement(ref _state);
70+
71+
/// <summary>
72+
/// Closes the gate so that no further calls can enter, then blocks until all in-flight calls
73+
/// have exited. After this returns it is safe to release the underlying TSFN. Calling this more
74+
/// than once is safe.
75+
/// </summary>
76+
public void Close()
77+
{
78+
int state = Volatile.Read(ref _state);
79+
while ((state & ClosedFlag) == 0)
80+
{
81+
int updated = Interlocked.CompareExchange(
82+
ref _state, state | ClosedFlag, state);
83+
if (updated == state)
84+
{
85+
break;
86+
}
87+
88+
state = updated;
89+
}
90+
91+
SpinWait spin = default;
92+
while ((Volatile.Read(ref _state) & CountMask) != 0)
93+
{
94+
spin.SpinOnce();
95+
}
96+
}
97+
}

‎src/NodeApi/NodeApi.csproj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@
2828
<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" />
2929
</ItemGroup>
3030

31+
<ItemGroup>
32+
<InternalsVisibleTo Include="Microsoft.JavaScript.NodeApi.Test" />
33+
</ItemGroup>
34+
3135
<Target Name="SetRuntimeConfigValues" BeforeTargets="GenerateBuildRuntimeConfigurationFiles">
3236
<ItemGroup>
3337
<RuntimeHostConfigurationOption Include="System.Runtime.InteropServices.EnableConsumingManagedCodeFromNativeHosting" Value="true" />

‎test/TsfnCallGateTests.cs‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
using System;
5+
using System.Threading;
6+
using System.Threading.Tasks;
7+
using Microsoft.JavaScript.NodeApi.Interop;
8+
using Xunit;
9+
10+
namespace Microsoft.JavaScript.NodeApi.Test;
11+
12+
public class TsfnCallGateTests
13+
{
14+
[Fact]
15+
public void EnterAndExitWhileOpen()
16+
{
17+
var gate = new TsfnCallGate();
18+
19+
Assert.True(gate.TryEnter());
20+
gate.Exit();
21+
22+
Assert.False(gate.IsClosed);
23+
}
24+
25+
[Fact]
26+
public void CloseRejectsFurtherEntry()
27+
{
28+
var gate = new TsfnCallGate();
29+
30+
gate.Close();
31+
32+
Assert.True(gate.IsClosed);
33+
Assert.False(gate.TryEnter());
34+
}
35+
36+
[Fact]
37+
public void CloseIsIdempotent()
38+
{
39+
var gate = new TsfnCallGate();
40+
41+
gate.Close();
42+
gate.Close();
43+
44+
Assert.True(gate.IsClosed);
45+
Assert.False(gate.TryEnter());
46+
}
47+
48+
// Close must not return while a call is in flight: it has to wait for the corresponding Exit,
49+
// guaranteeing no native TSFN call is in progress once the TSFN is released.
50+
[Fact]
51+
public void CloseWaitsForInFlightCallToExit()
52+
{
53+
var gate = new TsfnCallGate();
54+
55+
// Simulate an in-flight native call that has entered but not yet exited.
56+
Assert.True(gate.TryEnter());
57+
58+
Task closeTask = Task.Run(() => gate.Close());
59+
60+
// Close cannot complete while the call is in flight.
61+
Assert.False(closeTask.Wait(TimeSpan.FromMilliseconds(200)));
62+
63+
// Once the in-flight call exits, Close completes.
64+
gate.Exit();
65+
Assert.True(closeTask.Wait(TimeSpan.FromSeconds(5)));
66+
67+
// After closing, no further calls are admitted.
68+
Assert.False(gate.TryEnter());
69+
}
70+
71+
// Once Close has set the closed flag, an entry that races with it must be rejected, so the
72+
// in-flight count cannot rise again after Close begins draining.
73+
[Fact]
74+
public void EntryDoesNotSucceedAfterCloseFlagSet()
75+
{
76+
var gate = new TsfnCallGate();
77+
78+
gate.Close();
79+
80+
// A burst of concurrent entry attempts after Close must all fail.
81+
Parallel.For(0, 1000, _ => Assert.False(gate.TryEnter()));
82+
}
83+
84+
// Stress the gate with concurrent enter/exit callers while another thread closes it, then
85+
// assert the invariant that after Close returns no caller is inside the gate and none can
86+
// enter.
87+
[Fact]
88+
public void ConcurrentCallersDrainBeforeCloseCompletes()
89+
{
90+
var gate = new TsfnCallGate();
91+
using var start = new ManualResetEventSlim(false);
92+
93+
Task[] callers = new Task[8];
94+
for (int i = 0; i < callers.Length; i++)
95+
{
96+
callers[i] = Task.Run(() =>
97+
{
98+
start.Wait();
99+
for (int j = 0; j < 5000; j++)
100+
{
101+
if (gate.TryEnter())
102+
{
103+
// Represents a brief native call.
104+
Thread.SpinWait(10);
105+
gate.Exit();
106+
}
107+
}
108+
});
109+
}
110+
111+
start.Set();
112+
113+
// Close concurrently with the callers; it must wait for any in-flight call to exit.
114+
gate.Close();
115+
116+
// After Close returns, no caller may enter and the callers finish without error.
117+
Assert.False(gate.TryEnter());
118+
Assert.True(Task.WaitAll(callers, TimeSpan.FromSeconds(30)));
119+
Assert.True(gate.IsClosed);
120+
}
121+
}

0 commit comments

Comments
 (0)