LYD_NEW_PATH_OPAQ misclassifies valid leaf and separately supplied leaf-list values as opaque

[karowski]

LYD_NEW_PATH_OPAQ misclassifies valid leaf and separately supplied leaf-list values as opaque

Version: libyang v5.8.6 (47351e59e) and upstream devel — verified present on current head 7142ccfe9.

lyd_new_path2() / lyd_new_path() with LYD_NEW_PATH_OPAQ return LY_SUCCESS but create an
opaque node for a valid value of any concrete type, for leaves and for any leaf-list whose value
arrives through the separate value argument. The documented contract (tree_data.h:1298) is that an
opaque node appears only when the node is not uniquely defined or the value is invalid — "Otherwise a
regular node is created."

Scope — what matters is where the value comes from, not whether the path has a predicate. A
leaf-list value predicate ([.='7']) escapes the defect, because the value travels in the predicate
and is validated elsewhere. Everything that supplies the value through the value argument is
affected — including a path that does carry a predicate, if that predicate is positional. Measured
on head 7142ccfe9 (ll is config true, lls is config false):

/a:ll[.='7']    value predicate           -> REGULAR
/a:lls[.='7']   value predicate           -> REGULAR
/a:ll   = "7"   separate value argument   -> OPAQUE
/a:lls  = "7"   separate value argument   -> OPAQUE
/a:lls[1] = "7" POSITIONAL predicate + separate value argument -> OPAQUE

Regression test for libyang's own suite: four cases for tests/utests/data/test_new.c, in the
final section below. They fail on unpatched head at test_new.c:343 and on v5.8.6 at :329, and pass
once the behaviour is corrected.

Worth noting why the suite does not already catch this: test_path makes six LYD_NEW_PATH_OPAQ
calls, in four scenarios — a list with no predicate (/a:l1), an out-of-range value
(/a:ll3 = "1050"), an empty uint16 (/a:foo), and a list-key group (/a:l11 plus a, b).
Every one expects an opaque node and asserts schema == NULL, which stays true whether or not the
opaque fallback is over-eager. None tests a valid value.

path       value   want     got
/a:c/num   5       term     opaque   uint16 leaf, valid            <-- WRONG
/a:c/s     hello   term     opaque   string leaf, valid            <-- WRONG
/a:c/b     true    term     opaque   boolean leaf, valid           <-- WRONG
/a:top     hello   term     opaque   top-level string leaf, valid  <-- WRONG
/a:ll      7       term     opaque   config-true leaf-list, valid  <-- WRONG
/a:lls     7       term     opaque   config-false leaf-list, valid <-- WRONG
/a:c/num   abc     opaque   opaque   uint16 leaf, not a number
/a:c/num   (null)  opaque   opaque   uint16 leaf, no value

The last two rows are controls: opaque is correct there, and they stay correct under the fix, which
shows the fallback is not simply being disabled.

build	valid values	invalid values
v5.8.6	all opaque	opaque (correct)
devel 68263438a	all opaque	opaque (correct)
v5.8.6, patched locally	all term	opaque (correct)

Impact

A public API returns LY_SUCCESS on valid input and hands back a node that behaves differently from
the one it documents. Measured effects for /a:c/num = "5" on a uint16 leaf (head 7142ccfe9):

downstream operation	result
lyd_validate_all()	fails — Invalid non-number-encoded uint16 value "5" (LY_EVALID)
JSON print	wrong — emits "num": "5", a JSON string, for a numeric type
XML print	looks normal — <num>5</num>, indistinguishable from a regular node
lyd_compare_single() vs the equivalent regular node	unequal

So it is not simply "prints as opaque": XML output is identical, JSON output is silently mistyped,
and the tree cannot be validated afterwards. The XML/JSON asymmetry is the awkward part — a caller
checking XML would see nothing wrong.

Reproducer

#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include <libyang/libyang.h>

/* where libyang's bundled modules live; the Makefile sets this from LY_PREFIX */
#ifndef LY_MODULES_DIR_DEFAULT
# define LY_MODULES_DIR_DEFAULT NULL
#endif

static const char *yang_text =
        "module a {\n"
        "  namespace \"urn:a\";\n"
        "  prefix a;\n"
        "  container c {\n"
        "    leaf num { type uint16; }\n"
        "    leaf s   { type string; }\n"
        "    leaf b   { type boolean; }\n"
        "  }\n"
        "  leaf-list ll  { type uint16; }\n"
        "  leaf-list lls { type uint16; config false; }\n"
        "  leaf top { type string; }\n"
        "}\n";

struct tcase {
    const char *path;
    const char *value;
    int want_term;      /* 1 = a regular term node is required, 0 = opaque is correct */
    const char *why;
};

static const struct tcase cases[] = {
    /* valid values: every one of these must produce a term node */
    {"/a:c/num", "5",     1, "uint16 leaf, valid"},
    {"/a:c/s",   "hello", 1, "string leaf, valid"},
    {"/a:c/b",   "true",  1, "boolean leaf, valid"},
    {"/a:top",   "hello", 1, "top-level string leaf, valid"},
    {"/a:ll",    "7",     1, "config-true leaf-list, valid"},
    {"/a:lls",   "7",     1, "config-false leaf-list, valid"},
    /* genuinely invalid: opaque IS the documented behaviour, so these are the
     * controls that show a fix does not simply disable the fallback */
    {"/a:c/num", "abc",   0, "uint16 leaf, not a number"},
    {"/a:c/num", NULL,    0, "uint16 leaf, no value"},
};

int
main(void)
{
    const char *mod_dir = getenv("LY_MODULES_DIR");
    struct ly_ctx *ctx = NULL;
    int bad = 0, ret = 2;
    size_t i;

    setvbuf(stdout, NULL, _IOLBF, 0);
    if (!mod_dir) {
        mod_dir = LY_MODULES_DIR_DEFAULT;
    }
    if (ly_ctx_new(mod_dir, 0, &ctx)) {
        fprintf(stderr, "ly_ctx_new failed (module dir \"%s\")\n", mod_dir ? mod_dir : "(none)");
        return 2;
    }
    if (lys_parse_mem(ctx, yang_text, LYS_IN_YANG, NULL)) {
        fprintf(stderr, "lys_parse_mem failed\n");
        goto cleanup;
    }

    printf("lyd_new_path2(..., LYD_NEW_PATH_OPAQ, ...)\n\n");
    printf("  %-10s %-7s %-8s %-8s %s\n", "path", "value", "want", "got", "case");
    for (i = 0; i < sizeof cases / sizeof *cases; ++i) {
        const struct tcase *t = &cases[i];
        struct lyd_node *root = NULL, *node = NULL;
        const char *got;
        LY_ERR rc;

        rc = lyd_new_path2(NULL, ctx, t->path, t->value, 0, 0, LYD_NEW_PATH_OPAQ, &root, &node);
        if (rc) {
            got = "ERR";
        } else if (!node) {
            got = "(none)";
        } else {
            got = node->schema ? "term" : "opaque";
        }
        printf("  %-10s %-7s %-8s %-8s %s%s\n", t->path, t->value ? t->value : "(null)",
                t->want_term ? "term" : "opaque", got, t->why,
                strcmp(got, t->want_term ? "term" : "opaque") ? "   <-- WRONG" : "");
        if (strcmp(got, t->want_term ? "term" : "opaque")) {
            bad = 1;
        }
        lyd_free_all(root);
    }

    printf("\n");
    if (bad) {
        printf("  --> BUG: LYD_NEW_PATH_OPAQ degraded a valid value to an opaque node\n");
        printf("RESULT: bug reproduced\n");
        ret = 1;
    } else {
        printf("  --> ok: term for valid values, opaque only for invalid ones\n");
        printf("RESULT: contract honoured\n");
        ret = 0;
    }

cleanup:
    ly_ctx_destroy(ctx);
    return ret;
}

Regression test for libyang's own suite

Four cases for tests/utests/data/test_new.c (test_path), covering each distinct path that
reaches the misclassification, plus the positional-predicate boundary. Applies to v5.8.6 and to current devel. No schema change is needed — schema_a already
defines foo, ll and ll2 in both versions.

Verified:

build	utest_new
devel head 7142ccfe9, unmodified suite	passes — the defect is invisible to it
head + this test only	fails at test_new.c:343
head + this test + a partial correction	fails at test_new.c:365
head + this test + full fix	passes 4/4, incl. utest_new_valgrind
v5.8.6 + this test only	fails at test_new.c:329
v5.8.6 + this test + full fix	passes 4/4, incl. utest_new_valgrind

The third row is the point: the cases are not interchangeable — a correction that covers only some of
the affected paths still fails the test.

diff --git a/tests/utests/data/test_new.c b/tests/utests/data/test_new.c
index 13f8d1fc0..3a84314bb 100644
--- a/tests/utests/data/test_new.c
+++ b/tests/utests/data/test_new.c
@@ -323,6 +323,63 @@ test_path(void **state)
 
     lyd_free_tree(root);
 
+    /* LYD_NEW_PATH_OPAQ with a VALID value must still create a REGULAR node.
+     *
+     * The LYD_NEW_PATH_OPAQ cases above are all ones where an opaque node is the correct result --
+     * a list with no predicate, an invalid or empty value -- and they assert schema == NULL, which
+     * stays true whether or not the opaque fallback is over-eager. So none of them can detect it.
+     * These cases pin the other half of the documented contract: "the node is created opaq only if
+     * it is not uniquely defined or the value is invalid. Otherwise a regular node is created."
+     * (tree_data.h)
+     *
+     * There are three affected call sites, and a case is needed for each or a partial fix passes:
+     * the leaf check, the ordinary leaf-list check, and a third reached only when
+     * lysc_is_dup_inst_list() holds -- i.e. for a state (config false) leaf-list. */
+
+    /* uint16 leaf with a valid number -- exercises the DECNUM/OCTNUM/HEXNUM hint family */
+    ret = lyd_new_path2(NULL, UTEST_LYCTX, "/a:foo", "42", 0, 0, LYD_NEW_PATH_OPAQ, NULL, &root);
+    assert_int_equal(ret, LY_SUCCESS);
+    assert_non_null(root);
+    assert_non_null(root->schema);
+    assert_string_equal("foo", root->schema->name);
+    assert_string_equal("42", lyd_get_value(root));
+
+    lyd_free_tree(root);
+
+    /* config-true string leaf-list -- exercises LYD_VALHINT_STRING, which has no fallback */
+    ret = lyd_new_path2(NULL, UTEST_LYCTX, "/a:ll", "abc", 0, 0, LYD_NEW_PATH_OPAQ, NULL, &root);
+    assert_int_equal(ret, LY_SUCCESS);
+    assert_non_null(root);
+    assert_non_null(root->schema);
+    assert_string_equal("ll", root->schema->name);
+    assert_string_equal("abc", lyd_get_value(root));
+
+    lyd_free_tree(root);
+
+    /* Config-false leaf-list exercising the duplicate-instance pre-check. */
+    ret = lyd_new_path2(NULL, UTEST_LYCTX, "/a:ll2", "val", 0, 0, LYD_NEW_PATH_OPAQ, NULL, &root);
+    assert_int_equal(ret, LY_SUCCESS);
+    assert_non_null(root);
+    assert_non_null(root->schema);
+    assert_string_equal("ll2", root->schema->name);
+    assert_string_equal("val", lyd_get_value(root));
+
+    lyd_free_tree(root);
+
+    /* Same state leaf-list, but with a POSITIONAL predicate and the value still supplied separately.
+     * What decides whether a leaf-list is affected is where the value comes from, not whether the
+     * path carries a predicate: a leaf-list VALUE predicate (/a:ll2[.='val']) is validated elsewhere
+     * and was never affected, while a positional predicate leaves the value on the `value` argument
+     * and so goes through the same check. */
+    ret = lyd_new_path2(NULL, UTEST_LYCTX, "/a:ll2[1]", "val", 0, 0, LYD_NEW_PATH_OPAQ, NULL, &root);
+    assert_int_equal(ret, LY_SUCCESS);
+    assert_non_null(root);
+    assert_non_null(root->schema);
+    assert_string_equal("ll2", root->schema->name);
+    assert_string_equal("val", lyd_get_value(root));
+
+    lyd_free_tree(root);
+
     /* key-less list */
     ret = lyd_new_path2(NULL, UTEST_LYCTX, "/a:c2/l3/x", "val1", 0, 0, 0, &root, &node);
     assert_int_equal(ret, LY_SUCCESS);

[michalvasko]

Thanks, fixed.