fix(lock): don't dereference a NULL locker in the priority accessors (#148)

.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::"

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;
 }
 

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 <sys/types.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <db.h>
+
+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);
+}

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