diff --git a/src/SQLitePCLRaw.core/handles.cs b/src/SQLitePCLRaw.core/handles.cs index 0973a6de..e7a73ebc 100644 --- a/src/SQLitePCLRaw.core/handles.cs +++ b/src/SQLitePCLRaw.core/handles.cs @@ -28,6 +28,7 @@ namespace SQLitePCL using System; using System.Collections.Concurrent; using System.Runtime.InteropServices; + using System.Threading; public class sqlite3_backup : SafeHandle { @@ -346,24 +347,29 @@ internal void remove_stmt(sqlite3_stmt s) public T GetOrCreateExtra(Func f) where T : class, IDisposable { - if (extra != null) + // Audit #7: atomic check-and-set so two threads racing the first hook registration cannot both create T and silently drop one container's callbacks/GCHandles. + var existing = Volatile.Read(ref extra); + if (existing != null) { - return (T)extra; + return (T)existing; } - else + var candidate = f(); + var previous = Interlocked.CompareExchange(ref extra, candidate, null); + if (previous != null) { - var q = f(); - extra = q; - return q; + candidate.Dispose(); + return (T)previous; } + return candidate; } private void dispose_extra() { - if (extra != null) + // Audit #8: pair Interlocked.Exchange with GetOrCreateExtra's CAS so disposal and creation don't race on a half-disposed object. + var snapshot = Interlocked.Exchange(ref extra, null); + if (snapshot != null) { - extra.Dispose(); - extra = null; + snapshot.Dispose(); } } diff --git a/src/common/tests_xunit.cs b/src/common/tests_xunit.cs index 2124a929..1d84b700 100644 --- a/src/common/tests_xunit.cs +++ b/src/common/tests_xunit.cs @@ -349,6 +349,44 @@ public void test_prepare_tail_string() } } + // Audit #7: GetOrCreateExtra used to allow two threads racing the first + // call to both create T, with one quietly winning extra and the loser's + // instance silently leaking. The patched CAS path guarantees a single + // shared instance is returned to all callers. + sealed class extra_marker : IDisposable + { + public bool disposed; + public void Dispose() { disposed = true; } + } + + [Fact] + public void test_get_or_create_extra_returns_same_instance_under_contention() + { + using (var db = ugly.open(":memory:")) + { + var n_threads = Math.Max(4, Environment.ProcessorCount); + var instances = new extra_marker[n_threads]; + var threads = new System.Threading.Thread[n_threads]; + var start = new System.Threading.ManualResetEventSlim(); + for (var i = 0; i < n_threads; i++) + { + var idx = i; + threads[i] = new System.Threading.Thread(() => + { + start.Wait(); + instances[idx] = db.GetOrCreateExtra(() => new extra_marker()); + }); + threads[i].Start(); + } + start.Set(); + foreach (var t in threads) t.Join(); + + Assert.NotNull(instances[0]); + for (var i = 1; i < n_threads; i++) + { + Assert.Same(instances[0], instances[i]); + } + } // Regression test for the ArgumentOutOfRangeException variant of the // long-standing prepare-race bug class (see issues #108, #321, #430, // #479, #588). The provider's span overload of sqlite3_prepare_v2