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