From 29bb190f2c195156b1c4d8052f248f28ca8a8d5c Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Sun, 6 Sep 2026 23:54:12 -0400 Subject: [PATCH] fix(lock): don't dereference a NULL locker in the priority accessors Fixes #148. DB_ENV->set_lk_priority() and DB_ENV->get_lk_priority() crashed the library on a plain public-API call naming a locker id with no live locker: db_env_create(&e, 0); e->open(e, path, DB_CREATE|DB_INIT_LOCK|DB_INIT_MPOOL|DB_INIT_TXN|DB_INIT_LOG, 0600); e->set_lk_priority(e, 0, 100); /* SIGSEGV */ __lock_getlocker() with create == 0 reports 'no such locker' by returning 0 with a NULL locker rather than by returning an error -- its only assignment to *retp is at lock_id.c:408, after the 'if (sh_locker == NULL && create)' block. Both accessors tested only 'ret == 0' and then dereferenced. gdb: Program received signal SIGSEGV #0 __lock_set_lk_priority (priority=100) at ../src/lock/lock_method.c:483 #1 main () at r.c:13 Both now return EINVAL for a missing locker. The check is deliberately local to these two functions rather than a change to __lock_getlocker()'s contract: __lock_vec_pp() passes the same possibly-NULL locker into __lock_vec(), which handles it on purpose ('it's perfectly reasonable for there to be no locker; this is not an error'), so tightening the shared helper would break a caller that depends on the current behavior. The original report suspected __lock_vec_pp() was affected too; it is not, and I verified that by calling lock_vec() with DB_LOCK_PUT_ALL and DB_LOCK_UPGRADE_WRITE on an unused id -- both return 0. Regression test test/db/lock_priority_nullderef.c covers all three cases: EINVAL for the setter and getter on an unused id, and a set/get round trip on a LIVE locker so the fix cannot regress into simply disabling the feature. Proven to have teeth: reverting the two checks makes the runner report FAIL (rc=139, Segmentation fault). Wired into the CI regression-runner step. Found by the coverage-driver work in #147, which SIGSEGVed on the never-called getter surface. --- .github/workflows/ci.yml | 3 +- src/lock/lock_method.c | 18 ++++- test/db/lock_priority_nullderef.c | 105 +++++++++++++++++++++++++ test/db/run_lock_priority_nullderef.sh | 85 ++++++++++++++++++++ 4 files changed, 208 insertions(+), 3 deletions(-) create mode 100644 test/db/lock_priority_nullderef.c create mode 100644 test/db/run_lock_priority_nullderef.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 09518b74b..46247f16f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -97,7 +97,8 @@ jobs: working-directory: build_unix run: | set -e - for r in run_hash_unsorted_cmp run_recd_compact run_recd_handlers run_upgrade; do + for r in run_hash_unsorted_cmp run_recd_compact run_recd_handlers run_upgrade \ + run_lock_priority_nullderef; do echo "::group::$r" bash "../test/db/$r.sh" echo "::endgroup::" diff --git a/src/lock/lock_method.c b/src/lock/lock_method.c index b34e04ae5..c02415898 100644 --- a/src/lock/lock_method.c +++ b/src/lock/lock_method.c @@ -479,8 +479,18 @@ __lock_set_lk_priority(dbenv, lockid, priority) if (!LOCKING_ON(env)) return (EINVAL); - if ((ret = __lock_getlocker(env->lk_handle, lockid, 0, &locker)) == 0) + if ((ret = __lock_getlocker(env->lk_handle, lockid, 0, &locker)) == 0) { + /* + * __lock_getlocker() with create == 0 reports a missing locker + * by returning 0 with a NULL locker, not by returning an error, + * so ret == 0 alone does not mean we have one. Callers such as + * __lock_vec_pp() rely on that contract (a locker legitimately + * holds no locks), so check here rather than changing it. + */ + if (locker == NULL) + return (EINVAL); locker->priority = priority; + } return (ret); } @@ -504,8 +514,12 @@ __lock_get_lk_priority(dbenv, lockid, priorityp) if (!LOCKING_ON(env)) return (EINVAL); - if ((ret = __lock_getlocker(env->lk_handle, lockid, 0, &locker)) == 0) + if ((ret = __lock_getlocker(env->lk_handle, lockid, 0, &locker)) == 0) { + /* See the comment in __lock_set_lk_priority(). */ + if (locker == NULL) + return (EINVAL); *priorityp = locker->priority; + } return ret; } diff --git a/test/db/lock_priority_nullderef.c b/test/db/lock_priority_nullderef.c new file mode 100644 index 000000000..64bb20845 --- /dev/null +++ b/test/db/lock_priority_nullderef.c @@ -0,0 +1,105 @@ +/*- + * Regression test for the NULL-dereference in DB_ENV->set_lk_priority() and + * DB_ENV->get_lk_priority() (issue #148). + * + * __lock_getlocker(create = 0) reports "no such locker" by returning 0 with a + * NULL locker rather than by returning an error, because callers such as + * __lock_vec_pp() legitimately expect a locker that holds no locks. The two + * priority accessors adopted that idiom without the NULL check and dereferenced + * it, so a plain public-API call with an unused locker id crashed the library. + * + * Cases: + * 1. set_lk_priority() on an id with no live locker -> EINVAL, no crash. + * 2. get_lk_priority() on an id with no live locker -> EINVAL, no crash. + * 3. set/get on a LIVE locker still round-trips, so the fix did not simply + * disable the feature. + */ +#include +#include +#include +#include +#include +#include + +static int failures = 0; +static int checks = 0; + +#define CHECK(cond, ...) do { \ + checks++; \ + if (!(cond)) { \ + failures++; \ + printf(" FAIL: "); \ + printf(__VA_ARGS__); \ + printf("\n"); \ + } \ +} while (0) + +int +main(int argc, char *argv[]) +{ + DB_ENV *dbenv; + DB_TXN *txn; + u_int32_t id, prio; + const char *home = "LOCK_PRIORITY_TESTDIR"; + int ret; + + (void)argc; (void)argv; + + if ((ret = db_env_create(&dbenv, 0)) != 0) { + fprintf(stderr, "db_env_create: %s\n", db_strerror(ret)); + return (EXIT_FAILURE); + } + dbenv->set_errpfx(dbenv, "lock_priority_nullderef"); + if ((ret = dbenv->open(dbenv, home, DB_CREATE | DB_INIT_LOCK | + DB_INIT_MPOOL | DB_INIT_TXN | DB_INIT_LOG, 0600)) != 0) { + fprintf(stderr, "env open %s: %s\n", home, db_strerror(ret)); + return (EXIT_FAILURE); + } + + /* + * 1 + 2: an id with no live locker. Before the fix these dereferenced + * NULL and raised SIGSEGV; the process died here rather than failing a + * check, so simply reaching the end of this test is part of the result. + */ + ret = dbenv->set_lk_priority(dbenv, 0, 100); + printf(" set_lk_priority(unused id): ret=%d (%s)\n", + ret, ret == 0 ? "success" : db_strerror(ret)); + CHECK(ret == EINVAL, + "set_lk_priority on an unused id returned %d, expected EINVAL (%d)", + ret, EINVAL); + + prio = 0; + ret = dbenv->get_lk_priority(dbenv, 0, &prio); + printf(" get_lk_priority(unused id): ret=%d (%s)\n", + ret, ret == 0 ? "success" : db_strerror(ret)); + CHECK(ret == EINVAL, + "get_lk_priority on an unused id returned %d, expected EINVAL (%d)", + ret, EINVAL); + + /* 3: a live locker must still work. */ + if ((ret = dbenv->txn_begin(dbenv, NULL, &txn, 0)) != 0) { + fprintf(stderr, "txn_begin: %s\n", db_strerror(ret)); + return (EXIT_FAILURE); + } + id = txn->id(txn); + + ret = dbenv->set_lk_priority(dbenv, id, 77); + CHECK(ret == 0, "set_lk_priority on live locker %u returned %d (%s)", + id, ret, db_strerror(ret)); + + prio = 0; + ret = dbenv->get_lk_priority(dbenv, id, &prio); + CHECK(ret == 0, "get_lk_priority on live locker %u returned %d (%s)", + id, ret, db_strerror(ret)); + CHECK(prio == 77, "priority round trip on live locker %u: set 77, got %u", + id, prio); + printf(" live locker %u: set/get round trip prio=%u\n", id, prio); + + (void)txn->abort(txn); + (void)dbenv->close(dbenv, 0); + + printf("lock_priority_nullderef: %d checks, %d failures\n", + checks, failures); + printf("lock_priority_nullderef: %s\n", failures == 0 ? "PASS" : "FAIL"); + return (failures == 0 ? EXIT_SUCCESS : EXIT_FAILURE); +} diff --git a/test/db/run_lock_priority_nullderef.sh b/test/db/run_lock_priority_nullderef.sh new file mode 100644 index 000000000..915bb74f1 --- /dev/null +++ b/test/db/run_lock_priority_nullderef.sh @@ -0,0 +1,85 @@ +#!/bin/sh - +# +# $Id$ +# +# run_lock_priority_nullderef.sh -- +# Build and run lock_priority_nullderef.c, the regression test for +# https://github.com/berkeleydb/libdb/issues/148 -- +# DB_ENV->set_lk_priority() and DB_ENV->get_lk_priority() +# (src/lock/lock_method.c) dereferenced a NULL locker. +# +# __lock_getlocker() with create == 0 reports "no such locker" by +# returning 0 with a NULL locker rather than by returning an error, +# because callers such as __lock_vec_pp() legitimately expect a locker +# that holds no locks. Both priority accessors adopted that idiom +# without the NULL check, so a plain public-API call naming an id with no +# live locker raised SIGSEGV. They now return EINVAL. +# +# Exits non-zero on failure or hang. + +set -e + +BUILD=${BUILD:-.} +SRC=${SRC:-../test/db/lock_priority_nullderef.c} +HOME_DIR=${HOME_DIR:-LOCK_PRIORITY_TESTDIR} +TIMEOUT=${TIMEOUT:-180} + +# Prefer the STATIC library: on macOS the .dylib carries a baked-in install name +# (/usr/local/BerkeleyDB.5.3/lib/...) which takes precedence over -rpath, so a +# shared link runs against an uninstalled path and dyld aborts. Static linking +# avoids the dynamic loader entirely, which is what the other CI-wired suites +# (e.g. test/fuzz) already do. Fall back to the shared library if no static one +# was built. +LIB="" +LIBRPATH="" +for cand in "$BUILD"/libdb.a "$BUILD"/.libs/libdb-5.3.a "$BUILD"/.libs/libdb-*.a; do + if [ -f "$cand" ]; then LIB="$cand"; break; fi +done +if [ -z "$LIB" ]; then + for cand in "$BUILD"/.libs/libdb-5.3.so "$BUILD"/.libs/libdb-5.3.dylib \ + "$BUILD"/.libs/libdb-*.so "$BUILD"/.libs/libdb-*.dylib; do + if [ -f "$cand" ]; then LIB="$cand"; break; fi + done + [ -n "$LIB" ] && LIBRPATH="-Wl,-rpath,$(cd "$BUILD/.libs" && pwd)" +fi +[ -n "$LIB" ] || { echo "FAIL: no libdb library (static or shared) found under $BUILD"; exit 1; } + +# A static libdb needs its transitive deps named explicitly. +EXTRALIBS="" +for l in $(pkg-config --libs liburing 2>/dev/null); do EXTRALIBS="$EXTRALIBS $l"; done + +echo "Compiling lock_priority_nullderef against $LIB" +"${CC:-cc}" -g -O1 ${CFLAGS:-} -I"$BUILD" "$SRC" "$LIB" \ + -lpthread $LIBRPATH $EXTRALIBS \ + -o "$BUILD/lock_priority_nullderef" + +rm -f "$HOME_DIR"/*.db 2>/dev/null || true +mkdir -p "$HOME_DIR" + +# `timeout` is GNU coreutils: present on Linux, absent on stock macOS (where it +# is `gtimeout` if coreutils is installed). Resolve it once; if neither exists, +# run without a timeout rather than failing with rc=127. +if command -v timeout >/dev/null 2>&1; then + TIMEOUT_CMD="timeout" +elif command -v gtimeout >/dev/null 2>&1; then + TIMEOUT_CMD="gtimeout" +else + TIMEOUT_CMD="" +fi +run_with_timeout() { + if [ -n "$TIMEOUT_CMD" ]; then + "$TIMEOUT_CMD" "$@" + else + shift # drop the seconds argument + "$@" + fi +} +echo "Running lock_priority_nullderef (timeout ${TIMEOUT}s)" +if run_with_timeout "$TIMEOUT" "$BUILD/lock_priority_nullderef"; then + echo "run_lock_priority_nullderef.sh: PASS" + exit 0 +else + rc=$? + echo "run_lock_priority_nullderef.sh: FAIL (rc=$rc)" + exit $rc +fi