`<copy-config>` from a conventional datastore into a private candidate reads the shared candidate instead of `<source>`

[karowski]

<copy-config> from a conventional datastore into a private candidate reads the shared candidate instead of <source>

Version: netopeer2 v2.8.7 (newest release, 5be28a93) and devel 2.8.12 (c6f4363).
Reproduced on both. Still present at current upstream tip cb49dab (2.8.12, checked 2026-08-24):
np2srv_pc_copyconfig() is byte-identical across all three (same sha256 of the function body),
though src/netconf.c as a whole differs between v2.8.7 and devel (2 insertions, 2 deletions
elsewhere in the file). The sysrepo line numbers below (sr_get_data, sr_pc_*, sr_modinfo_init)
are from sysrepo c3b322e4 (v5.1.0-84-gc3b322e4).

Silent wrong data, answered with <ok/>. The client asks for startup and gets the contents of the
shared (conventional) candidate, is told the copy succeeded, and may then review and commit a
configuration that is not the one it requested.

In the reproducer that shared candidate is unmodified, and an unmodified candidate mirrors running
— which is why the observed marker is marker-in-running. That is the symptom, not the mechanism:
had the shared candidate been modified, its data would have been copied instead.

build	bug case: candidate ← startup	control: running ← startup
netopeer2 v2.8.7	marker-in-running	marker-in-startup
netopeer2 devel 2.8.12 (c6f4363)	marker-in-running	marker-in-startup
v2.8.7 + fix-privcand-copyconfig-wrong-source.patch	marker-in-startup	marker-in-startup

The control is the same RPC with the same source on a target that does not take the private-candidate
path, so the setup, the RPC and the datastores are demonstrably right and only the private-candidate
path is wrong.

Cause

np2srv_rpc_copyconfig_cb() switches the sysrepo session to the target datastore before
dispatching (src/netconf.c:531-532):

    /* update sysrepo session datastore */
    sr_session_switch_ds(user_sess->sess, ds);

Any conventional-datastore read through the session-scoped sr_get_data() API must therefore
switch the session to sds first. The URL path does exactly that (:550-551):

                /* get source datastore data */
                sr_session_switch_ds(user_sess->sess, sds);
                if (sr_get_data(user_sess->sess, "/*", 0, np2srv.sr_timeout, 0, &sr_data)) {

np2srv_pc_copyconfig() does not (:396-403):

        } else {
            /* conventional datastore -> private candidate */
            if (sr_get_data(user_sess->sess, "/*", 0, 0, 0, &sr_data)) {
                reply = np_reply_err_sr(user_sess->sess, LYD_NAME(rpc));
                goto cleanup;
            }

            if (sr_pc_replace_trg_config(user_sess->sess, user_sess->private_ds, NULL, sr_data->tree)) {

sds is a parameter of np2srv_pc_copyconfig() and is never read on this path — the compiler does
not complain because the other branch asserts on it (:409). The read at :398 therefore runs
against ds, which on this path is SR_DS_CANDIDATE, and an unmodified sysrepo candidate mirrors
running. So the private candidate is filled from the shared candidate no matter which conventional
datastore <source> names — and only because an unmodified shared candidate mirrors running does
that look like "running" here.

The reply is <ok/>: nothing fails, the wrong data is simply copied.

Second defect on the same lines

sr_get_data() returns SR_ERR_OK with a NULL result when the selected data is empty — the end
of sr_get_data() in sysrepo/src/sysrepo.c releases the sr_data_t and sets it to NULL when its
tree came out empty:

    if (err_info || !(*data)->tree) {
        sr_release_data(*data);
        *data = NULL;
    }

Only a non-zero return is treated as failure here, so sr_data->tree at :403 dereferences NULL
for an empty source. The same shape is at :421/:427 for the opposite direction
(sr_pc_get_data() out of the private candidate into a conventional datastore).

No trigger is established for either site. In particular a freshly created private candidate is
not empty — it semantically reflects running.

This reproducer does not trigger that path: its whole-datastore reads return non-empty trees,
and a newly created private candidate reflects running. No reachable empty-source trigger has been
established.

Stated narrowly: a successful sr_get_data() that yielded an empty result is dereferenced without
being checked. Nothing here characterises what an empty source should produce — in particular
sr_pc_replace_trg_config(..., NULL) discards private-candidate changes and restores the
running-based view rather than representing an empty replacement, so "crash instead of an empty
copy" would be the wrong framing. Reported as inspection, not as a demonstrated crash.

Trigger

A session that negotiated urn:ietf:params:netconf:capability:private-candidate:1.0 (the server
checks nc_session_cpblt() in np_new_session_cb(), common.c:743), then one RPC:

<copy-config xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
  <target><candidate/></target>
  <source><startup/></source>
</copy-config>

No race and no repetition. The session must be authorised to execute <copy-config> and to access
the source and target data involved; beyond that, nothing special is required. <source> may be
running too — that happens to give the right answer for the wrong reason, but only while the shared
candidate is unmodified, since an unmodified candidate mirrors running.

The reproducer disables NACM so that its default write-deny policy does not reject the NETCONF
<copy-config> operations. (The marker setup itself writes through direct sysrepo calls, which
NACM does not mediate.) The defect itself is independent of NACM.

Not covered

• The NULL-dereference half (see above) is by inspection only.
• The sds == SR_DS_CANDIDATE direction (private candidate → conventional datastore, :419-432) is
  untested and potentially affected by the same wrong-datastore defect. It reads via
  sr_pc_get_data(), which takes an explicit private_ds but builds the view against
  session->ds — sr_modinfo_init(&mod_info, session->conn, session->ds, session->ds, 0) in
  sysrepo/src/utils/private_candidate.c — and netopeer2 has already switched session->ds to the
  target. So candidate → startup would construct the private-candidate view on top of startup
  rather than running.
• The URL variants of copy-config (:536-579) are not exercised.
• No upstream test was added; tests/test_privcand.c would be the right home.

Minimal standalone reproducer

The standalone client below — no test harness, NETCONF only — advertises :private-candidate:1.0
(which netopeer2-cli cannot), seeds a distinct marker in running and startup, copies
startup -> candidate, reads the candidate back, then runs the same copy onto running as a control.

gcc -o privcand_min np2_privcand_copyconfig_min.c $(pkg-config --cflags --libs libnetconf2 libyang)
./privcand_min unix:/path/to/netopeer2.sock       # or ssh:[user@]host[:port]

With the required unmodified shared candidate, exit 0 means the source was honoured, 1 means the
bug was reproduced, and 2 means the test was inconclusive or could not be run.

/*
 * Short reproducer: <copy-config> into a private candidate ignores <source> and reads the SHARED
 * candidate datastore instead.
 *
 * WHY THIS CANNOT BE A netopeer2-cli SCRIPT
 *   The server decides per session:
 *       user_sess->use_private_cand = nc_session_cpblt(new_session,
 *               "urn:ietf:params:netconf:capability:private-candidate:1.0") ? 1 : 0;
 *       (netopeer2 src/common.c:743)
 *   so the CLIENT must advertise that capability in its <hello>. netopeer2-cli offers a fixed set
 *   of capabilities and no way to add one, so it can never take the private-candidate path. The
 *   nc_client_set_capability() call below is the whole reason this is a program.
 *
 * WHAT IT DOES
 *   Sets a different marker in running and in startup, using NETCONF only, then asks for
 *   startup -> candidate and reads the candidate back.
 *
 *       expected: marker-in-startup      (the requested source)
 *       observed: marker-in-running      (running-equivalent data, not the requested source. An
 *                                         unmodified shared candidate mirrors running, so this
 *                                         observation alone cannot tell a direct running read from a
 *                                         shared-candidate read; netopeer2 source inspection
 *                                         identifies the shared-candidate read as the mechanism --
 *                                         the source read runs against the session's datastore,
 *                                         which netopeer2 has already switched to the TARGET)
 *
 *   The control repeats the identical copy onto a target that does not take the private-candidate
 *   path, so the RPC and the datastores are demonstrably right.
 *
 * REQUIREMENTS
 *   - a running netopeer2 with ietf-subscribed-notifications and ietf-yang-push installed (the
 *     marker leaf lives in those modules)
 *   - the server offering :startup, :writable-running and :xpath -- the setup edits running, copies
 *     to and from startup, and reads the marker back with an XPath filter
 *   - a NETCONF user permitted to run <edit-config>, <copy-config> and <get-config>, and to read
 *     and write running, startup AND candidate; NACM's default is write-deny, so an unprivileged
 *     user will see access-denied instead of the defect
 *   - a shared (conventional) candidate known to be unmodified: this test assumes it mirrors
 *     running, and the results are valid only then. Otherwise its existing contents may produce
 *     EITHER verdict (including a false one) or an inconclusive "neither marker" result -- and the
 *     program cannot detect this, because a private-candidate session never sees the shared
 *     candidate directly
 *   - IT OVERWRITES the entire running and startup datastores (copy-config replaces the whole
 *     target) and does NOT restore them. It also replaces the session's PRIVATE candidate, which
 *     the server discards when the session closes; it does not modify the shared candidate. Do not
 *     point it at production.
 *
 * BUILD / RUN
 *     gcc -o privcand_min np2_privcand_copyconfig_min.c $(pkg-config --cflags --libs libnetconf2 libyang)
 *
 *     ./privcand_min unix:/path/to/netopeer2.sock
 *     ./privcand_min ssh:[user@]host[:port]
 *         host may be a DNS name, an IPv4 address, or an IPv6 literal in brackets, e.g.
 *         ssh:user@[::1]:830
 *
 * Exit (with the required unmodified shared candidate): 0 the source was honoured, 1 reproduced,
 * 2 inconclusive or could not be exercised.
 */

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

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

#define CAP_PRIVCAND "urn:ietf:params:netconf:capability:private-candidate:1.0"

#define MARK_RUNNING "marker-in-running"
#define MARK_STARTUP "marker-in-startup"

/* one leaf under /ietf-subscribed-notifications:filters, distinct per datastore */
/* nc_rpc_edit() supplies the <config> wrapper itself, so this is the bare data */
#define EDIT(id) \
    "<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>" id "</filter-id><datastore-xpath-filter>/x</datastore-xpath-filter>" \
    "</selection-filter></filters>"

/**
 * @brief Send one RPC. Returns 0 on success, -1 otherwise.
 *
 * @param[in] want_data If set, the reply must carry data, which is returned in @p out.
 */
static int
rpc(struct nc_session *s, const char *label, struct nc_rpc *r, int want_data, char **out)
{
    struct lyd_node *envp = NULL, *op = NULL;
    const struct lyd_node *child;
    NC_MSG_TYPE t;
    uint64_t msgid = 0;
    int rc = -1;

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

    t = nc_send_rpc(s, r, 5000, &msgid);
    if (t != NC_MSG_RPC) {
        printf("could not send (msgtype %d)\n", t);
        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 (msgtype %d)\n", t);
        goto cleanup;
    }

    child = envp ? lyd_child(envp) : NULL;
    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);
        goto cleanup;
    }

    if (want_data) {
        if (!op) {
            printf("expected data in the reply, got none\n");
            goto cleanup;
        }
        if (lyd_print_mem(out, op, LYD_XML, 0)) {
            printf("could not print the reply data\n");
            goto cleanup;
        }
        printf("data\n");
    } else {
        if (!child || strcmp(LYD_NAME(child), "ok")) {
            printf("expected <ok/>, got <%s>\n", child ? LYD_NAME(child) : "(none)");
            goto cleanup;
        }
        printf("ok\n");
    }
    rc = 0;

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

static int
edit(struct nc_session *s, const char *label, NC_DATASTORE target, const char *cfg)
{
    struct nc_rpc *r;
    int rc;

    r = nc_rpc_edit(target, NC_RPC_EDIT_DFLTOP_REPLACE, NC_RPC_EDIT_TESTOPT_TESTSET,
            NC_RPC_EDIT_ERROPT_STOP, cfg, NC_PARAMTYPE_CONST);
    if (!r) {
        printf("  could not build edit-config\n");
        return -1;
    }
    rc = rpc(s, label, r, 0, NULL);
    nc_rpc_free(r);
    return rc;
}

static int
copy_cfg(struct nc_session *s, const char *label, NC_DATASTORE target, NC_DATASTORE source)
{
    struct nc_rpc *r;
    int rc;

    r = nc_rpc_copy(target, NULL, source, NULL, NC_WD_UNKNOWN, NC_PARAMTYPE_CONST);
    if (!r) {
        printf("  could not build copy-config\n");
        return -1;
    }
    rc = rpc(s, label, r, 0, NULL);
    nc_rpc_free(r);
    return rc;
}

/* Returns 's' for the startup marker, 'r' for the running marker, '?' for neither/both. */
static char
read_marker(struct nc_session *s, const char *label, NC_DATASTORE source)
{
    struct nc_rpc *r;
    char *xml = NULL, got = '?';
    int hs, hr;

    r = nc_rpc_getconfig(source, "/ietf-subscribed-notifications:filters", NC_WD_UNKNOWN,
            NC_PARAMTYPE_CONST);
    if (!r) {
        printf("  could not build get-config\n");
        return '?';
    }
    if (!rpc(s, label, r, 1, &xml)) {
        hs = strstr(xml, MARK_STARTUP) ? 1 : 0;
        hr = strstr(xml, MARK_RUNNING) ? 1 : 0;
        got = (hs && !hr) ? 's' : (hr && !hs) ? 'r' : '?';
    }
    free(xml);
    nc_rpc_free(r);
    return got;
}

int
main(int argc, char **argv)
{
    struct nc_session *s = NULL;
    const char *dst;
    char got;
    int ret = 2;

    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;
    }

    /* THE point of this program: without it the server never takes the private-candidate path */
    if (nc_client_set_capability(CAP_PRIVCAND)) {
        printf("could not advertise %s\n", CAP_PRIVCAND);
        goto cleanup;
    }

    if (!strncmp(dst, "unix:", 5)) {
        s = nc_connect_unix(dst + 5, NULL);
    } else 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 \"%s\" is too long (max %zu chars)\n", dst + 4, sizeof host - 1);
            goto cleanup;
        }
        /* optional user@ */
        if ((at = strchr(host, '@'))) {
            *at = '\0';
            if (nc_client_ssh_set_username(host)) {
                printf("could not set the SSH username\n");
                goto cleanup;
            }
            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) {
                printf("malformed IPv6 literal in ssh destination (missing ']')\n");
                goto cleanup;
            }
            if (rb[1] == ':') {
                portstr = rb + 2;
            } else if (rb[1]) {
                printf("unexpected text after ']' in ssh destination\n");
                goto cleanup;
            }
            *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);
                goto cleanup;
            }
            port = (uint16_t)p;
        }
        s = nc_connect_ssh(host, port, NULL);
    } else {
        fprintf(stderr, "unknown destination \"%s\"\n", dst);
        goto cleanup;
    }
    if (!s) {
        printf("could not connect to %s\n", dst);
        goto cleanup;
    }
    if (!nc_session_cpblt(s, CAP_PRIVCAND)) {
        printf("the server did not advertise %s, so the private-candidate path is not in play\n",
                CAP_PRIVCAND);
        goto cleanup;
    }
    printf("connected, private-candidate capability negotiated\n\n");

    /* Distinct markers in running and startup, over NETCONF only: seed running with the STARTUP
     * marker, push it to startup, then overwrite running with its own. */
    printf("setup: a different marker in each datastore\n");
    if (edit(s, "edit-config running <- startup marker", NC_DATASTORE_RUNNING, EDIT(MARK_STARTUP)) ||
            copy_cfg(s, "copy-config startup <- running", NC_DATASTORE_STARTUP, NC_DATASTORE_RUNNING) ||
            edit(s, "edit-config running <- running marker", NC_DATASTORE_RUNNING, EDIT(MARK_RUNNING))) {
        printf("\nRESULT: could not set the markers up (NACM? missing modules?)\n");
        goto cleanup;
    }

    printf("\nbug case: copy-config into the private candidate\n");
    if (copy_cfg(s, "copy-config candidate <- startup", NC_DATASTORE_CANDIDATE, NC_DATASTORE_STARTUP)) {
        printf("\nRESULT: the copy-config itself did not succeed\n");
        goto cleanup;
    }
    got = read_marker(s, "get-config candidate", NC_DATASTORE_CANDIDATE);
    if (got == '?') {
        printf("\nRESULT: could not read the candidate back\n");
        goto cleanup;
    }
    printf("      candidate holds: %s\n", got == 's' ? MARK_STARTUP " (correct)" : MARK_RUNNING);

    printf("\ncontrol: the same copy onto a target that does not use the private candidate\n");
    if (copy_cfg(s, "copy-config running <- startup", NC_DATASTORE_RUNNING, NC_DATASTORE_STARTUP)) {
        printf("\nRESULT: the control copy-config did not succeed\n");
        goto cleanup;
    }
    if (read_marker(s, "get-config running", NC_DATASTORE_RUNNING) != 's') {
        printf("\nRESULT: the control did not honour <source> either, so this is not specific\n"
               "        to the private-candidate path\n");
        goto cleanup;
    }
    printf("      running holds: " MARK_STARTUP " (correct, so the RPC and the setup are sound)\n");

    printf("\n");
    if (got == 's') {
        printf("--> the requested source was honoured; not reproduced here\n");
        ret = 0;
    } else {
        printf("--> BUG: <source>startup</source> was ignored. The private candidate was filled\n"
               "    from the SHARED candidate, which mirrors running while unmodified -- hence\n"
               "    " MARK_RUNNING ". The source read runs against the session's datastore,\n"
               "    which netopeer2 switched to the TARGET before dispatching\n"
               "    (np2srv_pc_copyconfig(), src/netconf.c:398 reads with no `sds`).\n");
        ret = 1;
    }

cleanup:
    if (s) {
        nc_session_free(s, NULL);
    }
    nc_client_destroy();
    return ret;
}

[michalvasko]

Fine, should be fixed.