diff --git a/.github/workflows/cocci.yml b/.github/workflows/cocci.yml index fa24fe657..49ba46437 100644 --- a/.github/workflows/cocci.yml +++ b/.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 diff --git a/dist/cocci/baseline.txt b/dist/cocci/baseline.txt index d80f32759..c7bab5fbb 100644 --- a/dist/cocci/baseline.txt +++ b/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); diff --git a/dist/cocci/lockmode_inventory.sh b/dist/cocci/lockmode_inventory.sh new file mode 100755 index 000000000..878e48806 --- /dev/null +++ b/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 +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 +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" diff --git a/dist/cocci/lockmode_inventory.txt b/dist/cocci/lockmode_inventory.txt new file mode 100644 index 000000000..7b08cdea8 --- /dev/null +++ b/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 +# site +# exhaustive # 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 diff --git a/dist/cocci/rule_lock_mode_enum.cocci b/dist/cocci/rule_lock_mode_enum.cocci new file mode 100644 index 000000000..858db5947 --- /dev/null +++ b/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@ diff --git a/rfc/0003/lock-mode-audit.md b/rfc/0003/lock-mode-audit.md new file mode 100644 index 000000000..eca1bacd9 --- /dev/null +++ b/rfc/0003/lock-mode-audit.md @@ -0,0 +1,169 @@ +# Lock-mode audit: adding a `DB_LOCK_*` mode + +Status: enforced (CI) +Applies to: `db_lockmode_t` in `src/dbinc/db.in` + +## Why this note exists + +GitHub issue #140 was a heap out-of-bounds write in `__lock_vec` and, following +from it, a possible transaction-isolation violation on a replication client. +Neither was a bug in the SSI logic. Both were a bug in the *pre-existing* code +that SSI walked past: RFC 0003 added `DB_LOCK_SIREAD=9` to a 30-year-old enum +without revisiting every site that enumerates lock modes exhaustively. + +The concrete failure, in `__lock_vec`'s `DB_LOCK_PUT_READ` handling: + +- The temporary `DBT` descriptor array — which becomes the replication commit + lock list — was **sized** from `sh_locker->nwrites`. +- The loop that **populated** it released (and thus skipped) only the modes it + named by hand: `DB_LOCK_READ` and `DB_LOCK_READ_UNCOMMITTED`. +- `DB_LOCK_SIREAD` matched neither of those names *nor* `IS_WRITELOCK`. So a + retained SIREAD marker fell through to the populate branch and consumed a + descriptor slot the sizing had never allocated. +- The only bounds check was a `DB_ASSERT`, which compiles out of every + non-`DIAGNOSTIC` build. Release builds corrupted the heap in silence. +- `__lock_fix_list` was then handed `nwrites` rather than the number of + descriptors actually written, truncating the serialized list. Because newly + granted locks go to the **head** of the locker's `heldby` list, the SIREAD + object was visited first and could displace the modified page's write-lock + object. `__rep_process_txn` reacquires only the listed objects as write locks, + so apply could change a page a separate client transaction still read-locked. + +The generalisable lesson is not "we forgot SIREAD". It is: + +> **A write-lock counter must never size a buffer that a mode-enumerating loop +> then fills.** Sizing and population must derive from the *same* predicate, so +> they agree by construction rather than by coincidence. + +## The shape to write + +Derive the sizing pass and the populate branch from the **same** predicate. +Express that predicate as `IS_WRITELOCK(m)` rather than as a list of mode names: + +```c +/* GOOD -- sizing and population cannot disagree, for any future mode. */ +nobj = 0; +SH_LIST_FOREACH(lp, &sh_locker->heldby, locker_links, __db_lock) + if (IS_WRITELOCK(lp->mode)) + nobj++; +objlist->size = nobj * sizeof(DBT); +... +if (objlist != NULL && IS_WRITELOCK(lp->mode)) + ... populate ... +``` + +```c +/* BAD -- an independent counter sizes what a mode enumeration fills. */ +objlist->size = sh_locker->nwrites * sizeof(DBT); +... +if (writes == 1 || lp->mode == DB_LOCK_READ || + lp->mode == DB_LOCK_READ_UNCOMMITTED) + ... release ... +if (objlist != NULL) /* everything else falls in here */ + ... populate ... +``` + +Note what the fix did **not** change: the set of modes `DB_LOCK_PUT_READ` +releases. SIREAD markers must stay on `heldby` past this point so +`__lock_sicommit` can persist or drop them at `__txn_end`; releasing them here +would break SSI. The bug was never "SIREAD is retained" — it was that a retained +non-write lock silently entered a list sized only for write locks. So the fix +narrows the *populate* condition (and the sizing to match) and leaves the +*release* condition alone. + +Where the intent genuinely is one specific mode (the lock-coupling decisions in +`__db_lput`, the SIREAD-vs-holders list choice in `__lock_put_internal`), naming +the mode is correct. Record that judgement in the inventory rather than +"fixing" it. + +Two further rules: + +- **Bound the write at runtime, not just under `DIAGNOSTIC`.** If a mismatch + would be a memory-safety bug, `DB_ASSERT` is not a bounds check — it is + documentation. `__lock_vec` now fails the operation via `__env_panic` instead. +- **Pass the count you produced.** Hand downstream serializers the number of + entries actually populated (`np - (DBT *)objlist->data`), never an + independently maintained counter that "should" match. + +## Checklist: adding a lock mode + +CI will not let you skip this — `dist/cocci/lockmode_inventory.sh` fails as soon +as a new `DB_LOCK_*` member appears in `db_lockmode_t`. To make it pass you must: + +1. **Conflict matrix.** Bump `DB_LOCK_RIW_N` and add both a **row and a column** + to `db_riw_conflicts` (`src/lock/lock_region.c`). The matrix is indexed by + mode value; a missing row is caught at runtime by the + `lock_mode >= region->nmodes` check in `__lock_get_internal`, but only after + the mode is already in use. Decide explicitly whether the new mode conflicts + with each existing one. (SIREAD's row and column are all-zero: a marker never + blocks and is never blocked, because SSI detects conflicts by walking + `obj->sireaders`, not through the matrix.) + +2. **Read-vs-write classification.** Decide whether the mode is an + `IS_WRITELOCK` (`src/dbinc/lock.h`). This one decision propagates to + `nlocks`/`nwrites` accounting, `DB_LOCK_PUT_READ`, `__lock_failchk`'s + `nlocks == nwrites` test, the deadlock detector's `MINWRITE`/`MAXWRITE` + policies, and `__txn_doevents`' handle-lock trades. If the answer is "neither + exactly" — as it is for SIREAD — say so in the inventory note and check that + every consumer of the classification does the right thing with it. + +3. **Which list does it live on?** Every mode but SIREAD lives on + `obj->holders`. If the new mode needs its own list, audit every walker of + `holders`/`waiters` (`__lock_promote`, `__lock_put_internal`, + `__lock_dump_object`, `__dd_build`) for whether it must also walk yours. + +4. **Lifetime.** When is the lock released? SIREAD markers deliberately outlive + `DB_LOCK_PUT_READ` (they are handled by `__lock_sicommit` just before + `DB_LOCK_PUT_ALL` at `__txn_end`), which is exactly why they were on the + `heldby` list at the moment `__lock_vec` built the commit lock list. + +5. **Exhaustive switches.** Add a `case` arm to every switch marked + `exhaustive` in the inventory (currently `__lock_printlock` and + `__db_lockmode_to_string`). CI checks this mechanically. + +6. **Every `site` line.** Walk them all and record a verdict. That is the + deliverable: the inventory is an *auditable* record, not a list of files + someone glanced at. + +7. **Regression test.** Add a case to `test/c/test_lock_sireads.c` (or a sibling) + that puts the new mode on a locker's `heldby` list at the same time as a write + lock, and run it under ASan via `test/c/chk.locksireads`. #140 was invisible + to the entire existing suite because the suite had no test that held two + different lock modes across a commit-lock-list build. + +## What CI enforces + +| Guard | File | Failure mode | +| --- | --- | --- | +| New mode in `db_lockmode_t` not recorded | `dist/cocci/lockmode_inventory.sh` | hard fail, not baselined | +| Inventoried enumeration site renamed/removed | same | hard fail | +| `exhaustive` switch missing a `case` for any mode | same | hard fail | +| Allocation sized from `->nwrites` | `dist/cocci/rule_lock_mode_enum.cocci` (`LOCK_MODE_SIZING`) | new violation vs `baseline.txt` | +| Hand-enumerated read-mode test | same (`LOCK_MODE_READTEST`) | new violation vs `baseline.txt` | +| `__lock_vec` overrun at runtime | `src/lock/lock.c` | `__env_panic` in every build, not just `DIAGNOSTIC` | +| The #140 overflow itself | `test/c/chk.locksireads` | ASan heap-buffer-overflow | + +Both Coccinelle rules are wired into the existing baseline gate, so they fail on +*new* matches while the sites judged correct stay recorded in +`dist/cocci/baseline.txt`. The inventory script is **not** baselined: it is +absolute. + +### Known limitation + +Coccinelle cannot express "a `switch` over `db_lockmode_t` that is missing a +`case`" in the spatch build this repo uses — `... when != case X:` inside a +`switch` is a parse error (spatch 1.3.1). That is why the exhaustive-switch check +lives in `lockmode_inventory.sh` (awk over the function body) rather than in +SmPL. The Coccinelle rules cover the two expression-level shapes, where they are +a genuinely better fit than grep. + +## Audit performed for #140 + +See the PR for the full table. Summary: 19 enumeration sites inspected across +`src/lock/`, `src/db/`, and `src/txn/`. One real memory-safety bug +(`__lock_vec`, fixed). Two real reporting bugs (`__lock_printlock` and +`__db_lockmode_to_string` both printed SIREAD as `UNKNOWN`; `__lock_dump_object` +did not walk `obj->sireaders`, so an object pinned only by markers printed as +empty) — fixed. The remaining 15 sites were judged correct, and *why* is recorded +per-site in `dist/cocci/lockmode_inventory.txt` so the judgement can be +re-checked rather than re-derived. diff --git a/src/db/db_pr.c b/src/db/db_pr.c index 4eba08809..a898f1493 100644 --- a/src/db/db_pr.c +++ b/src/db/db_pr.c @@ -357,6 +357,8 @@ __db_lockmode_to_string(mode) return ("Read uncommitted"); case DB_LOCK_WWRITE: return ("Was written"); + case DB_LOCK_SIREAD: + return ("Snapshot isolation read"); default: break; } diff --git a/src/lock/lock.c b/src/lock/lock.c index 595283896..1e7d2005b 100644 --- a/src/lock/lock.c +++ b/src/lock/lock.c @@ -333,7 +333,7 @@ __lock_vec(env, sh_locker, flags, list, nlist, elistp) DB_LOCKREGION *region; DB_LOCKTAB *lt; DBT *objlist, *np; - u_int32_t ndx; + u_int32_t ndx, nobj; int did_abort, i, ret, run_dd, upgrade, writes; /* Check if locks have been globally turned off. */ @@ -398,9 +398,27 @@ __lock_vec(env, sh_locker, flags, list, nlist, elistp) * We know these should be ilocks, * but they could be something else, * so allocate room for the size too. + * + * Size from the SAME predicate the populate + * branch below uses -- one slot per retained + * write lock -- so sizing and population agree + * by construction. Do NOT size from + * sh_locker->nwrites: that counter only counts + * write locks whose status is DB_LSTAT_HELD, + * and it says nothing about the non-write + * modes this loop retains (DB_LOCK_SIREAD, + * DB_LOCK_IREAD, DB_LOCK_WAIT, ...), which used + * to fall through and consume uncounted + * slots -- a heap overflow (issue #140). */ - objlist->size = - sh_locker->nwrites * sizeof(DBT); + nobj = 0; + if (writes != 1) + SH_LIST_FOREACH(lp, + &sh_locker->heldby, + locker_links, __db_lock) + if (IS_WRITELOCK(lp->mode)) + nobj++; + objlist->size = nobj * sizeof(DBT); if ((ret = __os_malloc(env, objlist->size, &objlist->data)) != 0) goto up_done; @@ -450,10 +468,51 @@ __lock_vec(env, sh_locker, flags, list, nlist, elistp) break; continue; } - if (objlist != NULL) { - DB_ASSERT(env, (u_int8_t *)np < - (u_int8_t *)objlist->data + - objlist->size); + /* + * MODE ENUMERATION (keep in sync with the + * sizing pass above, and with IS_WRITELOCK in + * dbinc/lock.h): + * + * The replication commit lock list is consumed + * by __rep_process_txn, which reacquires every + * listed object as DB_LOCK_WRITE before apply. + * It must therefore contain EXACTLY the write + * locks this transaction retains. A retained + * non-write mode (DB_LOCK_SIREAD -- an SSI read + * marker, DB_LOCK_IREAD, DB_LOCK_WAIT, ...) is + * not a write lock and must not enter the list: + * before this test existed such a lock consumed + * a descriptor slot that the sizing never + * allocated (heap overflow) and, because newly + * granted locks go to the head of heldby, could + * displace a modified page's write-lock object + * from the truncated list -- letting apply + * change a page a client still read-locks + * (issue #140). + * + * Non-write locks retained here are released + * later by the DB_LOCK_PUT_ALL in __txn_end; + * SIREAD markers are handled just before it by + * __lock_sicommit, which is why they must be + * RETAINED (not released) on this path. + */ + if (objlist != NULL && IS_WRITELOCK(lp->mode)) { + /* + * Runtime bounds check, not a + * DB_ASSERT: DB_ASSERT compiles out of + * every non-DIAGNOSTIC build, which is + * precisely where a sizing/population + * skew would corrupt the heap in + * silence. Fail loudly instead. + */ + if ((u_int8_t *)(np + 1) > + (u_int8_t *)objlist->data + + objlist->size) { + __db_errx(env, DB_STR("2056", + "Lock list overflow")); + ret = __env_panic(env, EINVAL); + break; + } np->data = SH_DBT_PTR(&sh_obj->lockobj); np->size = sh_obj->lockobj.size; np++; @@ -462,10 +521,18 @@ __lock_vec(env, sh_locker, flags, list, nlist, elistp) if (ret != 0) goto up_done; - if (objlist != NULL) + /* + * Serialize exactly the descriptors populated above -- + * not sh_locker->nwrites, which is an independently + * maintained counter and so could truncate the list or + * read uninitialized slots. + */ + if (objlist != NULL) { + nobj = (u_int32_t)(np - (DBT *)objlist->data); if ((ret = __lock_fix_list(env, - objlist, sh_locker->nwrites)) != 0) + objlist, nobj)) != 0) goto up_done; + } switch (list[i].op) { case DB_LOCK_UPGRADE_WRITE: /* diff --git a/src/lock/lock_stat.c b/src/lock/lock_stat.c index de5987f0b..ecabc9ad8 100644 --- a/src/lock/lock_stat.c +++ b/src/lock/lock_stat.c @@ -606,6 +606,13 @@ __lock_dump_object(lt, mbp, op) __lock_printlock(lt, mbp, lp, 1); SH_TAILQ_FOREACH(lp, &op->waiters, links, __db_lock) __lock_printlock(lt, mbp, lp, 1); + /* + * SSI SIREAD markers live on their own list, not on holders: a mode + * enumeration that walks only holders/waiters reports the object as + * having no locks while markers still pin it (and pin their lockers). + */ + SH_TAILQ_FOREACH(lp, &op->sireaders, links, __db_lock) + __lock_printlock(lt, mbp, lp, 1); return (0); } @@ -678,6 +685,9 @@ __lock_printlock(lt, mbp, lp, ispgno) case DB_LOCK_WAIT: mode = "WAIT"; break; + case DB_LOCK_SIREAD: + mode = "SIREAD"; + break; default: mode = "UNKNOWN"; break; diff --git a/test/c/chk.locksireads b/test/c/chk.locksireads new file mode 100755 index 000000000..21b7b7c99 --- /dev/null +++ b/test/c/chk.locksireads @@ -0,0 +1,136 @@ +#!/bin/sh +# chk.locksireads -- ASan regression gate for GitHub issue #140. +# +# Builds an AddressSanitizer-instrumented libdb (reusing the fuzz gate's +# build_asan_gate/ mechanism -- see test/fuzz/check-crashes.sh) and runs +# test/c/test_lock_sireads.c twice: +# +# --trigger DB_TXN_SNAPSHOT txn on a DB_MULTIVERSION btree holding BOTH a +# write lock and a DB_LOCK_SIREAD marker, committed on a +# replication master (the only path that builds a commit lock +# list). Before the fix this was a heap-buffer-overflow WRITE in +# __lock_vec (the SIREAD lock consumed a DBT descriptor slot that +# the nwrites-based sizing never allocated) AND produced a commit +# lock list that omitted the modified page. Must be clean. +# +# --control The same transaction with the read removed: one write lock, no +# SIREAD marker, no overflow. Clean before AND after the fix; it +# proves the SIREAD lock is what triggers the bug. +# +# We assert BOTH: no ASan fault, and -- for the trigger -- that the commit lock +# list actually names the page the transaction modified (the isolation half of +# #140: a SIREAD object must never displace a write-lock object, because the +# replication apply path reacquires only the listed objects as write locks). +# +# Run from test/c/ inside a `nix develop` shell (or any shell with clang). +# Usage: sh test/c/chk.locksireads +# Env: CC (default clang), LIBDB_ASAN_BUILD (reuse an existing ASan tree) + +set -eu + +HERE=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +ROOT=$(CDPATH= cd -- "$HERE/../.." && pwd) +CC=${CC:-clang} +GATE=${LIBDB_ASAN_BUILD:-$ROOT/build_asan_gate} +WORK=$(mktemp -d) +trap 'rm -f -r "$WORK" 2>/dev/null || true' EXIT + +# ---- 1. ASan libdb (same recipe as test/fuzz/check-crashes.sh) ------------- +if [ ! -f "$GATE/libdb.a" ]; then + echo "building ASan libdb in $GATE (this takes a few minutes)..." + mkdir -p "$GATE" + ( cd "$GATE" && + ../dist/configure --enable-debug \ + CC="$CC" CFLAGS="-fsanitize=address -g -O1" >configure.log 2>&1 && + make -j"$(nproc 2>/dev/null || echo 4)" >build.log 2>&1 ) || { + echo "FAIL: could not build an ASan libdb (see $GATE/build.log)" >&2 + exit 1 + } +fi + +# liburing is linked in when the ASan tree autodetected io_uring. +URING= +grep -q '^#define[ ]*HAVE_IO_URING' "$GATE/db_config.h" 2>/dev/null && + URING=-luring + +echo "compiling test_lock_sireads against $GATE" +"$CC" -g -O1 -fsanitize=address -fno-omit-frame-pointer -I "$GATE" \ + "$HERE/test_lock_sireads.c" "$GATE/libdb.a" $URING -lpthread -ldl \ + -o "$WORK/test_lock_sireads" + +# ---- 2. run both variants ------------------------------------------------- +rc=0 +run() { # run