fix(lock): heap OOB write in __lock_vec + eliminate the lock-mode enumeration class (#140)

.github/workflows/cocci.yml

@@ -6,6 +6,8 @@
 #                   rule_*.cocci convention checks and fails ONLY on NEW
 #                   violations vs the committed baseline (dist/cocci/baseline.txt).
 #                   This is a lint-style gate, NOT an ABI guarantee.
+#                   It ALSO runs the lock-mode inventory (see below), which is
+#                   an absolute, non-baselined gate.
 #
 #   abi-diff     -- libabigail (abidiff) + nm are the AUTHORITATIVE binary-ABI
 #                   check (SHARED-struct layout, public symbols).  Advisory
@@ -56,6 +58,20 @@ jobs:
           ../dist/configure >/tmp/configure.log 2>&1 || { tail -40 /tmp/configure.log; exit 1; }
           test -f db.h && test -f db_int.h
 
+      # Lock-mode enumeration inventory (issue #140's root-cause class).
+      # BLOCKING and NOT baselined: adding a DB_LOCK_* mode to db_lockmode_t,
+      # renaming an inventoried enumeration site, or dropping a case arm from a
+      # switch declared exhaustive all fail here.  Coccinelle cannot express
+      # "switch missing a case" in this spatch build, so this script -- not
+      # rule_lock_mode_enum.cocci -- is the authority for that shape.
+      # See rfc/0003/lock-mode-audit.md.
+      - name: Lock-mode inventory (blocking)
+        run: |
+          sh dist/cocci/lockmode_inventory.sh || {
+            echo "::error::Lock-mode enumeration inventory is out of date. A new DB_LOCK_* mode (or a renamed/changed enumeration site) must be reviewed against every site in dist/cocci/lockmode_inventory.txt -- see rfc/0003/lock-mode-audit.md."
+            exit 1
+          }
+
       - name: Run convention rules
         run: |
           sh dist/cocci/run_conventions.sh > /tmp/cocci-current.txt
@@ -248,3 +264,30 @@ jobs:
                 owner: context.repo.owner, repo: context.repo.repo,
                 issue_number: prNumber, body });
             }
+
+  # ---------------------------------------------------------------------------
+  # Lock-mode memory-safety regression gate (GitHub issue #140).  BLOCKING.
+  #
+  # Builds an AddressSanitizer libdb and runs test/c/chk.locksireads, whose
+  # trigger is a DB_TXN_SNAPSHOT txn holding BOTH a write lock and a
+  # DB_LOCK_SIREAD marker committed on a replication master.  Before the fix
+  # that was a heap-buffer-overflow WRITE in __lock_vec (the SIREAD lock
+  # consumed a DBT slot the nwrites-based sizing never allocated) plus a commit
+  # lock list that omitted the modified page.  The gate asserts both: no ASan
+  # fault, and that the serialized commit lock list names the written page.
+  #
+  # This is the empirical half of the guard; the static half (Coccinelle rules +
+  # the mode inventory) runs in the `conventions` job above.  See
+  # rfc/0003/lock-mode-audit.md.
+  # ---------------------------------------------------------------------------
+  lock-mode-asan:
+    name: lock-mode asan regression (#140)
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Install clang
+        run: sudo apt-get update && sudo apt-get install -y clang
+      - name: Run the ASan lock-mode gate
+        run: timeout 2400 sh test/c/chk.locksireads
+        env:
+          CC: clang

dist/cocci/baseline.txt

@@ -1,3 +1,8 @@
+rule_lock_mode_enum|LOCK_MODE_READTEST|src/db/db_meta.c|else if (lockp->mode == DB_LOCK_READ_UNCOMMITTED )
+rule_lock_mode_enum|LOCK_MODE_READTEST|src/lock/lock.c|} else if (lock_mode == DB_LOCK_READ_UNCOMMITTED )
+rule_lock_mode_enum|LOCK_MODE_READTEST|src/lock/lock.c|lock_mode == DB_LOCK_READ_UNCOMMITTED )
+rule_lock_mode_enum|LOCK_MODE_READTEST|src/lock/lock.c|lock_mode == DB_LOCK_READ_UNCOMMITTED ) {
+rule_lock_mode_enum|LOCK_MODE_READTEST|src/lock/lock.c|lp->mode == DB_LOCK_READ_UNCOMMITTED ) {
 rule_mutex_unbalanced|MUTEX_UNBALANCED|src/btree/bt_curadj.c|return (ret);
 rule_mutex_unbalanced|MUTEX_UNBALANCED|src/crypto/mersenne/mt19937db.c|return (ret);
 rule_mutex_unbalanced|MUTEX_UNBALANCED|src/mp/mp_alloc.c|return (ret);

dist/cocci/lockmode_inventory.sh

@@ -0,0 +1,131 @@
+#!/bin/sh
+# lockmode_inventory.sh -- CHECKED inventory of every lock-MODE enumeration
+# site in libdb, plus the set of DB_LOCK_* modes declared in src/dbinc/db.in.
+#
+# WHY (GitHub issue #140): SSI added DB_LOCK_SIREAD=9 to db_lockmode_t without
+# revisiting the pre-existing sites that enumerate lock modes exhaustively.  One
+# of them -- __lock_vec's DB_LOCK_PUT_READ path -- sized a descriptor array from
+# sh_locker->nwrites while its population loop named the read modes by hand;
+# SIREAD matched neither, fell through, and wrote past the allocation (a heap
+# overflow in every release build, the only bounds check being a DIAGNOSTIC-only
+# DB_ASSERT), and truncated the replication commit lock list.
+#
+# This script is the AUTHORITATIVE guard for that class, because Coccinelle
+# cannot express "a switch over db_lockmode_t that is missing a case" in the
+# spatch build we use (`... when != case X:` inside a switch is a parse error).
+# dist/cocci/rule_lock_mode_enum.cocci covers the two expression-level shapes;
+# this covers the enum itself and the switch sites.
+#
+# WHAT IT CHECKS (all three are hard failures):
+#
+#  1. MODE SET.  The db_lockmode_t members in src/dbinc/db.in must exactly equal
+#     the committed list in dist/cocci/lockmode_inventory.txt.  Add a mode =>
+#     this fails => you must walk the SITES list below and record a verdict for
+#     each, then update the inventory in the same commit.
+#
+#  2. SITES.  Every file:function recorded in the inventory must still exist.
+#     A site that is renamed away silently would otherwise drop out of review.
+#
+#  3. EXHAUSTIVE SWITCHES.  Every switch marked `exhaustive` in the inventory
+#     must contain a case arm for EVERY mode in the mode set.  This is the check
+#     that #140's class cannot evade: a new mode with no arm fails CI.
+#
+# Usage:  sh dist/cocci/lockmode_inventory.sh [repo-root]
+#         sh dist/cocci/lockmode_inventory.sh --print   (emit the current mode
+#                                                        set, for updating .txt)
+set -eu
+
+if [ "${1:-}" = "--print" ]; then
+	shift
+	PRINT=1
+else
+	PRINT=0
+fi
+ROOT="${1:-$(pwd)}"
+cd "$ROOT"
+
+DBIN=src/dbinc/db.in
+INV=dist/cocci/lockmode_inventory.txt
+
+# ---------------------------------------------------------------------------
+# The declared mode set: the DB_LOCK_* members of the db_lockmode_t enum.
+# Anchored on the typedef block so the unrelated DB_LOCK_* #defines above it
+# (detection policies) and the db_lockop_t enum below it are not picked up.
+# ---------------------------------------------------------------------------
+modes() {
+	awk '
+	  /^typedef enum \{/ { inenum = 1; next }
+	  inenum && /\} db_lockmode_t;/ { exit }
+	  inenum && match($0, /DB_LOCK_[A-Z_]+=[0-9]+/) {
+		s = substr($0, RSTART, RLENGTH)
+		sub(/=.*/, "", s)
+		print s
+	  }
+	' "$DBIN" | sort -u
+}
+
+if [ "$PRINT" = 1 ]; then
+	modes
+	exit 0
+fi
+
+[ -f "$INV" ] || { echo "FAIL: missing $INV" >&2; exit 1; }
+
+rc=0
+
+# ---- 1. mode set vs inventory ---------------------------------------------
+modes > /tmp/lmi-actual.$$
+awk '$1 == "mode" { print $2 }' "$INV" | sort -u > /tmp/lmi-recorded.$$
+if ! cmp -s /tmp/lmi-actual.$$ /tmp/lmi-recorded.$$; then
+	echo "FAIL: db_lockmode_t in $DBIN differs from $INV"
+	echo "  added (in db.in, not in inventory):"
+	comm -23 /tmp/lmi-actual.$$ /tmp/lmi-recorded.$$ | sed 's/^/    /'
+	echo "  removed (in inventory, not in db.in):"
+	comm -13 /tmp/lmi-actual.$$ /tmp/lmi-recorded.$$ | sed 's/^/    /'
+	echo "  => A new lock mode must be reviewed against EVERY 'site' line in"
+	echo "     $INV (see rfc/0003/lock-mode-audit.md), then recorded here."
+	rc=1
+fi
+
+# ---- 2. every recorded site still exists ----------------------------------
+# site <path> <function> <verdict> <note...>
+awk '$1 == "site" { print $2 "\t" $3 }' "$INV" |
+while IFS="$(printf '\t')" read -r path fn; do
+	[ -f "$path" ] || { echo "FAIL: inventory site file gone: $path"; echo x >> /tmp/lmi-fail.$$; continue; }
+	# K&R definition: the function name at column 0 followed by '('.
+	grep -q "^$fn(" "$path" || grep -q "^$fn(" "$path" ||
+	  { echo "FAIL: inventory site function gone: $fn in $path"; echo x >> /tmp/lmi-fail.$$; }
+done
+
+# ---- 3. switches declared exhaustive cover every mode ---------------------
+# exhaustive <path> <function>
+awk '$1 == "exhaustive" { print $2 "\t" $3 }' "$INV" |
+while IFS="$(printf '\t')" read -r path fn; do
+	[ -f "$path" ] || { echo "FAIL: exhaustive-switch file gone: $path"; echo x >> /tmp/lmi-fail.$$; continue; }
+	# Body = from the K&R definition line to the next line that is exactly '}'.
+	body=$(awk -v fn="$fn" '
+	  $0 ~ "^" fn "\\(" { inf = 1 }
+	  inf { print }
+	  inf && /^\}/ { exit }
+	' "$path")
+	if [ -z "$body" ]; then
+		echo "FAIL: exhaustive-switch function gone: $fn in $path"
+		echo x >> /tmp/lmi-fail.$$
+		continue
+	fi
+	while read -r m; do
+		printf '%s\n' "$body" | grep -q "case[ 	]*$m[ 	]*:" || {
+			echo "FAIL: $path:$fn (declared exhaustive) has no 'case $m:'"
+			echo "  => add an arm for $m, or drop the 'exhaustive' marker in $INV."
+			echo x >> /tmp/lmi-fail.$$
+		}
+	done < /tmp/lmi-actual.$$
+done
+
+[ -f /tmp/lmi-fail.$$ ] && rc=1
+rm -f /tmp/lmi-actual.$$ /tmp/lmi-recorded.$$ /tmp/lmi-fail.$$
+
+if [ "$rc" = 0 ]; then
+	echo "lock-mode inventory: OK ($(awk '$1=="mode"' "$INV" | wc -l | tr -d ' ') modes, $(awk '$1=="site"' "$INV" | wc -l | tr -d ' ') sites, $(awk '$1=="exhaustive"' "$INV" | wc -l | tr -d ' ') exhaustive switches)"
+fi
+exit "$rc"

dist/cocci/lockmode_inventory.txt

@@ -0,0 +1,107 @@
+# lockmode_inventory.txt -- the CHECKED inventory of DB_LOCK_* mode enumeration
+# sites in libdb.  Enforced by dist/cocci/lockmode_inventory.sh in CI
+# (.github/workflows/cocci.yml).  Rationale + the reviewer checklist:
+# rfc/0003/lock-mode-audit.md.
+#
+# Adding a lock mode to db_lockmode_t (src/dbinc/db.in) FAILS CI until you
+# (a) walk every `site` line below and record a verdict, and (b) add a `mode`
+# line here.  Every `exhaustive` switch must gain a case arm for the new mode.
+#
+# Line formats
+#   mode        <DB_LOCK_NAME>
+#   site        <path> <function> <verdict> <note>
+#   exhaustive  <path> <function>            # switch must cover EVERY mode
+#
+# Verdicts
+#   writelock-predicate  Decides read-vs-write via IS_WRITELOCK(); correct for
+#                        any future mode by construction.  PREFERRED SHAPE.
+#   mode-specific        Deliberately about ONE named mode; a new mode does not
+#                        belong in it.  Correct as written.
+#   exhaustive           Enumerates the whole mode set; a new mode needs an arm.
+#   conflict-matrix      Indexed by mode value; a new mode needs a matrix row
+#                        AND column (see the note on the row).
+
+# --- the mode set (must match db_lockmode_t in src/dbinc/db.in exactly) ------
+mode	DB_LOCK_IREAD
+mode	DB_LOCK_IWR
+mode	DB_LOCK_IWRITE
+mode	DB_LOCK_NG
+mode	DB_LOCK_READ
+mode	DB_LOCK_READ_UNCOMMITTED
+mode	DB_LOCK_SIREAD
+mode	DB_LOCK_WAIT
+mode	DB_LOCK_WRITE
+mode	DB_LOCK_WWRITE
+
+# --- lock manager: the release / accounting paths ---------------------------
+# FIXED for #140.  Both the objlist SIZING pass and the populate branch now key
+# on IS_WRITELOCK, so they agree by construction; __lock_fix_list is handed the
+# count actually populated, and a runtime bounds check (not a DIAGNOSTIC-only
+# DB_ASSERT) fails the op rather than overrunning the heap.
+site	src/lock/lock.c	__lock_vec	writelock-predicate	objlist sizing+population+fix_list count all keyed on IS_WRITELOCK (issue #140); the RELEASE condition still names read modes deliberately -- SIREAD must be retained here for __lock_sicommit
+# nwrites/nlocks accounting: all keyed on IS_WRITELOCK, so SIREAD is correctly
+# counted in nlocks and never in nwrites.
+site	src/lock/lock.c	__lock_get_internal	writelock-predicate	nwrites++ under IS_WRITELOCK; SIREAD handled by its own safe_si arms
+site	src/lock/lock.c	__lock_freelock	writelock-predicate	nlocks/nwrites decrement under IS_WRITELOCK
+site	src/lock/lock.c	__lock_downgrade	writelock-predicate	nwrites-- only on write->non-write transition
+site	src/lock/lock.c	__lock_inherit_locks	writelock-predicate	parent nlocks/nwrites under IS_WRITELOCK
+site	src/lock/lock.c	__lock_trade	writelock-predicate	new locker nlocks/nwrites under IS_WRITELOCK
+# Mode-specific by design: SIREAD markers live on obj->sireaders, every other
+# mode on obj->holders, so removal MUST branch on the mode.
+site	src/lock/lock.c	__lock_put_internal	mode-specific	SIREAD removed from ->sireaders, all other modes from ->holders
+site	src/lock/lock.c	__lock_sicommit	mode-specific	walks heldby for SIREAD markers only (SSI commit/abort)
+site	src/lock/lock.c	__lock_siclean_obj	mode-specific	walks obj->sireaders (all entries are SIREAD by construction)
+
+# --- deadlock detector ------------------------------------------------------
+# atype switch is over db_lockdetect policies (DB_LOCK_MINWRITE, ...), NOT over
+# db_lockmode_t; the nlocks/nwrites counts it reads are maintained above.  A new
+# lock MODE needs no change here.
+site	src/lock/lock_deadlock.c	__dd_build	mode-specific	switches on detector atype, not on lock mode; uses nlocks/nwrites
+
+# --- failchk ---------------------------------------------------------------
+# `lip->nlocks == lip->nwrites` means "holds no non-write locks".  SIREAD is
+# counted in nlocks and not in nwrites, so a locker holding SIREAD markers is
+# correctly treated as holding non-write locks and is NOT skipped.  It then goes
+# down the DB_LOCK_PUT_READ path, which now retains SIREAD (they are released by
+# __lock_sicommit / DB_LOCK_PUT_ALL at txn end) -- correct.
+site	src/lock/lock_failchk.c	__lock_failchk	writelock-predicate	nlocks==nwrites test; SIREAD counts in nlocks only
+
+# --- conflict matrix -------------------------------------------------------
+# db_riw_conflicts is DB_LOCK_RIW_N x DB_LOCK_RIW_N, indexed by mode value; the
+# SI row/column exist and are all-zero (a SIREAD marker never blocks and is
+# never blocked -- SSI detects conflicts by walking obj->sireaders, not via the
+# matrix).  A new mode MUST bump DB_LOCK_RIW_N and add a row AND a column.
+# __lock_get_internal validates lock_mode < region->nmodes, so a mode added
+# without a matrix row is rejected at runtime rather than reading out of bounds.
+site	src/lock/lock_region.c	__lock_region_init	conflict-matrix	db_riw_conflicts is 10x10 incl. the SI row/column; DB_LOCK_RIW_N must track db_lockmode_t
+
+# --- lock lists (serialization for replication / prepare) ------------------
+# Mode-agnostic: operate on DBT object descriptors, never on modes.  The caller
+# (__lock_vec, above) decides WHICH locks enter the list; __lock_get_list
+# reacquires them all in one caller-supplied mode.
+site	src/lock/lock_list.c	__lock_fix_list	mode-specific	serializes DBT objects; mode-agnostic
+site	src/lock/lock_list.c	__lock_get_list	mode-specific	reacquires listed objects in the caller's mode (DB_LOCK_WRITE from rep apply)
+
+# --- diagnostics / printing (exhaustive switches) -------------------------
+# FIXED for #140's class: both printers previously fell through to
+# "UNKNOWN"/"UNKNOWN LOCK MODE" for SIREAD.  Now exhaustive and CI-enforced.
+exhaustive	src/lock/lock_stat.c	__lock_printlock
+exhaustive	src/db/db_pr.c	__db_lockmode_to_string
+# FIXED for #140's class: walked only holders+waiters, so an object pinned only
+# by SIREAD markers printed as empty.  Now walks ->sireaders too.
+site	src/lock/lock_stat.c	__lock_dump_object	mode-specific	walks holders, waiters AND sireaders
+site	src/lock/lock_stat.c	__lock_dump_locker	mode-specific	walks heldby; mode printed by __lock_printlock
+
+# --- access-method lock decisions ----------------------------------------
+# Deliberately mode-specific: these choose a mode to REQUEST and decide lock
+# coupling for the mode actually granted.  SIREAD is produced here (from
+# DB_LOCK_READ under MULTIVERSION+TXN_SNAPSHOT) and is intentionally excluded
+# from the couple/downgrade tests -- an SSI read marker must be held to txn end.
+site	src/db/db_meta.c	__db_lget	mode-specific	converts READ->SIREAD under MULTIVERSION+SSI; coupling tests name single modes
+site	src/db/db_meta.c	__db_lput	mode-specific	couple/downgrade decisions per named mode; SIREAD retained to txn end
+
+# --- txn event (handle-lock) trades --------------------------------------
+# IS_WRITELOCK on a HANDLE lock (DB_LOCK_READ or DB_LOCK_WRITE on a database
+# handle).  Handle locks are never SIREAD (that conversion is page-read only),
+# so the predicate is correct and future-proof.
+site	src/txn/txn_util.c	__txn_doevents	writelock-predicate	IS_WRITELOCK on handle locks; handle locks are never SIREAD

dist/cocci/rule_lock_mode_enum.cocci

@@ -0,0 +1,66 @@
+/*
+ * rule_lock_mode_enum.cocci -- flag the two lock-mode patterns that produced
+ * GitHub issue #140.
+ *
+ * WHY: SSI added DB_LOCK_SIREAD=9 to db_lockmode_t without auditing the
+ * pre-existing loops that enumerate lock modes exhaustively.  In __lock_vec's
+ * DB_LOCK_PUT_READ path the descriptor array was sized from
+ * sh_locker->nwrites, while the population loop released only the modes it
+ * named by hand (DB_LOCK_READ, DB_LOCK_READ_UNCOMMITTED).  DB_LOCK_SIREAD
+ * matched neither those names nor IS_WRITELOCK, so it fell through to the
+ * populate branch and wrote a DBT past the allocation -- a heap overflow in
+ * every release build (the only bounds check was a DIAGNOSTIC-only DB_ASSERT)
+ * plus a truncated, wrong replication commit lock list.
+ *
+ * Two shapes are flagged.  Neither is automatically a bug; the point is that
+ * adding a lock mode must be a deliberate visit to each one.  Sites judged
+ * correct live in dist/cocci/baseline.txt, so a NEW match fails CI
+ * (.github/workflows/cocci.yml).  The complementary, exhaustive check -- which
+ * catches mode SWITCHES too, and which fires when a mode is added to
+ * src/dbinc/db.in at all -- is dist/cocci/lockmode_inventory.sh; Coccinelle
+ * cannot express "switch statement missing a case" in this spatch build (a
+ * `... when != case X:` inside a switch is a parse error), so the inventory is
+ * the authority for that shape.  See rfc/0003/lock-mode-audit.md.
+ *
+ *   //@LOCK_MODE_SIZING@   An allocation size computed from ->nwrites.  A
+ *                          write-lock counter must never size a buffer that a
+ *                          mode-enumerating loop then fills: the two can
+ *                          disagree.  Count with the SAME predicate the loop
+ *                          uses (see __lock_vec) so they agree by
+ *                          construction.  Expected: ZERO matches.
+ *
+ *   //@LOCK_MODE_READTEST@ A hand-written read-mode test naming
+ *                          DB_LOCK_READ_UNCOMMITTED.  Such a list is silently
+ *                          incomplete the moment another non-write mode
+ *                          exists (DB_LOCK_SIREAD did exactly this).  Where
+ *                          the intent is "every non-write mode", prefer
+ *                          !IS_WRITELOCK(m), which covers present and future
+ *                          modes.  Where the intent really is that one mode
+ *                          (e.g. the lock-coupling decisions in db_meta.c),
+ *                          the site is correct -- baseline it.
+ *
+ * Source-level EARLY WARNING / convention check -- see README.md.  Written as
+ * identity transforms because @script:python@ does not work in this spatch
+ * build; the produced diff IS the report.
+ */
+
+/*
+ * 1. Allocation sized from a write-lock counter.
+ */
+@lock_mode_sizing@
+expression e;
+type T;
+@@
+- e->nwrites * sizeof(T)
++ e->nwrites * sizeof(T) //@LOCK_MODE_SIZING@
+
+/*
+ * 2. Hand-enumerated read mode.  Matched one comparison at a time: BDB's
+ *    conditions are long `||` chains, which parse left-associated, so a
+ *    two-operand pattern would not match a subchain.
+ */
+@lock_mode_readtest@
+expression m;
+@@
+- m == DB_LOCK_READ_UNCOMMITTED
++ m == DB_LOCK_READ_UNCOMMITTED //@LOCK_MODE_READTEST@