From 5d70b411f31ed31b1354e95431d4d8a956770bf5 Mon Sep 17 00:00:00 2001
From: James Cater <j_cater@hotmail.com>
Date: Fri, 24 Apr 2026 14:41:22 +0100
Subject: [PATCH] handles.cs: Interlocked CAS for sqlite3 extra slot
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two related fixes around the sqlite3.extra slot that holds hook-container
state (commit hooks, update hooks, etc.):

Audit #7 — GetOrCreateExtra used to do a plain check-and-set:
    if (extra != null) return (T)extra;
    else { var q = f(); extra = q; return q; }
Two threads racing the first hook registration can both observe extra
== null, both create T, and both write into extra.  One write wins;
the other container holds GCHandles for delegates the db no longer
tracks.  When the db closes, only the winning container is disposed —
the loser's GCHandles leak, and if SQLite still holds the loser's
native function pointer, it outlives managed tracking and can be
invoked on a freed handle.  Replace with Volatile.Read + Interlocked.
CompareExchange; dispose the losing candidate on a CAS miss.

Audit #8 — dispose_extra used a non-atomic null-and-dispose:
    if (extra != null) { extra.Dispose(); extra = null; }
Pair it with Interlocked.Exchange so disposal and a concurrent
creation path can't race on a half-swapped field.

Adds test_get_or_create_extra_returns_same_instance_under_contention
which spawns N threads racing GetOrCreateExtra and asserts every
caller gets the same instance back.

Note: this branch is independent of the PR currently open for the
span-overload pzTail fix (fix/prepare-v2-pztail-bounds-check).  The
new test compiles fine but main's tests.csproj currently fails to
build due to pre-existing test-infrastructure drift that the other
PR's test refactor also fixes; the two will need to land together
for CI to go green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 src/SQLitePCLRaw.core/handles.cs | 24 ++++++++++++-------
 src/common/tests_xunit.cs        | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 9 deletions(-)

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<T>(Func<T> 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 7bb69d66..18dcbbe7 100644
--- a/src/common/tests_xunit.cs
+++ b/src/common/tests_xunit.cs
@@ -349,6 +349,46 @@ 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]);
+                }
+            }
+        }
+
         [Fact]
         public void test_bind_parameter_index()
         {