lib: fix crashes with leafrefs that point to non-implemented modules

Whenever libyang loads a module that contains a leafref, it will
also implicitly load the module of the referring node if it's
not loaded already. That makes sense as otherwise it wouldn't be
possible to validate the leafref value correctly.

The problem is that loading a module implicitly violates the
assumption of the northbound layer that all loaded modules
are implemented (i.e. they have a northbound node associated
to each schema node). This means that loading a module that
isn't implemented can lead to crashes as the "priv" pointer
of schema nodes is no longer guaranteed to be valid. To fix this
problem, add a few null checks to ignore data nodes associated
to non-implemented modules.

The side effect of this change is harmless. If a daemon receives
configuration it doesn't support (e.g. BFD peers on staticd),
that configuration will be stored but otherwise ignored. This can
only happen when using a northbound client like gRPC, as the CLI
will never send to a daemon a command it doesn't support. This
minor problem should go away in the long run as FRR migrates to
a centralized management model, at which point the YANG-modeled
configuration of all daemons will be maintained in a single place.

Finally, update some daemons to stop implementing YANG modules
they don't need to (i.e. revert 1b741a01c and a74b47f5).

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

diff --git a/lib/northbound.c b/lib/northbound.c
index 18500a8bd29210e12bf84268a499c53d6b3b8287..895647cfb76cf116ab1ed79b89cb640760558440 100644 (file)
--- a/lib/northbound.c
+++ b/lib/northbound.c
@@ -383,6 +383,10 @@ static void nb_config_diff_add_change(struct nb_config_cbs *changes,
 {
        struct nb_config_change *change;
 
+       /* Ignore unimplemented nodes. */
+       if (!dnode->schema->priv)
+               return;
+
        change = XCALLOC(MTYPE_TMP, sizeof(*change));
        change->cb.operation = operation;
        change->cb.seq = *seq;
@@ -416,6 +420,10 @@ static void nb_config_diff_created(const struct lyd_node *dnode, uint32_t *seq,
        enum nb_operation operation;
        struct lyd_node *child;
 
+       /* Ignore unimplemented nodes. */
+       if (!dnode->schema->priv)
+               return;
+
        switch (dnode->schema->nodetype) {
        case LYS_LEAF:
        case LYS_LEAFLIST:
@@ -450,6 +458,10 @@ static void nb_config_diff_created(const struct lyd_node *dnode, uint32_t *seq,
 static void nb_config_diff_deleted(const struct lyd_node *dnode, uint32_t *seq,
                                   struct nb_config_cbs *changes)
 {
+       /* Ignore unimplemented nodes. */
+       if (!dnode->schema->priv)
+               return;
+
        if (nb_operation_is_valid(NB_OP_DESTROY, dnode->schema))
                nb_config_diff_add_change(changes, NB_OP_DESTROY, seq, dnode);
        else if (CHECK_FLAG(dnode->schema->nodetype, LYS_CONTAINER)) {
@@ -618,7 +630,7 @@ static int nb_candidate_validate_code(struct nb_context *context,
                        struct nb_node *nb_node;
 
                        nb_node = child->schema->priv;
-                       if (!nb_node->cbs.pre_validate)
+                       if (!nb_node || !nb_node->cbs.pre_validate)
                                goto next;
 
                        ret = nb_callback_pre_validate(context, nb_node, child,
@@ -1385,7 +1397,7 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction,
                        struct nb_node *nb_node;
 
                        nb_node = dnode->schema->priv;
-                       if (!nb_node->cbs.apply_finish)
+                       if (!nb_node || !nb_node->cbs.apply_finish)
                                goto next;
 
                        /*

diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c
index ee080bca3f22f17e19f6b3be2aa7bcc0135d58f7..6ce520149ab058756860d6274b91ac5d4ee1a9f4 100644 (file)
--- a/lib/northbound_cli.c
+++ b/lib/northbound_cli.c
@@ -573,7 +573,7 @@ void nb_cli_show_dnode_cmds(struct vty *vty, struct lyd_node *root,
                struct nb_node *nb_node;
 
                nb_node = child->schema->priv;
-               if (!nb_node->cbs.cli_show)
+               if (!nb_node || !nb_node->cbs.cli_show)
                        goto next;
 
                /* Skip default values. */
@@ -591,7 +591,7 @@ void nb_cli_show_dnode_cmds(struct vty *vty, struct lyd_node *root,
                parent = ly_iter_next_up(child);
                if (parent != NULL) {
                        nb_node = parent->schema->priv;
-                       if (nb_node->cbs.cli_show_end)
+                       if (nb_node && nb_node->cbs.cli_show_end)
                                (*nb_node->cbs.cli_show_end)(vty, parent);
                }
 

diff --git a/nhrpd/nhrp_main.c b/nhrpd/nhrp_main.c
index 43aa5117c961ba5de5feaf6db764e18186668514..9fc13761c801b832c7c15b4f946dd15762d01f1d 100644 (file)
--- a/nhrpd/nhrp_main.c
+++ b/nhrpd/nhrp_main.c
@@ -119,7 +119,6 @@ static struct quagga_signal_t sighandlers[] = {
 static const struct frr_yang_module_info *const nhrpd_yang_modules[] = {
        &frr_filter_info,
        &frr_interface_info,
-       &frr_vrf_info,
 };
 
 FRR_DAEMON_INFO(nhrpd, NHRP, .vty_port = NHRP_VTY_PORT,

diff --git a/pbrd/pbr_main.c b/pbrd/pbr_main.c
index 0711c66d4a373b66e10f6ae19319da74a6aabc6f..9a9edd79c6712266b4f6c0e26a8fd8ea36f07ea3 100644 (file)
--- a/pbrd/pbr_main.c
+++ b/pbrd/pbr_main.c
@@ -117,7 +117,6 @@ struct quagga_signal_t pbr_signals[] = {
 static const struct frr_yang_module_info *const pbrd_yang_modules[] = {
        &frr_filter_info,
        &frr_interface_info,
-       &frr_vrf_info,
 };
 
 FRR_DAEMON_INFO(pbrd, PBR, .vty_port = PBR_VTY_PORT,