From d0187732cfcc578554f184efbdab42e9a2ae9f48 Mon Sep 17 00:00:00 2001 From: Renato Westphal Date: Fri, 11 Sep 2020 10:43:49 -0300 Subject: [PATCH] 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 --- lib/northbound.c | 16 ++++++++++++++-- lib/northbound_cli.c | 4 ++-- nhrpd/nhrp_main.c | 1 - pbrd/pbr_main.c | 1 - 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lib/northbound.c b/lib/northbound.c index 18500a8bd2..895647cfb7 100644 --- 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 ee080bca3f..6ce520149a 100644 --- 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 43aa5117c9..9fc13761c8 100644 --- 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 0711c66d4a..9a9edd79c6 100644 --- 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, -- 2.39.5