NULL dereference in __lock_set_lk_priority / __lock_get_lk_priority for an unknown locker id

[gburd]

Found while building coverage drivers for the never-called getter surface (#147). Verified by source inspection.

The bug

src/lock/lock_method.c, both priority accessors:

if ((ret = __lock_getlocker(env->lk_handle, lockid, 0, &locker)) == 0)
        locker->priority = priority;     /* :482  set */
        ...
        *priorityp = locker->priority;   /* :507  get */

__lock_getlocker is called with create = 0. In __lock_getlocker_int (src/lock/lock_id.c) the only assignment to *retp is line 408, *retp = sh_locker, at the end of the function, and the create branch is guarded by if (sh_locker == NULL && create). So for an id that is not present with create == 0, the function returns 0 with *retp == NULL, and both accessors dereference it unconditionally.

DB_ENV->set_lk_priority() / get_lk_priority() are public APIs taking a caller-supplied lockid, so an unknown or already-freed id is reachable from application code.

Same pattern nearby

__lock_vec_pp (src/lock/lock.c:93) uses the same create = 0 call and passes the result into __lock_vec, so it can feed a NULL locker downstream. Worth auditing together.

Suggested fix

Treat "found, but NULL" as not-found — return EINVAL (or DB_NOTFOUND) when *retp == NULL after a create = 0 lookup, ideally by having __lock_getlocker report not-found distinctly so callers can't repeat the mistake.

Not fixed in #147, which was scoped to tests only.

[gburd]

Escalating this from static analysis to a demonstrated crash, and correcting one part of the original report.

Confirmed: both priority accessors SIGSEGV from plain public API

~12 lines, no fault injection, no invalid arguments — just a lockid 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 */

Both directions crash (exit 139):

env open: 0
calling set_lk_priority(0, 100)...
Segmentation fault

calling get_lk_priority(0)...
Segmentation fault

gdb pins it exactly as predicted:

Program received signal SIGSEGV
#0  __lock_set_lk_priority (lockid=<optimized out>, priority=100) at ../src/lock/lock_method.c:483
#1  main () at r.c:13

Root cause as originally described: __lock_getlocker_int's only assignment to *retp is *retp = sh_locker (src/lock/lock_id.c:408), and the create branch is guarded by if (sh_locker == NULL && create), so a not-found lookup with create = 0 returns 0 with *retp == NULL.

Correction: __lock_vec_pp is NOT affected

The original report flagged __lock_vec_pp (src/lock/lock.c:93) as sharing the bug because it passes the possibly-NULL locker into __lock_vec. That is wrong — I tested it (DB_LOCK_PUT_ALL and DB_LOCK_UPGRADE_WRITE with lid=0, both return 0 cleanly) and __lock_vec handles the case deliberately:

/*
 * Since the locker may hold no locks (i.e., you could call abort before
 * you've done any work), it's perfectly reasonable for there to be no
 * locker; this is not an error.
 */
if (sh_locker == NULL)
        break;

So the create = 0 idiom is safe where the caller expects it. The defect is specifically that the two priority accessors adopted the idiom without the NULL check.

Fix shape

Add the missing check in __lock_set_lk_priority / __lock_get_lk_priority only (EINVAL on locker == NULL), rather than changing __lock_getlocker's contract — __lock_vec_pp depends on the current behavior.