A confirmed `<commit>` that fails still leaves an armed rollback behind

[karowski]

A confirmed <commit> that fails still leaves an armed rollback behind

Version: reproduced on netopeer2 v2.8.7 (newest release) and on the current devel head
f8b0d7ae (2.8.16), the latter against sysrepo b0b8f7c5 (5.2.18), libyang 6.3.0 (SOVERSION
5.9.2, 384c2237) and libnetconf2 4.5.1 (dcd4a096), all built from source. Line numbers below are
f8b0d7ae; src/netconf_confirmed_commit.c changed between the release and that head, so some of
them differ from v2.8.7, but every site named here is unchanged in substance.

The client is correctly told its commit failed. The server nevertheless believes a confirmed
commit is pending: <cancel-commit> succeeds, a START notification has already gone out to
subscribers, and a timer is armed to "roll back" to a state the server never left.

Reproducer: np2_cc_failed_state_min.c, inlined in full at the end of this report. It is a
standalone NETCONF client that runs against an already-running server — no test harness — and
exits 1 when the state survives.

build	control: fresh server	after a failed confirmed commit
netopeer2 v2.8.7 (newest release)	cancel-commit → No pending confirmed commit to cancel.	cancel-commit → <ok>
devel head 2.8.16 (f8b0d7ae) + sysrepo 5.2.18 (b0b8f7c5) + libyang 6.3.0 (384c2237)	refused	<ok>

The control is the same cancel-commit on a fresh server: it is refused, so the probe demonstrably
distinguishes "pending" from "not pending".

Impact

Although the client receives <rpc-error>, the server keeps a rollback snapshot and pending
confirmed-commit state. If running is legitimately modified afterwards — by this client, another
client, or a plugin — that state can silently discard the intervening changes in two ways:

• the hidden timer expires and ncc_changes_rollback_cb() restores running from the snapshot
  taken before the commit that never happened;
• netopeer2 restarts while the meta file exists. ncc_try_restore() (called from
  main.c:1075) reads it and calls the same rollback immediately, without waiting for whatever
  was left of the timeout — so a restart converts the leftover into an instant revert.

The phantom pending commit also blocks further confirmed commits until it is cancelled or times
out, because the server treats the next one as a follow-up to it:

• a <commit><confirmed/> carrying a different <persist> is rejected with "Follow-up confirm
  commit does not match pending confirmed commit." (:886-889);
• if the phantom carries no persist-id, the check is by session instead, so a confirmed commit from
  any other session is rejected with "Follow-up confirm commit session does not match the
  pending confirmed commit session." (:893-896).

None of this is visible to the client that caused it: its commit was correctly refused, and
commit_ctx is a single server-wide structure (:55-60), so the consequences fall on whoever
writes running next.

Cause

np2srv_confirmed_commit_cb() (src/netconf_confirmed_commit.c:840) does all of its setup before
attempting the commit, and undoes none of it if the commit fails:

    if (!commit_ctx.timer) {
        if (ncc_running_backup(LYD_CTX(rpc))) {          /* :871  backups written  */
    ...
    ncc_create_meta_file(timeout);                       /* :907  meta file written */
    if (persist) {
        if (ncc_set_persist(persist)) {                  /* :911  persist stored    */
    ...
    if (ncc_commit_timeout_schedule(timeout, user_sess)) {   /* :921  timer armed    */
    ...
        np_send_notif_confirmed_commit(nc_sess, user_sess->sess, NP_CC_START, timeout, 0);  /* :930 */
    }

    /* sysrepo API */
    ...
        sr_session_switch_ds(user_sess->sess, SR_DS_RUNNING);
        if (sr_copy_config(user_sess->sess, NULL, SR_DS_CANDIDATE, np2srv.sr_timeout)) {
            reply = np_reply_err_sr(user_sess->sess, LYD_NAME(rpc));
            goto cleanup;                                /* :946  nothing is undone */
        }

cleanup:
    sr_pc_free_conflicts(conflict_set);                  /* :954  only this         */
    return reply;

So after a failed commit the server holds: a running backup on disk, a meta file, persist (or
commit_ctx.nc_sess), an armed SIGEV_THREAD rollback timer, and it has already emitted the
netconf-confirmed-commit START notification.

The armed timer is the dangerous part: when it fires it restores the backup taken before a commit
that never happened, i.e. it rewrites running from a snapshot for no reason. The meta file also
survives a restart, so ncc_try_restore() will act on it.

Why the commit fails on ordinary input

sysrepo does not validate the candidate datastore, so <edit-config> into candidate succeeds even
when the content cannot be committed to running. The reproducer uses a NACM rule missing its
mandatory true action leaf — accepted by candidate, rejected on commit with
"An expected element is missing."

Any client permitted to edit candidate and invoke confirmed commit can do this. Other routes to a failing commit — another session
holding a lock on running, a subscriber rejecting the change — leave exactly the same state behind.

Trigger

<edit-config><target><candidate/></target><config>…invalid for running…</config></edit-config>
<commit><confirmed/><confirm-timeout>30</confirm-timeout><persist>p1</persist></commit>
<cancel-commit><persist-id>p1</persist-id></cancel-commit>   <!-- answered <ok/> -->

Not covered

• The reproducer stops at "a confirmed commit is pending". It does not wait out the timer to show
  the rollback actually rewriting running — that needs a 30 s wait and a data comparison.
• The START notification sent for a commit that did not happen is described but not observed.
• The <persist>-less variant (state tied to the session rather than a persist-id) is not
  exercised.

Reproducer — np2_cc_failed_state_min.c

/*
 * Reproducer: a confirmed <commit> that FAILS still leaves an armed rollback behind -- the running
 * backup, meta file, persist-id, rollback timer and START notification are all set up before the
 * commit is attempted, and none are undone when it fails. The cause and line numbers are in the
 * accompanying report; this file only demonstrates it. Runs against an already-running server.
 *
 * control:    <cancel-commit persist-id=p1> before anything is committed -- must be REFUSED
 *             ("No pending confirmed commit to cancel."). That is what shows the probe can tell
 *             "pending" from "not pending"; without it a later <ok> would mean nothing.
 * setup:      <edit-config> into candidate with content running will reject (an ietf-netconf-acm
 *             rule missing its mandatory "action" leaf). Candidate is not validated, so this must
 *             be <ok> and the commit to running is what fails.
 * bug case:   <commit><confirmed/><confirm-timeout>30</confirm-timeout><persist>p1</persist> --
 *             must come back <rpc-error>, or there is no failed commit to examine.
 * observable: the SAME <cancel-commit persist-id=p1> is now answered <ok> -- the server cancels a
 *             confirmed commit that never happened.
 *
 * Needs write access to running: disable NACM as test setup (sysrepocfg --datastore running set
 * /ietf-netconf-acm:nacm/enable-nacm false) or connect as a recovery user, else the setup edit or
 * the commit comes back access-denied and the run is inconclusive, not a false pass. The candidate
 * datastore is shared between sessions -- this writes into it and issues <discard-changes> before
 * exiting. The failed commit does not change the effective contents of running, but the probe
 * invokes the erroneous rollback, which rewrites running from the backup; the regression control
 * additionally performs a real successful confirmed commit and then cancels it, which rolls running
 * back again -- but only when that control SUCCEEDS. If it reports a regression, the successful
 * commit was torn down and could not be cancelled, so running keeps the cc-regression filter it
 * committed; remove it by hand. Use a dedicated test server, and not one where another client is
 * using candidate.
 *
 * Build: gcc -o np2_cc_failed_state_min np2_cc_failed_state_min.c \
 *            $(pkg-config --cflags --libs libnetconf2 libyang)
 * Run:   ./np2_cc_failed_state_min unix:/path/to/netopeer2.sock
 *        ./np2_cc_failed_state_min ssh:[user@]host[:port]      (IPv6 bracketed: ssh:user@[::1]:830)
 * Exit:  0 nothing pending after the failed commit (bug absent) - 1 reproduced - 2 inconclusive
 */

#include <errno.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

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

/* per-RPC timeout; a confirmed commit does a backup and a full validation of running */
#define RPC_TIMEOUT_MS 8000

#define CANCEL \
    "<cancel-commit xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\">" \
    "<persist-id>p1</persist-id></cancel-commit>"

/* a NACM rule without its mandatory "action" leaf: valid enough for candidate, rejected when
 * running is validated ("An expected element is missing.") */
#define EDIT_INVALID \
    "<edit-config xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\">" \
    "<target><candidate/></target><config>" \
    "<nacm xmlns=\"urn:ietf:params:xml:ns:yang:ietf-netconf-acm\"><rule-list><name>rl</name>" \
    "<rule><name>r</name><module-name>x</module-name></rule></rule-list></nacm>" \
    "</config></edit-config>"

#define COMMIT_CONFIRMED \
    "<commit xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\"><confirmed/>" \
    "<confirm-timeout>30</confirm-timeout><persist>p1</persist></commit>"

#define DISCARD \
    "<discard-changes xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\"/>"

/* Regression control: a VALID edit, so the confirmed commit below actually succeeds. A
 * subscribed-notifications selection-filter is ordinary configuration data and validates. */
#define EDIT_VALID \
    "<edit-config xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\">" \
    "<target><candidate/></target><config>" \
    "<filters xmlns=\"urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications\">" \
    "<selection-filter xmlns=\"urn:ietf:params:xml:ns:yang:ietf-yang-push\">" \
    "<filter-id>cc-regression</filter-id>" \
    "<datastore-xpath-filter>/</datastore-xpath-filter>" \
    "</selection-filter></filters>" \
    "</config></edit-config>"

#define COMMIT_CONFIRMED_OK \
    "<commit xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\"><confirmed/>" \
    "<confirm-timeout>30</confirm-timeout><persist>p2</persist></commit>"

#define CANCEL_OK \
    "<cancel-commit xmlns=\"urn:ietf:params:xml:ns:netconf:base:1.0\">" \
    "<persist-id>p2</persist-id></cancel-commit>"

enum outcome {
    OUT_OK,         /* <ok/> */
    OUT_RPCERR,     /* <rpc-error> -- a clean rejection, server alive */
    OUT_DEAD,       /* send/recv failed: the session (and maybe the server) is gone */
    OUT_FAIL        /* could not build the RPC (send failures classify as OUT_DEAD) */
};

/* Parse a "unix:PATH" / "ssh:[user@]host[:port]" destination and connect. Returns NULL on
 * failure. */
static struct nc_session *
connect_dst(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)) {
            fprintf(stderr, "ssh destination \"%s\" is too long (max %zu chars)\n", dst + 4,
                    sizeof host - 1);
            return NULL;
        }
        /* optional user@ */
        if ((at = strchr(host, '@'))) {
            *at = '\0';
            if (nc_client_ssh_set_username(host)) {
                fprintf(stderr, "could not set the SSH username\n");
                return NULL;
            }
            memmove(host, at + 1, strlen(at + 1) + 1);
        }
        /* host[:port]; an IPv6 literal is bracketed as [addr] or [addr]:port so its own colons are
         * not mistaken for the port separator */
        if (host[0] == '[') {
            char *rb = strchr(host, ']');

            if (!rb) {
                fprintf(stderr, "malformed IPv6 literal in ssh destination (missing ']')\n");
                return NULL;
            }
            if (rb[1] == ':') {
                portstr = rb + 2;
            } else if (rb[1]) {
                fprintf(stderr, "unexpected text after ']' in ssh destination\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)) {
                fprintf(stderr, "invalid ssh port \"%s\" (expected 1..65535)\n", portstr);
                return NULL;
            }
            port = (uint16_t)p;
        }
        return nc_connect_ssh(host, port, NULL);
    }
    fprintf(stderr, "unknown destination \"%s\" (expected unix:... or ssh:...)\n", dst);
    return NULL;
}

/* Text of the most recent <error-message>, "" when the last reply carried none. A file-static
 * buffer keeps send_rpc()'s signature unchanged; this program is single-threaded. */
static char last_errmsg[512];

/* Send a whole RPC as a generic RPC and classify the reply. */
static enum outcome
send_rpc(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;
    enum outcome res = OUT_FAIL;

    printf("  %-46s ", label);
    fflush(stdout);
    last_errmsg[0] = '\0';

    r = nc_rpc_act_generic_xml(xml, NC_PARAMTYPE_CONST);
    if (!r) {
        printf("could not build the RPC\n");
        return OUT_FAIL;
    }

    t = nc_send_rpc(s, r, RPC_TIMEOUT_MS, &msgid);
    if (t != NC_MSG_RPC) {
        printf("could not send (msgtype %d) -- session may be gone\n", t);
        res = OUT_DEAD;
        goto cleanup;
    }
    do {
        t = nc_recv_reply(s, r, msgid, RPC_TIMEOUT_MS, &envp, &op);
    } while (t == NC_MSG_NOTIF);
    if (t != NC_MSG_REPLY) {
        printf("no reply (msgtype %d) -- session did not answer\n", t);
        res = OUT_DEAD;
        goto cleanup;
    }

    child = envp ? lyd_child(envp) : NULL;
    if (child && !strcmp(LYD_NAME(child), "rpc-error")) {
        char *err = NULL, *b, *e;

        if (!lyd_print_mem(&err, child, LYD_XML, 0) && err &&
                (b = strstr(err, "<error-message")) && (b = strchr(b, '>')) &&
                (e = strstr(++b, "</error-message>"))) {
            snprintf(last_errmsg, sizeof last_errmsg, "%.*s", (int)(e - b), b);
            printf("<rpc-error> %s\n", last_errmsg);
        } else {
            printf("<rpc-error>\n");
        }
        free(err);
        res = OUT_RPCERR;
        goto cleanup;
    }
    if (child && !strcmp(LYD_NAME(child), "ok")) {
        printf("<ok>\n");
        res = OUT_OK;
        goto cleanup;
    }
    printf("<%s> (unexpected)\n", child ? LYD_NAME(child) : "(none)");
    res = OUT_RPCERR;   /* well-formed reply, so the server is alive */

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

int
main(int argc, char **argv)
{
    struct nc_session *s = NULL;
    const char *dst;
    enum outcome ctl, commit, probe;
    int ret = 2, edited = 0;

    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", argv[0]);
        return 2;
    }
    dst = argv[1];

    setvbuf(stdout, NULL, _IOLBF, 0);
    if (nc_client_init()) {
        fprintf(stderr, "nc_client_init failed\n");
        return 2;
    }

    s = connect_dst(dst);
    if (!s) {
        printf("could not connect to %s\n", dst);
        goto cleanup;
    }
    printf("connected to %s\n\n", dst);

    printf("control: nothing is pending yet\n");
    ctl = send_rpc(s, "cancel-commit persist-id=p1", CANCEL);
    if ((ctl == OUT_RPCERR) && !strstr(last_errmsg, "No pending confirmed commit")) {
        printf("\nRESULT: inconclusive -- cancel-commit was refused, but not with the \"no pending\n"
               "        confirmed commit\" error this control needs (\"%s\"). An authorization\n"
               "        failure, a mismatched persist-id or any other rejection would let the\n"
               "        probe below pass without ever showing the two states apart.\n", last_errmsg);
        goto cleanup;
    }
    if (ctl != OUT_RPCERR) {
        printf("\nRESULT: inconclusive -- cancel-commit does not report \"nothing pending\" before\n"
               "        any commit, so it cannot tell the two states apart. An <ok> here means a\n"
               "        confirmed commit was ALREADY pending on this server (a leftover from an\n"
               "        earlier run has just been cancelled); re-run against an idle server.\n");
        goto cleanup;
    }

    printf("\nsetup: content candidate accepts and running will reject\n");
    if (send_rpc(s, "edit-config candidate: rule without action", EDIT_INVALID) != OUT_OK) {
        printf("\nRESULT: inconclusive -- candidate did not accept the edit (NACM on? no write\n"
               "        access to candidate?)\n");
        goto cleanup;
    }
    edited = 1;

    printf("\nbug case: a confirmed commit that fails\n");
    commit = send_rpc(s, "commit confirmed persist=p1 timeout=30", COMMIT_CONFIRMED);
    if (commit != OUT_RPCERR) {
        printf("\nRESULT: inconclusive -- the commit did not fail, so there is no failed commit\n"
               "        whose leftovers could be examined\n");
        if (commit == OUT_OK) {
            /* it went through: a confirmed commit really is pending now, so cancel it to put
             * running back where it was */
            printf("        (the commit succeeded; cancelling it to restore running)\n");
            send_rpc(s, "cancel-commit persist-id=p1 (undo)", CANCEL);
        }
        goto cleanup;
    }

    probe = send_rpc(s, "cancel-commit persist-id=p1", CANCEL);
    if (probe == OUT_OK) {
        printf("      --> a confirmed commit is pending for a commit that never happened\n");
        printf("\nRESULT: bug reproduced -- the failed confirmed commit left its running backup,\n"
               "        meta file, persist-id and armed rollback timer in place (the START\n"
               "        notification has gone out to subscribers too)\n");
        ret = 1;
        goto cleanup;
    }
    if (probe != OUT_RPCERR) {
        printf("\nRESULT: inconclusive -- the probe did not get a usable answer\n");
        goto cleanup;
    }
    /* Same requirement as the opening control: only the specific "nothing pending" error shows the
     * state was torn down. Any other rejection (authorization, a mismatched persist-id, an
     * unrelated cancellation failure) leaves the question open and must not read as "bug absent". */
    if (!strstr(last_errmsg, "No pending confirmed commit")) {
        printf("\nRESULT: inconclusive -- cancel-commit was refused, but not with the \"no pending\n"
               "        confirmed commit\" error (\"%s\"), so it does not show the state was torn\n"
               "        down rather than the cancel simply failing for another reason\n", last_errmsg);
        goto cleanup;
    }
    printf("      --> nothing pending, as after any other failed commit\n");
    printf("\nRESULT: the failed confirmed commit left no state behind; NOT reproduced\n");
    ret = 0;

    /* REGRESSION CONTROL -- only meaningful on a build that passed the case above, i.e. one that
     * tears the state down on failure. A fix must undo the setup ONLY when the commit failed; a
     * naive one that keys off the reply pointer dismantles SUCCESSFUL confirmed commits too, since
     * np_reply_success() and np_reply_err_*() are both non-NULL. Prove the success path survives:
     * a valid confirmed commit must still be pending afterwards. */
    printf("\nregression control: a confirmed commit that SUCCEEDS must stay pending\n");
    /* the invalid rule from the case above is still in the shared candidate; drop it, or the
     * "valid" commit below fails for that reason instead and proves nothing */
    if (send_rpc(s, "discard-changes (clear the invalid edit)", DISCARD) != OUT_OK) {
        printf("      --> could not clear candidate; success path NOT checked\n");
        goto cleanup;
    }
    edited = 0;
    if (send_rpc(s, "edit-config candidate: valid filter", EDIT_VALID) != OUT_OK) {
        printf("      --> could not stage a valid edit; success path NOT checked\n");
        goto cleanup;
    }
    edited = 1;
    if (send_rpc(s, "commit confirmed persist=p2 timeout=30", COMMIT_CONFIRMED_OK) != OUT_OK) {
        printf("      --> the valid confirmed commit did not succeed; success path NOT checked\n");
        goto cleanup;
    }
    if (send_rpc(s, "cancel-commit persist-id=p2", CANCEL_OK) == OUT_OK) {
        printf("      --> still pending and cancelled cleanly: the success path is intact\n");
    } else {
        printf("      --> REGRESSION: the successful confirmed commit is NOT pending any more --\n"
               "          this build tore down a commit that went through (\"%s\"). Running has\n"
               "          been left holding that commit, with no pending confirmation to cancel.\n",
                last_errmsg);
        printf("\nRESULT: inconclusive -- the failed-commit case passed, but the success path is\n"
               "        broken, so this build is not a valid fix\n");
        ret = 2;
    }

cleanup:
    if (s) {
        if (edited) {
            /* candidate is shared between sessions: do not leave the invalid rule in it */
            printf("\ncleanup\n");
            send_rpc(s, "discard-changes", DISCARD);
        }
        nc_session_free(s, NULL);
    }
    nc_client_destroy();
    return ret;
}

[michalvasko]

Okay, should be fixed in latest devel.