fix(txn,lock): reclaim SSI reader bookkeeping (three resource leaks)

.github/workflows/test-tiers.yml

@@ -16,11 +16,13 @@
 #     serializability violation appears AND when a scenario marked as
 #     reproducing a known issue stops violating, so a fix cannot land without
 #     updating the expectation.
-#   - B3 is advisory (continue-on-error) for now, because on current master it
-#     legitimately aborts under ASan: that IS the #140 reproduction.  Flip it to
-#     a hard gate in the same PR that fixes #140.
-#   - B2 (soak) is scheduled/nightly plus manual dispatch: it is a long run and
-#     the same expectation-flip applies once #137/#138 land.
+#   - B3 is a HARD GATE.  It was advisory while #140 was open (on that master it
+#     legitimately aborted under ASan -- that abort WAS the #140 reproduction);
+#     #145 fixed the lock-list sizing, so the matrix passes and any future ASan
+#     fault in the lock list is a real regression.
+#   - B2 (soak) is scheduled/nightly plus manual dispatch: it is a long run.  It
+#     was advisory while #137/#138 were open; those are fixed, so when it runs it
+#     is a hard gate.
 
 name: Test tiers (isolation / soak / lock matrix)
 
@@ -89,13 +91,13 @@ jobs:
           if-no-files-found: ignore
 
   # --------------------------------------------------------------------------
-  # Tier B3 -- lock-mode matrix under ASan.  Advisory until #140 lands, because
-  # the ASan heap-buffer-overflow it finds in __lock_vec IS the bug report.
+  # Tier B3 -- lock-mode matrix under ASan.  A HARD GATE since #145 fixed the
+  # __lock_vec lock-list sizing (#140): the matrix passes, so any ASan fault in
+  # the lock list is now a real regression.
   # --------------------------------------------------------------------------
   lock-matrix:
     name: B3 lock-mode matrix (ASan)
     runs-on: ubuntu-latest
-    continue-on-error: true   # advisory until #140 is fixed
     steps:
       - uses: actions/checkout@v4
 
@@ -141,7 +143,6 @@ jobs:
     name: B2 resource-accounting soak
     if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch'
     runs-on: ubuntu-latest
-    continue-on-error: true   # advisory until #137/#138 are fixed
     timeout-minutes: 60
     steps:
       - uses: actions/checkout@v4

dist/Makefile.in

@@ -218,6 +218,9 @@ SIM_OBJS=	sim_core@o@ sim_os_hooks@o@
 # by configure only when --enable-faultinject.  Compile rule below.
 FI_OBJS=	fi_alloc@o@
 
+# SSI resource-accounting leak tests -- test/c/.  Compile rules below.
+LEAK_TESTS=	leak_si_locker leak_si_mvcc_mtx
+
 BTREE_OBJS=\
 	bt_compare@o@ bt_compress@o@ bt_conv@o@ bt_curadj@o@ bt_cursor@o@ \
 	bt_delete@o@ bt_method@o@ bt_open@o@ bt_put@o@ bt_rec@o@ \
@@ -1308,6 +1311,7 @@ mostly-clean clean:
 	$(RM) -r ALL.OUT.* PARALLEL_TESTDIR.*
 	$(RM) -r RUN_LOG RUNQUEUE TESTDIR TESTDIR.A TEST.LIST
 	$(RM) -r logtrack_seen.db test_micro test_mutex .libs
+	$(RM) -r $(LEAK_TESTS)
 	$(RM) -r $(LIB_INSTALL_FILE_LIST)
 	$(RM) compile_commands.json
 	@subdir_cmd@
@@ -1563,6 +1567,30 @@ test_mutex: test_mutex@o@ $(DEF_LIB)
 	$(CCLINK) -o $@ $(LDFLAGS) test_mutex@o@ $(DEF_LIB) $(TEST_LIBS) $(LIBS)
 	$(POSTLINK) $@
 
+##################################################
+# SSI resource-accounting leak tests -- test/c/.
+#
+# Regression gates for the SIREAD cleanup resource leaks (issues #137, #138).
+# They assert through the public statistics APIs that locker / mutex / detail
+# slot counts stay bounded across thousands of sequential snapshot txns.
+# `make leak_tests` builds them; test/c/leak-run.sh builds+runs both modes.
+##################################################
+leak_tests: $(LEAK_TESTS)
+
+leak_si_locker@o@: $(testdir)/c/leak_si_locker.c
+	$(CC) $(CFLAGS) $(DEPFLAGS) $<
+leak_si_locker: leak_si_locker@o@ $(DEF_LIB)
+	$(CCLINK) -o $@ \
+	    $(LDFLAGS) leak_si_locker@o@ $(DEF_LIB) $(TEST_LIBS) $(LIBS)
+	$(POSTLINK) $@
+
+leak_si_mvcc_mtx@o@: $(testdir)/c/leak_si_mvcc_mtx.c
+	$(CC) $(CFLAGS) $(DEPFLAGS) $<
+leak_si_mvcc_mtx: leak_si_mvcc_mtx@o@ $(DEF_LIB)
+	$(CCLINK) -o $@ \
+	    $(LDFLAGS) leak_si_mvcc_mtx@o@ $(DEF_LIB) $(TEST_LIBS) $(LIBS)
+	$(POSTLINK) $@
+
 ##################################################
 # Deterministic Simulation Testing (DST) -- test/sim/.
 #

src/lock/lock.c

@@ -161,8 +161,31 @@ __lock_siclean_obj(env, obj, old_lsnp)
 		 * detail and the (DB_LOCKER_FREED) locker are reclaimed by
 		 * __lock_sicleanup once their last marker is gone.
 		 */
-		if (sh_locker->td_off != INVALID_ROFF)
-			(void)atomic_dec(env, &LOCKER_TD(env, sh_locker)->si_ref);
+		if (sh_locker->td_off != INVALID_ROFF &&
+		    atomic_dec(env,
+		    &LOCKER_TD(env, sh_locker)->si_ref) == 0 &&
+		    F_ISSET(sh_locker, DB_LOCKER_FREED))
+			/*
+			 * We just removed the LAST marker of a locker whose
+			 * reclamation __lock_freelocker_int deferred (that
+			 * deferral is why DB_LOCKER_FREED is set, and it always
+			 * happened already: __txn_end frees the locker before it
+			 * publishes status != TXN_RUNNING, which is what let this
+			 * sweep consider the marker at all).  Nothing references
+			 * the locker any more, so mark it reclaimable by dropping
+			 * its detail link, and let __lock_sireap_lockers free it
+			 * after this partition mutex is released -- freeing a
+			 * locker needs LOCK_LOCKERS, and the established order is
+			 * LOCK_LOCKERS -> partition, never the reverse.
+			 *
+			 * Clearing td_off here (rather than re-reading si_ref
+			 * later) is what makes the reclaim UAF-free: mpool's
+			 * __txn_remove_buffer may free this detail the instant
+			 * si_ref reaches zero, so the reclaim pass must never
+			 * dereference it again.  atomic_dec's return value is the
+			 * last safe observation of the detail.
+			 */
+			sh_locker->td_off = INVALID_ROFF;
 		if (sh_locker->nlocks > 0)
 			sh_locker->nlocks--;
 		if ((ret = __lock_freelock(lt, lp, sh_locker,
@@ -216,6 +239,16 @@ __lock_sicleanup(env)
 		OBJECT_UNLOCK(lt, region, i);
 	}
 
+	/*
+	 * Reclaim committed-reader lockers whose last marker was just removed
+	 * (marked by __lock_siclean_obj), then free the details those markers
+	 * were pinning.  Both are done here, with no object partition mutex
+	 * held: locker frees need LOCK_LOCKERS and detail frees need the txn
+	 * region lock, and the established order puts both outside a partition
+	 * mutex.  Lockers first, so no locker is left naming a freed detail.
+	 */
+	(void)__lock_sireap_lockers(env);
+
 	/*
 	 * Free committed-reader details whose last SIREAD marker was just
 	 * reclaimed above.  __txn_end parked them on the mvcc_txn list
@@ -1030,12 +1063,32 @@ again:	if (obj == NULL) {
 				/*
 				 * Upgrading our own SIREAD to WRITE: drop the
 				 * SIREAD marker to avoid self-conflicts.
+				 *
+				 * Account for the detail reference the marker
+				 * held, exactly as the other two removal sites
+				 * (__lock_sicommit, __lock_siclean_obj) do and
+				 * with the same td_off guard the grant used.
+				 * Without it si_ref stays permanently above the
+				 * true marker count, so the owning detail (and
+				 * the locker deferring on it) can never be
+				 * reclaimed: a snapshot txn that reads then
+				 * writes the same key leaks a detail, a locker
+				 * and their mutex slots on every iteration until
+				 * txn_begin returns ENOMEM.
+				 *
+				 * This is the reader's OWN marker (sh_off ==
+				 * holder), so the detail is its live, running
+				 * transaction: si_ref cannot reach zero here and
+				 * no reclaim can trigger underneath us.
 				 */
 				SH_TAILQ_REMOVE(&sh_obj->sireaders,
 				    sireadlp, links, __db_lock);
 				if (atomic_read_relaxed(&region->nsireaders) > 0)
 					(void)atomic_dec(env,
 					    &region->nsireaders);
+				if (sh_locker->td_off != INVALID_ROFF)
+					(void)atomic_dec(env,
+					    &LOCKER_TD(env, sh_locker)->si_ref);
 				if ((ret = __lock_freelock(lt, sireadlp,
 				    LOCK_HOLDER(env, sireadlp),
 				    DB_LOCK_UNLINK | DB_LOCK_FREE)) != 0)

src/lock/lock_id.c

@@ -544,6 +544,65 @@ __lock_freelocker_int(lt, region, sh_locker, reallyfree)
 	return (0);
 }
 
+/*
+ * __lock_sireap_lockers --
+ *	Free committed-reader (SSI) lockers whose last SIREAD marker has been
+ *	reclaimed.  __lock_siclean_obj marks such a locker while holding the
+ *	object partition mutex, by clearing its td_off once the marker count
+ *	reaches zero; here, with no partition mutex held, we take LOCK_LOCKERS
+ *	and release the locker and its logical mutex.  Without this the
+ *	DB_LOCKER_FREED locker stayed allocated for the life of the environment,
+ *	so sequential read-only snapshot transactions eventually exhausted the
+ *	mutex region (DB_ENV->txn_begin returning ENOMEM).
+ *
+ *	This deliberately dereferences no TXN_DETAIL: mpool may free the detail
+ *	as soon as si_ref reaches zero, so the marker-count observation has to
+ *	happen (and does) in __lock_siclean_obj, not here.
+ *
+ * PUBLIC: int __lock_sireap_lockers __P((ENV *));
+ */
+int
+__lock_sireap_lockers(env)
+	ENV *env;
+{
+	DB_LOCKER *sh_locker, *next_locker;
+	DB_LOCKREGION *region;
+	DB_LOCKTAB *lt;
+	int ret;
+
+	if (!LOCKING_ON(env))
+		return (0);
+	lt = env->lk_handle;
+	region = lt->reginfo.primary;
+	ret = 0;
+
+	LOCK_LOCKERS(env, region);
+	for (sh_locker = SH_TAILQ_FIRST(&region->lockers, __db_locker);
+	    sh_locker != NULL; sh_locker = next_locker) {
+		next_locker = SH_TAILQ_NEXT(sh_locker, ulinks, __db_locker);
+		/*
+		 * (DB_LOCKER_FREED && td_off == INVALID_ROFF) is set only by
+		 * __lock_siclean_obj: a locker whose reclamation was deferred
+		 * for SIREAD markers that are now all gone.  A live locker never
+		 * carries DB_LOCKER_FREED, and a still-deferred one still has
+		 * its td_off.  heldby must be empty (__lock_sicommit detached
+		 * the markers, DB_LOCK_PUT_ALL released everything else) --
+		 * __lock_freelocker_int would return EINVAL rather than free a
+		 * locker with locks, so skip it instead of failing the sweep.
+		 */
+		if (!F_ISSET(sh_locker, DB_LOCKER_FREED) ||
+		    sh_locker->td_off != INVALID_ROFF ||
+		    !SH_LIST_EMPTY(&sh_locker->heldby))
+			continue;
+		if ((ret =
+		    __lock_freelocker_int(lt, region, sh_locker, 1)) != 0)
+			break;
+	}
+	UNLOCK_LOCKERS(env, region);
+
+	return (ret);
+}
+
 /*
  * __lock_freelocker
  *	Remove a locker its family from the hash table.

src/lock/lock_stub.c

@@ -493,6 +493,14 @@ __lock_addfamilylocker(env, pid, id, is_family)
 	return (0);
 }
 
+int
+__lock_sireap_lockers(env)
+	ENV *env;
+{
+	COMPQUIET(env, NULL);
+	return (0);
+}
+
 int
 __lock_freelocker(lt, sh_locker)
 	DB_LOCKTAB *lt;

src/txn/txn.c

@@ -268,10 +268,12 @@ __txn_begin(env, ip, parent, txnpp, flags)
 			 * Trigger the marker sweep when live SIREAD markers pass
 			 * half the allocated lock objects, so the committed-reader
 			 * marker footprint stays bounded instead of growing until
-			 * the next checkpoint.  (Committed-reader locker structs are
-			 * not yet reclaimed -- see the SSI known-issues note.)
-			 * st_objects is always non-zero, so the bound holds whether
-			 * or not a max is configured.
+			 * the next checkpoint.  The sweep also reclaims the
+			 * committed-reader locker and detail structs the markers
+			 * were pinning (__lock_sireap_lockers /
+			 * __txn_reap_si_details), so those do not accumulate
+			 * either.  st_objects is always non-zero, so the bound
+			 * holds whether or not a max is configured.
 			 */
 			u_int32_t nobj = lkreg->stat.st_objects;
 			if (nobj != 0 && atomic_read_relaxed(&lkreg->nsireaders) > nobj / 2)