fix(rep,cds): don't crash on two NULL slots reachable from public APIs (#149)

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

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;
 

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()"));

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 <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)
+
+/*
+ * 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);
+}

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