diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46247f16f..720c36b90 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -98,7 +98,7 @@ jobs: run: | set -e for r in run_hash_unsorted_cmp run_recd_compact run_recd_handlers run_upgrade \ - run_lock_priority_nullderef; do + run_lock_priority_nullderef run_null_method_slots; do echo "::group::$r" bash "../test/db/$r.sh" echo "::endgroup::" diff --git a/src/db/db_cds.c b/src/db/db_cds.c index bcd57c54c..1c0890231 100644 --- a/src/db/db_cds.c +++ b/src/db/db_cds.c @@ -24,6 +24,11 @@ static int __cdsgroup_get_name __P((DB_TXN *txn, const char **namep)); static int __cdsgroup_set_name __P((DB_TXN *txn, const char *name)); static int __cdsgroup_set_timeout __P((DB_TXN *txn, db_timeout_t timeout, u_int32_t flags)); +static int __cdsgroup_get_priority __P((DB_TXN *txn, u_int32_t *priorityp)); +static int __cdsgroup_set_priority __P((DB_TXN *txn, u_int32_t priority)); +static int __cdsgroup_set_commit_token __P((DB_TXN *txn, DB_TXN_TOKEN *tokenp)); +static void __cdsgroup_set_txn_lsnp + __P((DB_TXN *txn, DB_LSN **rlsnp, DB_LSN **lsnp)); /* * __cdsgroup_notsup -- @@ -127,6 +132,53 @@ static int __cdsgroup_set_timeout(txn, timeout, flags) return (__cdsgroup_notsup(txn->mgrp->env, "set_timeout")); } +static int +__cdsgroup_get_priority(txn, priorityp) + DB_TXN *txn; + u_int32_t *priorityp; +{ + COMPQUIET(priorityp, NULL); + return (__cdsgroup_notsup(txn->mgrp->env, "get_priority")); +} + +static int +__cdsgroup_set_priority(txn, priority) + DB_TXN *txn; + u_int32_t priority; +{ + COMPQUIET(priority, 0); + return (__cdsgroup_notsup(txn->mgrp->env, "set_priority")); +} + +static int +__cdsgroup_set_commit_token(txn, tokenp) + DB_TXN *txn; + DB_TXN_TOKEN *tokenp; +{ + COMPQUIET(tokenp, NULL); + return (__cdsgroup_notsup(txn->mgrp->env, "set_commit_token")); +} + +/* + * __cdsgroup_set_txn_lsnp -- + * The DB_TXN method table declares this one void, so it cannot report + * DB_OPNOTSUP the way the others do. A CDS group has no transactional + * LSNs, so hand back NULL pointers: callers in the log path test these for + * NULL, and leaving the slot itself NULL meant an indirect call through a + * NULL function pointer instead. + */ +static void +__cdsgroup_set_txn_lsnp(txn, rlsnp, lsnp) + DB_TXN *txn; + DB_LSN **rlsnp, **lsnp; +{ + COMPQUIET(txn, NULL); + if (rlsnp != NULL) + *rlsnp = NULL; + if (lsnp != NULL) + *lsnp = NULL; +} + /* * PUBLIC: int __cdsgroup_begin __P((ENV *, DB_TXN **)); */ @@ -161,6 +213,16 @@ __cdsgroup_begin(env, txnpp) txn->get_name = __cdsgroup_get_name; txn->set_name = __cdsgroup_set_name; txn->set_timeout = __cdsgroup_set_timeout; + /* + * A DB_TXN handle has 12 methods; the eight above are the ones a CDS + * group can implement. The remaining four used to be left NULL, so an + * application calling one of them made an indirect call through a NULL + * function pointer and crashed instead of getting an error. + */ + txn->get_priority = __cdsgroup_get_priority; + txn->set_priority = __cdsgroup_set_priority; + txn->set_commit_token = __cdsgroup_set_commit_token; + txn->set_txn_lsnp = __cdsgroup_set_txn_lsnp; *txnpp = txn; diff --git a/src/repmgr/repmgr_util.c b/src/repmgr/repmgr_util.c index 5f41bc795..cf02f0b9c 100644 --- a/src/repmgr/repmgr_util.c +++ b/src/repmgr/repmgr_util.c @@ -517,6 +517,18 @@ __repmgr_get_nsites(env, nsitesp) db_rep = env->rep_handle; + /* + * db_rep->region is only attached once the environment is opened, but + * ENV_NOT_CONFIGURED() (the caller's guard) only tests anything after + * ENV_OPEN_CALLED is set, so a pre-open call reached this dereference + * and crashed. Report the same error as an unstarted repmgr instead. + */ + if (db_rep->region == NULL) { + __db_errx(env, DB_STR("3672", + "Nsites unknown before repmgr_start()")); + return (EINVAL); + } + if ((nsites = db_rep->region->config_nsites) == 0) { __db_errx(env, DB_STR("3672", "Nsites unknown before repmgr_start()")); diff --git a/test/db/null_method_slots.c b/test/db/null_method_slots.c new file mode 100644 index 000000000..b565dd237 --- /dev/null +++ b/test/db/null_method_slots.c @@ -0,0 +1,160 @@ +/*- + * Regression test for the NULL-dereference pair in issue #149. + * + * 1. DB_ENV->rep_get_nsites() on a repmgr-configured but UNOPENED environment. + * __repmgr_get_nsites() dereferenced db_rep->region, which is only attached + * at env open. The caller's ENV_NOT_CONFIGURED() guard does nothing before + * ENV_OPEN_CALLED is set, so the call reached the dereference and crashed. + * + * 2. __cdsgroup_begin() installed only 8 of DB_TXN's 12 methods, leaving + * get_priority, set_priority, set_commit_token and set_txn_lsnp as NULL + * function pointers. Calling one made an indirect call through NULL. + * + * Both are reachable from documented public APIs with valid arguments -- only + * the call order, or the handle type, is unusual. + */ +#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) + +/* + * rep_nsites_preopen -- + * Case 1. Configure repmgr but never open the environment. + */ +static int +rep_nsites_preopen(void) +{ + DB_ENV *dbenv; + u_int32_t nsites; + int ret; + + if ((ret = db_env_create(&dbenv, 0)) != 0) { + fprintf(stderr, "db_env_create: %s\n", db_strerror(ret)); + return (1); + } + /* Quiet the expected BDB3672 diagnostic. */ + dbenv->set_errfile(dbenv, NULL); + + /* + * Touching any repmgr setting allocates the DB_REP handle without + * attaching its shared region -- the pre-open state that crashed. + */ + if ((ret = dbenv->repmgr_set_ack_policy(dbenv, + DB_REPMGR_ACKS_ALL)) != 0) { + printf(" (repmgr unavailable in this build: %s -- skipping " + "case 1)\n", db_strerror(ret)); + (void)dbenv->close(dbenv, 0); + return (0); + } + + nsites = 12345; + ret = dbenv->rep_get_nsites(dbenv, &nsites); + printf(" rep_get_nsites(unopened env): ret=%d (%s)\n", + ret, ret == 0 ? "success" : db_strerror(ret)); + CHECK(ret != 0, + "rep_get_nsites on an unopened env returned success; it cannot " + "know nsites yet"); + + (void)dbenv->close(dbenv, 0); + return (0); +} + +/* + * cds_method_slots -- + * Case 2. Every DB_TXN slot on a CDS group handle must be callable. + */ +static int +cds_method_slots(const char *home) +{ + DB_ENV *dbenv; + DB_TXN *txn; + DB_TXN_TOKEN token; + DB_LSN *rlsnp, *lsnp; + u_int32_t priority; + int ret; + + if ((ret = db_env_create(&dbenv, 0)) != 0) { + fprintf(stderr, "db_env_create: %s\n", db_strerror(ret)); + return (1); + } + dbenv->set_errfile(dbenv, NULL); + if ((ret = dbenv->open(dbenv, home, + DB_CREATE | DB_INIT_CDB | DB_INIT_MPOOL, 0600)) != 0) { + fprintf(stderr, "env open %s: %s\n", home, db_strerror(ret)); + return (1); + } + if ((ret = dbenv->cdsgroup_begin(dbenv, &txn)) != 0) { + fprintf(stderr, "cdsgroup_begin: %s\n", db_strerror(ret)); + return (1); + } + + /* + * A CDS group cannot honor any of these, so an error is the correct + * answer -- the point is that the call returns at all. + */ + CHECK(txn->get_priority != NULL, "get_priority slot is NULL"); + CHECK(txn->set_priority != NULL, "set_priority slot is NULL"); + CHECK(txn->set_commit_token != NULL, "set_commit_token slot is NULL"); + CHECK(txn->set_txn_lsnp != NULL, "set_txn_lsnp slot is NULL"); + + priority = 0; + ret = txn->get_priority(txn, &priority); + printf(" cdsgroup get_priority: ret=%d\n", ret); + CHECK(ret != 0, "get_priority on a CDS group returned success"); + + ret = txn->set_priority(txn, 5); + printf(" cdsgroup set_priority: ret=%d\n", ret); + CHECK(ret != 0, "set_priority on a CDS group returned success"); + + memset(&token, 0, sizeof(token)); + ret = txn->set_commit_token(txn, &token); + printf(" cdsgroup set_commit_token: ret=%d\n", ret); + CHECK(ret != 0, "set_commit_token on a CDS group returned success"); + + /* void method: it must null the out-parameters, not crash. */ + rlsnp = (DB_LSN *)(uintptr_t)1; + lsnp = (DB_LSN *)(uintptr_t)1; + txn->set_txn_lsnp(txn, &rlsnp, &lsnp); + printf(" cdsgroup set_txn_lsnp: rlsnp=%p lsnp=%p\n", + (void *)rlsnp, (void *)lsnp); + CHECK(rlsnp == NULL && lsnp == NULL, + "set_txn_lsnp left non-NULL LSN pointers (%p, %p) for a CDS group", + (void *)rlsnp, (void *)lsnp); + + (void)txn->commit(txn, 0); + (void)dbenv->close(dbenv, 0); + return (0); +} + +int +main(int argc, char *argv[]) +{ + const char *home = "NULL_METHOD_TESTDIR"; + + (void)argc; (void)argv; + + if (rep_nsites_preopen() != 0) + return (EXIT_FAILURE); + if (cds_method_slots(home) != 0) + return (EXIT_FAILURE); + + printf("null_method_slots: %d checks, %d failures\n", checks, failures); + printf("null_method_slots: %s\n", failures == 0 ? "PASS" : "FAIL"); + return (failures == 0 ? EXIT_SUCCESS : EXIT_FAILURE); +} diff --git a/test/db/run_null_method_slots.sh b/test/db/run_null_method_slots.sh new file mode 100644 index 000000000..44225572c --- /dev/null +++ b/test/db/run_null_method_slots.sh @@ -0,0 +1,82 @@ +#!/bin/sh - +# +# $Id$ +# +# run_null_method_slots.sh -- +# Build and run null_method_slots.c, the regression test for +# https://github.com/berkeleydb/libdb/issues/149 -- two NULL +# dereferences reachable from documented public APIs: +# +# 1. DB_ENV->rep_get_nsites() on a repmgr-configured but unopened +# environment dereferenced db_rep->region (attached only at open). +# 2. __cdsgroup_begin() left 4 of DB_TXN's 12 method slots NULL, so +# calling one was an indirect call through a NULL pointer. +# +# Exits non-zero on failure or hang. + +set -e + +BUILD=${BUILD:-.} +SRC=${SRC:-../test/db/null_method_slots.c} +HOME_DIR=${HOME_DIR:-NULL_METHOD_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 null_method_slots against $LIB" +"${CC:-cc}" -g -O1 ${CFLAGS:-} -I"$BUILD" "$SRC" "$LIB" \ + -lpthread $LIBRPATH $EXTRALIBS \ + -o "$BUILD/null_method_slots" + +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 null_method_slots (timeout ${TIMEOUT}s)" +if run_with_timeout "$TIMEOUT" "$BUILD/null_method_slots"; then + echo "run_null_method_slots.sh: PASS" + exit 0 +else + rc=$? + echo "run_null_method_slots.sh: FAIL (rc=$rc)" + exit $rc +fi