Successful confirmed-commit timeout rollback leaks a sysrepo context lock until restart

[karowski]

Successful confirmed-commit timeout rollback leaks a sysrepo context lock until restart

Version: reproduced on netopeer2 v2.8.7 (5be28a9, stock — the newest release tag). Still
present at the current devel head (d5eb87d, checked 2026-08-28) by inspection:
np_send_notif_confirmed_commit() contains no sr_session_release_context() there either. Line
numbers below are v2.8.7.

After one confirmed-commit timeout, no module can be installed, removed or have a feature
changed until netopeer2 exits or is restarted — and because the lock is an rwlock in sysrepo's
main shared memory, it blocks context-writing operations attempted by any sysrepo process on
the machine, not just netopeer2's own. Ordinary data reads and writes are unaffected; what is lost
is schema management.

Reproducer: np2_cc_ctxlock_min.c (inlined in full at the end of this report). It needs no
harness and no build-time paths — it drives an already running server, so it can be pointed at
yours. Exits 1 when the write lock is unobtainable; takes ~15 s on a failing build, because the
failing probe waits out sysrepo's 10 s lock timeout. Produced the table below.

build	control: install a module	after one confirmed-commit timeout
netopeer2 v2.8.7 (stock)	installed in 0.1 s	FAILED after 10.0 s: Timeout expired
netopeer2 devel	installed in 0.1 s	FAILED after 10.0 s
v2.8.7 + fix-cc-timeout-context-lock-leak.patch	installed	installed

Both probes run with no NETCONF session connected, which is what makes them comparable — see
below.

Cause

np_send_notif_confirmed_commit() (src/common.c:497) takes the context lock when it has no NC
session to get a context from:

    /* get context */
    if (session) {
        ly_ctx = nc_session_get_ctx(session);
    } else {
        assert(event == NP_CC_TIMEOUT);
        ly_ctx = sr_session_acquire_context(sr_session);
    }

and its only cleanup is:

cleanup:
    lyd_free_tree(notif);
    return rc;

There is no sr_session_release_context() anywhere in the function — not on the success path, not
on any of its seven goto cleanups (:518, :524, :529, :551, :555, :564, :572). Around twenty-five other acquire sites in netopeer2 release
immediately.

sr_session_acquire_context() is not a refcount on a private object. It takes SR_LOCK_READ on
main_shm->context_lock, an rwlock living in sysrepo's main shared memory and therefore shared with
every sysrepo process on the host.

The NULL session is the normal path, not an edge case

ncc_commit_confirmed() sets commit_ctx.nc_sess = NULL (netconf_confirmed_commit.c:386) and is
called at :516 — twelve lines before the :528 call that passes it to this function. So the
else branch runs on every successful timer rollback.

There is a second, client-free entry: ncc_try_restore() calls
ncc_changes_rollback_cb((union sigval)NULL) at netconf_confirmed_commit.c:725 during start-up
when a confirmed-commit meta file survives a restart. That leaks the same lock with nobody
connected.

Why the sessions are closed before each probe

netopeer2 deliberately holds one context read lock per live NETCONF session, for the session's
whole lifetime: server_accept_session() (src/main.c:1361) acquires the context at :1372 and,
when the new-session callback succeeds, returns without releasing it — the code says so itself at
:1381, /* callback success, keep the session with the context lock */. It is released only when
the session terminates, sr_release_context(np2srv.sr_conn) under NC_PSPOLL_SESSION_TERM
(:1438). A context writer being
blocked while a client is connected is therefore expected behaviour, not this defect, and probing
with a session open would prove nothing.

Both probes run with every NETCONF session closed. A healthy server then holds no read lock at all,
which the control demonstrates by installing a module in 0.1 s. What the leak changes is that the
count never returns to zero again.

The reproducer's own probe connection is careful to avoid the same trap: it never calls
sr_acquire_context() itself, so it holds no read lock of its own and cannot block its own probe —
what the probe measures is only the server's lock.

There is a second rollback path, and it does not leak

ncc_del_session() (src/netconf_confirmed_commit.c:547) rolls back a pending confirmed commit
immediately when the session that issued it terminates:

552:    if (commit_ctx.timer && !commit_ctx.persist && (commit_ctx.nc_sess == user_sess->ntf_arg.nc_sess)) {
553:        /* rollback */
555:        ncc_changes_rollback_cb((union sigval)(void *)user_sess);
557:        /* send notification about canceling confirmed-commits */
558:        np_send_notif_confirmed_commit(user_sess->ntf_arg.nc_sess, sr_sess, NP_CC_CANCEL, 0, 0);

Note the sigval at :555 is non-NULL, and that is what decides it. Inside
ncc_changes_rollback_cb() the notification is sent only on the NULL-sigval path:

518: cleanup:
519:     if (!sev.sival_ptr) {
...
528:         np_send_notif_confirmed_commit(commit_ctx.nc_sess, sr_sess, NP_CC_TIMEOUT, 0, 0);
529:     }

So on session termination that call is skipped entirely, and ncc_del_session() sends the
notification itself at :558 — with the terminating session and NP_CC_CANCEL, which takes the
if (session) branch and acquires nothing. Only the timer rollback reaches :528, where
commit_ctx.nc_sess has just been cleared by ncc_commit_confirmed() (:516), and that is the
leaking branch. The assert(event == NP_CC_TIMEOUT) beside the acquire says the same thing.

Two consequences worth knowing before reproducing this by hand. The timeout must expire while the
issuing session is still open: disconnect first and this path runs instead, and the server looks
healthy. And a commit carrying <persist> decouples the two, since the test above requires
!commit_ctx.persist — that shape is not exercised here.

(The lines are identical at v2.8.7 (5be28a9, stock) and the current devel head.)

Trigger

<commit><confirmed/><confirm-timeout>1</confirm-timeout></commit>

and then nothing. Any client authorized to commit (NACM default-deny blocks unprivileged users, so
this is an operator-level action, not an anonymous one). The reproducer disables NACM because
<commit> needs write access to running; that is setup, not part of the defect.

Not covered

• The start-up route via ncc_try_restore() is not exercised.
• No upstream test was added; tests/test_confirmed_commit.c would be the right home, though
  asserting on a lock leak needs a probe like this one rather than a protocol assertion.

Reproducer — np2_cc_ctxlock_min.c

No harness: it drives a netopeer2 server that is already running. Probes the context write lock with
no NETCONF session open (control), sends one <commit><confirmed/><confirm-timeout>1</confirm-timeout></commit>,
waits for the timer to fire with the session still open — see There is a second rollback path
above, closing it first exercises the non-leaking branch — then closes the session and probes again.

It is not pure NETCONF, and cannot be: the leak is a sysrepo context read lock, and taking the write
lock to detect it is not something any NETCONF RPC does. So the trigger is NETCONF and the probe is
sr_install_module(). Run it with the same SYSREPO_REPOSITORY_PATH, SYSREPO_SHM_DIR and
SYSREPO_SHM_PREFIX the server uses, or it will connect to a different repository and report
nothing.

cc -o np2_cc_ctxlock_min np2_cc_ctxlock_min.c \
    $(pkg-config --cflags --libs libnetconf2 libyang sysrepo)
./np2_cc_ctxlock_min unix:/path/to/netopeer2.sock

Exit 0 if the write lock was still obtainable, 1 if it was not, 2 if inconclusive. The leak is
permanent, so each run needs a freshly started server — otherwise the control probe fails and the
run correctly reports inconclusive rather than a verdict.

#define _GNU_SOURCE

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>

#include <libyang/libyang.h>
#include <nc_client.h>
#include <sysrepo.h>

/* two throwaway modules, one per probe: installing the same name twice would not need the lock */
#define MOD_A "np2ccmin-a"
#define MOD_B "np2ccmin-b"

/* probe outcomes */
#define P_FREE    0   /* the write lock was taken, i.e. it is free */
#define P_LOCKED  1   /* SR_ERR_TIME_OUT -- somebody holds a read lock */
#define P_OTHER  -1   /* some other failure: the probe measures nothing */

static void
sleep_ms(long ms)
{
    struct timespec ts = { ms / 1000, (ms % 1000) * 1000000L };

    nanosleep(&ts, NULL);
}

/* Write a throwaway YANG module to /tmp, per-pid so concurrent runs do not collide. */
static int
write_yang(char *path_out, size_t path_len, const char *name)
{
    FILE *f;
    int n;

    n = snprintf(path_out, path_len, "/tmp/%s.%ld.yang", name, (long)getpid());
    if ((n < 0) || ((size_t)n >= path_len)) {
        return -1;
    }
    if (!(f = fopen(path_out, "w"))) {
        perror("fopen");
        return -1;
    }
    if ((fprintf(f, "module %s {\n  namespace \"urn:%s\";\n  prefix p;\n"
            "  leaf l { type string; }\n}\n", name, name) < 0) || fclose(f)) {
        perror("writing YANG module");
        unlink(path_out);
        return -1;
    }
    return 0;
}

/*
 * Try to take the sysrepo context WRITE lock, by installing a module. This connection never calls
 * sr_acquire_context(), so it holds no read lock of its own and cannot block itself.
 */
static int
probe_write_lock(sr_conn_ctx_t *conn, const char *label, const char *path, const char *name)
{
    struct timespec t0, t1;
    double secs;
    int r;

    printf("    %-38s ", label);
    fflush(stdout);

    clock_gettime(CLOCK_MONOTONIC, &t0);
    r = sr_install_module(conn, path, NULL, NULL);
    clock_gettime(CLOCK_MONOTONIC, &t1);
    secs = (t1.tv_sec - t0.tv_sec) + (t1.tv_nsec - t0.tv_nsec) / 1e9;

    if (!r) {
        printf("--> installed in %.1fs\n", secs);
        sr_remove_module(conn, name, 1);
        return P_FREE;
    }
    printf("--> FAILED after %.1fs: %s\n", secs, sr_strerror(r));
    /* SR_ERR_TIME_OUT is the lock being unobtainable; anything else is a different problem */
    return (r == SR_ERR_TIME_OUT) ? P_LOCKED : P_OTHER;
}

/* Send one generic RPC; returns 0 on an <ok/> reply. */
static int
rpc_ok(struct nc_session *s, const char *label, const char *xml)
{
    struct lyd_node *envp = NULL, *op = NULL;
    const struct lyd_node *child;
    struct nc_rpc *r;
    NC_MSG_TYPE t;
    uint64_t msgid = 0;
    int rc = -1;

    printf("    %-38s ", label);
    fflush(stdout);

    if (!(r = nc_rpc_act_generic_xml(xml, NC_PARAMTYPE_CONST))) {
        printf("--> could not build the RPC\n");
        return -1;
    }
    if (nc_send_rpc(s, r, 5000, &msgid) != NC_MSG_RPC) {
        printf("--> could not send\n");
        goto cleanup;
    }
    do {
        t = nc_recv_reply(s, r, msgid, 5000, &envp, &op);
    } while (t == NC_MSG_NOTIF);
    if (t != NC_MSG_REPLY) {
        printf("--> no reply\n");
        goto cleanup;
    }

    child = envp ? lyd_child(envp) : NULL;
    if (child && !strcmp(LYD_NAME(child), "ok")) {
        printf("--> <ok>\n");
        rc = 0;
    } else if (child && !strcmp(LYD_NAME(child), "rpc-error")) {
        char *err = NULL;

        lyd_print_mem(&err, envp, LYD_XML, 0);
        printf("--> rpc-error\n%s\n", err ? err : "");
        free(err);
    } else {
        printf("--> unexpected reply <%s>\n", child ? LYD_NAME(child) : "(none)");
    }

cleanup:
    lyd_free_tree(envp);
    lyd_free_siblings(op);
    nc_rpc_free(r);
    return rc;
}

/* Connect to unix:<path> or ssh:[user@]host[:port]. */
static struct nc_session *
connect_to(const char *dst)
{
    if (!strncmp(dst, "unix:", 5)) {
        return nc_connect_unix(dst + 5, NULL);
    }
    if (!strncmp(dst, "ssh:", 4)) {
        char host[256], *at, *portstr = NULL, *end;
        unsigned long p;
        uint16_t port = 830;
        int n;

        n = snprintf(host, sizeof host, "%s", dst + 4);
        if ((n < 0) || ((size_t)n >= sizeof host)) {
            printf("ssh destination is too long (max %zu chars)\n", sizeof host - 1);
            return NULL;
        }
        if ((at = strchr(host, '@'))) {
            *at = '\0';
            if (nc_client_ssh_set_username(host)) {
                printf("could not set the SSH username\n");
                return NULL;
            }
            memmove(host, at + 1, strlen(at + 1) + 1);
        }
        if (host[0] == '[') {
            char *rb = strchr(host, ']');

            if (!rb) {
                printf("malformed IPv6 literal (missing ']')\n");
                return NULL;
            }
            if (rb[1] == ':') {
                portstr = rb + 2;
            } else if (rb[1]) {
                printf("unexpected text after ']'\n");
                return NULL;
            }
            *rb = '\0';
            memmove(host, host + 1, strlen(host + 1) + 1);
        } else if ((portstr = strrchr(host, ':'))) {
            *portstr++ = '\0';
        }
        if (portstr) {
            errno = 0;
            p = strtoul(portstr, &end, 10);
            if (errno || *end || (end == portstr) || (p < 1) || (p > 65535)) {
                printf("invalid ssh port \"%s\" (expected 1..65535)\n", portstr);
                return NULL;
            }
            port = (uint16_t)p;
        }
        return nc_connect_ssh(host, port, NULL);
    }
    printf("unknown destination \"%s\" (expected unix:... or ssh:...)\n", dst);
    return NULL;
}

int
main(int argc, char **argv)
{
    sr_conn_ctx_t *conn = NULL;
    struct nc_session *nc = NULL;
    char path_a[128], path_b[128];
    int ret = 2, rc, before, after;

    if (argc != 2) {
        fprintf(stderr, "usage: %s unix:/path/to/sock | ssh:[user@]host[:port]\n"
                "       (IPv6 hosts in brackets, e.g. ssh:user@[::1]:830)\n"
                "Run with the same SYSREPO_* environment the server uses.\n"
                "NOTE: leaves the server unable to install/remove modules until it is restarted.\n",
                argv[0]);
        return 2;
    }

    setvbuf(stdout, NULL, _IOLBF, 0);

    if (nc_client_init()) {
        fprintf(stderr, "nc_client_init failed\n");
        return 2;
    }

    if (write_yang(path_a, sizeof path_a, MOD_A) || write_yang(path_b, sizeof path_b, MOD_B)) {
        return 2;
    }

    /* our own sysrepo connection, used ONLY to probe the write lock. It never acquires the
     * context, so it holds no read lock and cannot block its own probe. */
    if ((rc = sr_connect(0, &conn))) {
        printf("sr_connect: %s\n", sr_strerror(rc));
        printf("\nRESULT: could not reach the sysrepo repository. Run this with the same\n"
               "        SYSREPO_REPOSITORY_PATH / SYSREPO_SHM_DIR / SYSREPO_SHM_PREFIX as the\n"
               "        server, and as a user that may write the repository.\n");
        goto cleanup;
    }

    /* 1. control: with no NETCONF session open, the write lock must be free */
    printf("control: no NETCONF session open, so the server holds no per-session lock\n");
    before = probe_write_lock(conn, "install a module", path_a, MOD_A);
    if (before != P_FREE) {
        printf("\nRESULT: inconclusive -- the write lock is already unobtainable before any\n"
               "        confirmed commit, so this probe measures something else (another client\n"
               "        connected? a previous run of this test? an unrelated holder?)\n");
        goto cleanup;
    }

    /* 2. trigger: one confirmed commit, abandoned */
    printf("\nsetup: one confirmed commit over NETCONF, left to time out\n");
    if (!(nc = connect_to(argv[1]))) {
        printf("    could not connect to %s\n", argv[1]);
        goto cleanup;
    }
    printf("    connected to %s\n", argv[1]);
    if (rpc_ok(nc, "commit confirmed, confirm-timeout=1",
            "<commit xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\">"
              "<confirmed/><confirm-timeout>1</confirm-timeout>"
            "</commit>")) {
        printf("\nRESULT: inconclusive -- the confirmed commit was not accepted (NACM? <commit>\n"
               "        needs write access to running)\n");
        goto cleanup;
    }

    /*
     * ORDER MATTERS HERE, and getting it wrong makes the bug disappear.
     *
     * The timer must fire while this session is STILL OPEN. netopeer2 has a second rollback path
     * that never reaches the leaking branch: ncc_del_session() (netconf_confirmed_commit.c:547)
     * calls ncc_changes_rollback_cb() at :555 with a non-NULL sigval, and that callback's
     * notification call is guarded by `if (!sev.sival_ptr)` (:519, call at :528), so it is skipped.
     * ncc_del_session() sends the notification itself at :558 with the terminating session and
     * NP_CC_CANCEL -- the `if (session)` branch, which acquires nothing. Closing the session first
     * therefore exercises the CORRECT path and reports "not reproduced". (A commit with <persist>
     * would decouple the two, since
     * the :552 shortcut requires !commit_ctx.persist.)
     *
     * So: wait out the 1 s timeout with the session up, and only then close it -- the probe needs
     * the session gone because netopeer2 holds a context read lock per live session by design,
     * which would mask the leak either way.
     */
    printf("    waiting 3s for the rollback TIMER to fire (session still open)\n");
    sleep_ms(3000);

    nc_session_free(nc, NULL);
    nc = NULL;
    printf("    NETCONF session closed; waiting 2s for the server to run its teardown\n");
    sleep_ms(2000);

    /* 3. the finding: the same probe, same conditions */
    printf("\nbug case: same probe, still no NETCONF session open\n");
    after = probe_write_lock(conn, "install another module", path_b, MOD_B);

    printf("\n");
    if (after == P_LOCKED) {
        printf("  --> BUG: the context write lock is no longer obtainable, with no client\n");
        printf("      connected. np_send_notif_confirmed_commit() acquired a sysrepo context\n");
        printf("      READ lock on its NULL-session branch -- the normal confirmed-commit timeout\n");
        printf("      path -- and never released it. The lock lives in sysrepo's MAIN shared\n");
        printf("      memory, so module install/remove/feature-change is now blocked for every\n");
        printf("      sysrepo process on this host, until the server is restarted.\n");
        printf("RESULT: bug reproduced\n");
        ret = 1;
    } else if (after == P_FREE) {
        printf("  --> the write lock is still free; not reproduced here\n");
        printf("RESULT: not reproduced\n");
        ret = 0;
    } else {
        printf("  --> the probe failed for an unrelated reason, so nothing is established\n");
        printf("RESULT: inconclusive\n");
        ret = 2;
    }
    if (ret == 1) {
        printf("\nNOTE: this server now has a leaked context read lock. Nothing here can release\n"
               "      it -- restart the server.\n");
    }

cleanup:
    if (nc) {
        nc_session_free(nc, NULL);
    }
    if (conn) {
        /* best effort: the probes remove their own module on success, this catches the rest */
        sr_remove_module(conn, MOD_A, 1);
        sr_remove_module(conn, MOD_B, 1);
        sr_disconnect(conn);
    }
    unlink(path_a);
    unlink(path_b);
    nc_client_destroy();
    return ret;
}

[michalvasko]

Fine, fixed.