From 3b55aba1958c78b8838ce9ee5eed865d22d2787b Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Sun, 1 Mar 2020 01:19:55 -0500 Subject: [PATCH 01/13] lib: remove unnecessary null checks Signed-off-by: Quentin Young --- lib/if.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/if.c b/lib/if.c index 24b103b3ff..d4d9c4a5a4 100644 --- a/lib/if.c +++ b/lib/if.c @@ -1249,8 +1249,6 @@ struct if_link_params *if_link_params_get(struct interface *ifp) struct if_link_params *iflp = XCALLOC(MTYPE_IF_LINK_PARAMS, sizeof(struct if_link_params)); - if (iflp == NULL) - return NULL; /* Set TE metric equal to standard metric */ iflp->te_metric = ifp->metric; @@ -1278,8 +1276,6 @@ struct if_link_params *if_link_params_get(struct interface *ifp) void if_link_params_free(struct interface *ifp) { - if (ifp->link_params == NULL) - return; XFREE(MTYPE_IF_LINK_PARAMS, ifp->link_params); } From c2b5a4e5ff6135b9f530dbc5a89611cd61fe6419 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Sun, 1 Mar 2020 01:20:40 -0500 Subject: [PATCH 02/13] lib: add STREAM_GETQ, STREAM_GETF Signed-off-by: Quentin Young --- lib/stream.c | 21 +++++++++++++++++++++ lib/stream.h | 20 ++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/lib/stream.c b/lib/stream.c index f046572f41..683a130e44 100644 --- a/lib/stream.c +++ b/lib/stream.c @@ -543,6 +543,27 @@ uint64_t stream_getq(struct stream *s) return q; } +bool stream_getq2(struct stream *s, uint64_t *q) +{ + STREAM_VERIFY_SANE(s); + + if (STREAM_READABLE(s) < sizeof(uint64_t)) { + STREAM_BOUND_WARN2(s, "get uint64"); + return false; + } + + *q = ((uint64_t)s->data[s->getp++]) << 56; + *q |= ((uint64_t)s->data[s->getp++]) << 48; + *q |= ((uint64_t)s->data[s->getp++]) << 40; + *q |= ((uint64_t)s->data[s->getp++]) << 32; + *q |= ((uint64_t)s->data[s->getp++]) << 24; + *q |= ((uint64_t)s->data[s->getp++]) << 16; + *q |= ((uint64_t)s->data[s->getp++]) << 8; + *q |= ((uint64_t)s->data[s->getp++]); + + return true; +} + /* Get next long word from the stream. */ uint32_t stream_get_ipv4(struct stream *s) { diff --git a/lib/stream.h b/lib/stream.h index 6fcf9a53cf..5c7d94fab8 100644 --- a/lib/stream.h +++ b/lib/stream.h @@ -215,6 +215,7 @@ extern bool stream_getl2(struct stream *s, uint32_t *l); extern uint32_t stream_getl_from(struct stream *, size_t); extern uint64_t stream_getq(struct stream *); extern uint64_t stream_getq_from(struct stream *, size_t); +bool stream_getq2(struct stream *s, uint64_t *q); extern uint32_t stream_get_ipv4(struct stream *); /* IEEE-754 floats */ @@ -402,6 +403,25 @@ static inline const uint8_t *ptr_get_be32(const uint8_t *ptr, uint32_t *out) (P) = _pval; \ } while (0) +#define STREAM_GETF(S, P) \ + do { \ + union { \ + float r; \ + uint32_t d; \ + } _pval; \ + if (stream_getl2((S), &_pval.d)) \ + goto stream_failure; \ + (P) = _pval.r; \ + } while (0) + +#define STREAM_GETQ(S, P) \ + do { \ + uint64_t _pval; \ + if (!stream_getq2((S), &_pval)) \ + goto stream_failure; \ + (P) = _pval; \ + } while (0) + #define STREAM_GET(P, STR, SIZE) \ do { \ if (!stream_get2((P), (STR), (SIZE))) \ From efc7191bbef83ff619dcafcd6c91d19dd214ee72 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Sun, 1 Mar 2020 01:21:56 -0500 Subject: [PATCH 03/13] lib: make all zclient.[ch] stream funcs safe Use STREAM_GET* variants of stream getters, which all have non-assert error paths. Signed-off-by: Quentin Young --- lib/zclient.c | 298 ++++++++++++++++++++++++++++++++++++-------------- lib/zclient.h | 5 +- 2 files changed, 220 insertions(+), 83 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index d8c6e002d5..b32be1c310 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -49,8 +49,8 @@ enum event { ZCLIENT_SCHEDULE, ZCLIENT_READ, ZCLIENT_CONNECT }; /* Prototype for event manager. */ static void zclient_event(enum event, struct zclient *); -static void zebra_interface_if_set_value(struct stream *s, - struct interface *ifp); +static int zebra_interface_if_set_value(struct stream *s, + struct interface *ifp); struct zclient_options zclient_options_default = {.receive_notify = false, .synchronous = false}; @@ -1635,33 +1635,34 @@ int zebra_redistribute_default_send(int command, struct zclient *zclient, } /* Get prefix in ZServ format; family should be filled in on prefix */ -static void zclient_stream_get_prefix(struct stream *s, struct prefix *p) +static int zclient_stream_get_prefix(struct stream *s, struct prefix *p) { size_t plen = prefix_blen(p); uint8_t c; p->prefixlen = 0; if (plen == 0) - return; + return -1; - stream_get(&p->u.prefix, s, plen); + STREAM_GET(&p->u.prefix, s, plen); STREAM_GETC(s, c); p->prefixlen = MIN(plen * 8, c); + return 0; stream_failure: - return; + return -1; } /* Router-id update from zebra daemon. */ -void zebra_router_id_update_read(struct stream *s, struct prefix *rid) +int zebra_router_id_update_read(struct stream *s, struct prefix *rid) { /* Fetch interface address. */ STREAM_GETC(s, rid->family); - zclient_stream_get_prefix(s, rid); + return zclient_stream_get_prefix(s, rid); stream_failure: - return; + return -1; } /* Interface addition from zebra daemon. */ @@ -1710,15 +1711,15 @@ stream_failure: * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ */ -static void zclient_vrf_add(struct zclient *zclient, vrf_id_t vrf_id) +static int zclient_vrf_add(struct zclient *zclient, vrf_id_t vrf_id) { struct vrf *vrf; char vrfname_tmp[VRF_NAMSIZ]; struct vrf_data data; - stream_get(&data, zclient->ibuf, sizeof(struct vrf_data)); + STREAM_GET(&data, zclient->ibuf, sizeof(struct vrf_data)); /* Read interface name. */ - stream_get(vrfname_tmp, zclient->ibuf, VRF_NAMSIZ); + STREAM_GET(vrfname_tmp, zclient->ibuf, VRF_NAMSIZ); /* Lookup/create vrf by vrf_id. */ vrf = vrf_get(vrf_id, vrfname_tmp); @@ -1728,6 +1729,10 @@ static void zclient_vrf_add(struct zclient *zclient, vrf_id_t vrf_id) if (vrf_id == VRF_DEFAULT) vrf_set_default_name(vrfname_tmp, false); vrf_enable(vrf); + + return 0; +stream_failure: + return -1; } static void zclient_vrf_delete(struct zclient *zclient, vrf_id_t vrf_id) @@ -1748,14 +1753,14 @@ static void zclient_vrf_delete(struct zclient *zclient, vrf_id_t vrf_id) vrf_delete(vrf); } -static void zclient_interface_add(struct zclient *zclient, vrf_id_t vrf_id) +static int zclient_interface_add(struct zclient *zclient, vrf_id_t vrf_id) { struct interface *ifp; char ifname_tmp[INTERFACE_NAMSIZ]; struct stream *s = zclient->ibuf; /* Read interface name. */ - stream_get(ifname_tmp, s, INTERFACE_NAMSIZ); + STREAM_GET(ifname_tmp, s, INTERFACE_NAMSIZ); /* Lookup/create interface by name. */ ifp = if_get_by_name(ifname_tmp, vrf_id); @@ -1763,6 +1768,10 @@ static void zclient_interface_add(struct zclient *zclient, vrf_id_t vrf_id) zebra_interface_if_set_value(s, ifp); if_new_via_zapi(ifp); + + return 0; +stream_failure: + return -1; } /* @@ -1777,7 +1786,7 @@ struct interface *zebra_interface_state_read(struct stream *s, vrf_id_t vrf_id) char ifname_tmp[INTERFACE_NAMSIZ]; /* Read interface name. */ - stream_get(ifname_tmp, s, INTERFACE_NAMSIZ); + STREAM_GET(ifname_tmp, s, INTERFACE_NAMSIZ); /* Lookup this by interface index. */ ifp = if_lookup_by_name(ifname_tmp, vrf_id); @@ -1791,6 +1800,8 @@ struct interface *zebra_interface_state_read(struct stream *s, vrf_id_t vrf_id) zebra_interface_if_set_value(s, ifp); return ifp; +stream_failure: + return NULL; } static void zclient_interface_delete(struct zclient *zclient, vrf_id_t vrf_id) @@ -1844,21 +1855,23 @@ static void zclient_handle_error(ZAPI_CALLBACK_ARGS) (*zclient->handle_error)(error); } -static void link_params_set_value(struct stream *s, struct if_link_params *iflp) +static int link_params_set_value(struct stream *s, struct if_link_params *iflp) { if (iflp == NULL) - return; + return -1; - iflp->lp_status = stream_getl(s); - iflp->te_metric = stream_getl(s); - iflp->max_bw = stream_getf(s); - iflp->max_rsv_bw = stream_getf(s); - uint32_t bwclassnum = stream_getl(s); + uint32_t bwclassnum; + + STREAM_GETL(s, iflp->lp_status); + STREAM_GETL(s, iflp->te_metric); + STREAM_GETF(s, iflp->max_bw); + STREAM_GETF(s, iflp->max_rsv_bw); + STREAM_GETL(s, bwclassnum); { unsigned int i; for (i = 0; i < bwclassnum && i < MAX_CLASS_TYPE; i++) - iflp->unrsv_bw[i] = stream_getf(s); + STREAM_GETF(s, iflp->unrsv_bw[i]); if (i < bwclassnum) flog_err( EC_LIB_ZAPI_MISSMATCH, @@ -1866,19 +1879,23 @@ static void link_params_set_value(struct stream *s, struct if_link_params *iflp) " - outdated library?", __func__, bwclassnum, MAX_CLASS_TYPE); } - iflp->admin_grp = stream_getl(s); - iflp->rmt_as = stream_getl(s); + STREAM_GETL(s, iflp->admin_grp); + STREAM_GETL(s, iflp->rmt_as); iflp->rmt_ip.s_addr = stream_get_ipv4(s); - iflp->av_delay = stream_getl(s); - iflp->min_delay = stream_getl(s); - iflp->max_delay = stream_getl(s); - iflp->delay_var = stream_getl(s); + STREAM_GETL(s, iflp->av_delay); + STREAM_GETL(s, iflp->min_delay); + STREAM_GETL(s, iflp->max_delay); + STREAM_GETL(s, iflp->delay_var); - iflp->pkt_loss = stream_getf(s); - iflp->res_bw = stream_getf(s); - iflp->ava_bw = stream_getf(s); - iflp->use_bw = stream_getf(s); + STREAM_GETF(s, iflp->pkt_loss); + STREAM_GETF(s, iflp->res_bw); + STREAM_GETF(s, iflp->ava_bw); + STREAM_GETF(s, iflp->use_bw); + + return 0; +stream_failure: + return -1; } struct interface *zebra_interface_link_params_read(struct stream *s, @@ -1887,9 +1904,7 @@ struct interface *zebra_interface_link_params_read(struct stream *s, struct if_link_params *iflp; ifindex_t ifindex; - assert(s); - - ifindex = stream_getl(s); + STREAM_GETL(s, ifindex); struct interface *ifp = if_lookup_by_index(ifindex, vrf_id); @@ -1903,47 +1918,142 @@ struct interface *zebra_interface_link_params_read(struct stream *s, if ((iflp = if_link_params_get(ifp)) == NULL) return NULL; - link_params_set_value(s, iflp); + if (link_params_set_value(s, iflp) != 0) + goto stream_failure; return ifp; + +stream_failure: + return NULL; } -static void zebra_interface_if_set_value(struct stream *s, - struct interface *ifp) +static int zebra_interface_if_set_value(struct stream *s, struct interface *ifp) { + int rc; uint8_t link_params_status = 0; ifindex_t old_ifindex; - old_ifindex = ifp->ifindex; + /* + * XXX: + * Copy interface structure to temporary one. This way if stream + * processing fails partway through, we can safely destroy the + * temporary copy without ever affecting the actual interface, which + * would leave us in a "torn write" situation. If everything succeeds, + * we copy the temporary structure back to the real one and destroy the + * temporary structure. + * + * Obviously all fields of the ifp that could be modified need to be + * copied, so any heap blocks need to be deep copied. In both failure + * and success cases, we need to properly free any allocations that + * result from this deep copy. When modifying this code, take care. + */ + struct interface ifp_tmp = {}; + memcpy(&ifp_tmp, ifp, sizeof(struct interface)); + + /* Not safe to use ifp_tmp in the context of an RB tree */ + memset(&ifp_tmp.name_entry, 0x00, sizeof(ifp_tmp.name_entry)); + memset(&ifp_tmp.index_entry, 0x00, sizeof(ifp_tmp.index_entry)); + + bool ifp_has_link_params = (bool)!!ifp->link_params; + + if (ifp_has_link_params) { + if_link_params_get(&ifp_tmp); + memcpy(ifp_tmp.link_params, ifp->link_params, + sizeof(struct if_link_params)); + } + + /* Read params message */ + + old_ifindex = ifp_tmp.ifindex; + /* Read interface's index. */ - if_set_index(ifp, stream_getl(s)); - ifp->status = stream_getc(s); + STREAM_GETL(s, ifp_tmp.ifindex); + STREAM_GETC(s, ifp_tmp.status); /* Read interface's value. */ - ifp->flags = stream_getq(s); - ifp->ptm_enable = stream_getc(s); - ifp->ptm_status = stream_getc(s); - ifp->metric = stream_getl(s); - ifp->speed = stream_getl(s); - ifp->mtu = stream_getl(s); - ifp->mtu6 = stream_getl(s); - ifp->bandwidth = stream_getl(s); - ifp->link_ifindex = stream_getl(s); - ifp->ll_type = stream_getl(s); - ifp->hw_addr_len = stream_getl(s); - if (ifp->hw_addr_len) - stream_get(ifp->hw_addr, s, - MIN(ifp->hw_addr_len, INTERFACE_HWADDR_MAX)); + STREAM_GETQ(s, ifp_tmp.flags); + STREAM_GETC(s, ifp_tmp.ptm_enable); + STREAM_GETC(s, ifp_tmp.ptm_status); + STREAM_GETL(s, ifp_tmp.metric); + STREAM_GETL(s, ifp_tmp.speed); + STREAM_GETL(s, ifp_tmp.mtu); + STREAM_GETL(s, ifp_tmp.mtu6); + STREAM_GETL(s, ifp_tmp.bandwidth); + STREAM_GETL(s, ifp_tmp.link_ifindex); + STREAM_GETL(s, ifp_tmp.ll_type); + STREAM_GETL(s, ifp_tmp.hw_addr_len); + if (ifp_tmp.hw_addr_len) + STREAM_GET(ifp_tmp.hw_addr, s, + MIN(ifp_tmp.hw_addr_len, INTERFACE_HWADDR_MAX)); /* Read Traffic Engineering status */ - link_params_status = stream_getc(s); + STREAM_GETC(s, link_params_status); /* Then, Traffic Engineering parameters if any */ if (link_params_status) { - struct if_link_params *iflp = if_link_params_get(ifp); - link_params_set_value(s, iflp); + /* if_link_params_get can XCALLOC if they don't exist, beware */ + struct if_link_params *iflp = if_link_params_get(&ifp_tmp); + if (link_params_set_value(s, iflp) != 0) + goto stream_failure; + } + + /* Success - apply changes to ifp_tmp to ifp */ + + /* + * Note: we cannot directly memcpy() because this will overwrite the + * rb_entry, causing many hours of pain + */ + if (if_set_index(ifp, ifp_tmp.ifindex) < 0) + goto stream_failure; + ifp->status = ifp_tmp.status; + ifp->flags = ifp_tmp.flags; + ifp->ptm_enable = ifp_tmp.ptm_enable; + ifp->ptm_status = ifp_tmp.ptm_status; + ifp->metric = ifp_tmp.metric; + ifp->speed = ifp_tmp.speed; + ifp->mtu = ifp_tmp.mtu; + ifp->mtu6 = ifp_tmp.mtu6; + ifp->bandwidth = ifp_tmp.bandwidth; + ifp->link_ifindex = ifp_tmp.link_ifindex; + ifp->ll_type = ifp_tmp.ll_type; + ifp->hw_addr_len = ifp_tmp.hw_addr_len; + ifp->link_params = ifp_tmp.link_params; + + /* + * If ifp had no link params, and link_params_status was nonzero, then + * if_link_params_get created them. We just copied the pointer to ifp, + * so in this case, we use move semantics, and NULL ifp_tmp.link_params + * after copying. This way we do not free ifp->link_params in our later + * free call by virtue of freeing ifp_tmp.link_params, which would make + * ifp->link_params a dangling pointer. + * + * If ifp had no link params, and link_params_status was zero, then the + * pointer is still NULL, and we MUST NOT copy; memcpy(0, _, x != 0) is + * undefined behavior. Nothing was created, so we do not have to free + * either. + * + * If it did have link params, we duplicated them for modification + * purposes, and need to copy if something was changed. This handles + * all four cases. + */ + if (!ifp_has_link_params && ifp_tmp.link_params != NULL) + ifp_tmp.link_params = NULL; + if (ifp_has_link_params && link_params_status != 0) { + assert(ifp->link_params != NULL && ifp_tmp.link_params != NULL); + memcpy(ifp->link_params, ifp_tmp.link_params, + sizeof(struct if_link_params)); } nexthop_group_interface_state_change(ifp, old_ifindex); + + rc = 0; + goto cleanup; +stream_failure: + rc = -1; +cleanup: + /* Free copied resources */ + if_link_params_free(&ifp_tmp); + + return rc; } size_t zebra_interface_link_params_write(struct stream *s, @@ -2042,7 +2152,7 @@ struct connected *zebra_interface_address_read(int type, struct stream *s, memset(&d, 0, sizeof(d)); /* Get interface index. */ - ifindex = stream_getl(s); + STREAM_GETL(s, ifindex); /* Lookup index. */ ifp = if_lookup_by_index(ifindex, vrf_id); @@ -2055,16 +2165,17 @@ struct connected *zebra_interface_address_read(int type, struct stream *s, } /* Fetch flag. */ - ifc_flags = stream_getc(s); + STREAM_GETC(s, ifc_flags); /* Fetch interface address. */ - d.family = p.family = stream_getc(s); + STREAM_GETC(s, d.family); plen = prefix_blen(&d); - zclient_stream_get_prefix(s, &p); + if (zclient_stream_get_prefix(s, &p) != 0) + goto stream_failure; /* Fetch destination address. */ - stream_get(&d.u.prefix, s, plen); + STREAM_GET(&d.u.prefix, s, plen); /* N.B. NULL destination pointers are encoded as all zeroes */ dp = memconstant(&d.u.prefix, 0, plen) ? NULL : &d; @@ -2100,6 +2211,9 @@ struct connected *zebra_interface_address_read(int type, struct stream *s, } return ifc; + +stream_failure: + return NULL; } /* @@ -2135,7 +2249,7 @@ zebra_interface_nbr_address_read(int type, struct stream *s, vrf_id_t vrf_id) struct nbr_connected *ifc; /* Get interface index. */ - ifindex = stream_getl(s); + STREAM_GETL(s, ifindex); /* Lookup index. */ ifp = if_lookup_by_index(ifindex, vrf_id); @@ -2148,9 +2262,9 @@ zebra_interface_nbr_address_read(int type, struct stream *s, vrf_id_t vrf_id) return NULL; } - p.family = stream_getc(s); - stream_get(&p.u.prefix, s, prefix_blen(&p)); - p.prefixlen = stream_getc(s); + STREAM_GETC(s, p.family); + STREAM_GET(&p.u.prefix, s, prefix_blen(&p)); + STREAM_GETC(s, p.prefixlen); if (type == ZEBRA_INTERFACE_NBR_ADDRESS_ADD) { /* Currently only supporting P2P links, so any new RA source @@ -2174,6 +2288,9 @@ zebra_interface_nbr_address_read(int type, struct stream *s, vrf_id_t vrf_id) } return ifc; + +stream_failure: + return NULL; } struct interface *zebra_interface_vrf_update_read(struct stream *s, @@ -2185,7 +2302,7 @@ struct interface *zebra_interface_vrf_update_read(struct stream *s, vrf_id_t new_id; /* Read interface name. */ - stream_get(ifname, s, INTERFACE_NAMSIZ); + STREAM_GET(ifname, s, INTERFACE_NAMSIZ); /* Lookup interface. */ ifp = if_lookup_by_name(ifname, vrf_id); @@ -2197,10 +2314,13 @@ struct interface *zebra_interface_vrf_update_read(struct stream *s, } /* Fetch new VRF Id. */ - new_id = stream_getl(s); + STREAM_GETL(s, new_id); *new_vrf_id = new_id; return ifp; + +stream_failure: + return NULL; } /* filter unwanted messages until the expected one arrives */ @@ -2309,8 +2429,11 @@ int lm_label_manager_connect(struct zclient *zclient, int async) s = zclient->ibuf; /* read instance and proto */ - uint8_t proto = stream_getc(s); - uint16_t instance = stream_getw(s); + uint8_t proto; + uint16_t instance; + + STREAM_GETC(s, proto); + STREAM_GETW(s, instance); /* sanity */ if (proto != zclient->redist_default) @@ -2325,11 +2448,14 @@ int lm_label_manager_connect(struct zclient *zclient, int async) instance, zclient->instance); /* result code */ - result = stream_getc(s); + STREAM_GETC(s, result); if (zclient_debug) zlog_debug("LM connect-response received, result %u", result); return (int)result; + +stream_failure: + return -1; } /* @@ -2437,8 +2563,11 @@ int lm_get_label_chunk(struct zclient *zclient, uint8_t keep, uint32_t base, s = zclient->ibuf; /* read proto and instance */ - uint8_t proto = stream_getc(s); - uint16_t instance = stream_getw(s); + uint8_t proto; + uint8_t instance; + + STREAM_GETC(s, proto); + STREAM_GETW(s, instance); /* sanities */ if (proto != zclient->redist_default) @@ -2460,10 +2589,10 @@ int lm_get_label_chunk(struct zclient *zclient, uint8_t keep, uint32_t base, } /* keep */ - response_keep = stream_getc(s); + STREAM_GETC(s, response_keep); /* start and end labels */ - *start = stream_getl(s); - *end = stream_getl(s); + STREAM_GETL(s, *start); + STREAM_GETL(s, *end); /* not owning this response */ if (keep != response_keep) { @@ -2485,6 +2614,9 @@ int lm_get_label_chunk(struct zclient *zclient, uint8_t keep, uint32_t base, response_keep); return 0; + +stream_failure: + return -1; } /** @@ -2874,7 +3006,7 @@ int zebra_send_pw(struct zclient *zclient, int command, struct zapi_pw *pw) /* * Receive PW status update from Zebra and send it to LDE process. */ -void zebra_read_pw_status_update(ZAPI_CALLBACK_ARGS, struct zapi_pw_status *pw) +int zebra_read_pw_status_update(ZAPI_CALLBACK_ARGS, struct zapi_pw_status *pw) { struct stream *s; @@ -2883,8 +3015,12 @@ void zebra_read_pw_status_update(ZAPI_CALLBACK_ARGS, struct zapi_pw_status *pw) /* Get data. */ stream_get(pw->ifname, s, IF_NAMESIZE); - pw->ifindex = stream_getl(s); - pw->status = stream_getl(s); + STREAM_GETL(s, pw->ifindex); + STREAM_GETL(s, pw->status); + + return 0; +stream_failure: + return -1; } static void zclient_capability_decode(ZAPI_CALLBACK_ARGS) diff --git a/lib/zclient.h b/lib/zclient.h index e747809f16..214226cf5f 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -723,7 +723,7 @@ zebra_interface_nbr_address_read(int, struct stream *, vrf_id_t); extern struct interface *zebra_interface_vrf_update_read(struct stream *s, vrf_id_t vrf_id, vrf_id_t *new_vrf_id); -extern void zebra_router_id_update_read(struct stream *s, struct prefix *rid); +extern int zebra_router_id_update_read(struct stream *s, struct prefix *rid); extern struct interface *zebra_interface_link_params_read(struct stream *s, vrf_id_t vrf_id); @@ -752,7 +752,8 @@ extern int zapi_labels_decode(struct stream *s, struct zapi_labels *zl); extern int zebra_send_pw(struct zclient *zclient, int command, struct zapi_pw *pw); -extern void zebra_read_pw_status_update(ZAPI_CALLBACK_ARGS, struct zapi_pw_status *pw); +extern int zebra_read_pw_status_update(ZAPI_CALLBACK_ARGS, + struct zapi_pw_status *pw); extern int zclient_route_send(uint8_t, struct zclient *, struct zapi_route *); extern int zclient_send_rnh(struct zclient *zclient, int command, From 7239d3d9e6c131c859dae627c1238b5838a5ab8e Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Mon, 2 Mar 2020 18:42:56 -0500 Subject: [PATCH 04/13] lib: handle bogus VRF backend type And use an enum... Signed-off-by: Quentin Young --- lib/vrf.c | 9 +++++++-- lib/vrf.h | 15 +++++++++------ lib/zclient.c | 9 ++++++++- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/lib/vrf.c b/lib/vrf.c index 31ea2d6c4c..fc5aa8f2b6 100644 --- a/lib/vrf.c +++ b/lib/vrf.c @@ -593,10 +593,15 @@ int vrf_get_backend(void) return vrf_backend; } -void vrf_configure_backend(int vrf_backend_netns) +int vrf_configure_backend(enum vrf_backend_type backend) { - vrf_backend = vrf_backend_netns; + if (backend > VRF_BACKEND_MAX) + return -1; + + vrf_backend = backend; vrf_backend_configured = 1; + + return 0; } int vrf_handler_create(struct vty *vty, const char *vrfname, diff --git a/lib/vrf.h b/lib/vrf.h index f231d2433f..2dc2648837 100644 --- a/lib/vrf.h +++ b/lib/vrf.h @@ -101,9 +101,12 @@ RB_PROTOTYPE(vrf_name_head, vrf, name_entry, vrf_name_compare) DECLARE_QOBJ_TYPE(vrf) /* Allow VRF with netns as backend */ -#define VRF_BACKEND_VRF_LITE 0 -#define VRF_BACKEND_NETNS 1 -#define VRF_BACKEND_UNKNOWN 2 +enum vrf_backend_type { + VRF_BACKEND_VRF_LITE, + VRF_BACKEND_NETNS, + VRF_BACKEND_UNKNOWN, + VRF_BACKEND_MAX, +}; extern struct vrf_id_head vrfs_by_id; extern struct vrf_name_head vrfs_by_name; @@ -292,10 +295,10 @@ extern void vrf_install_commands(void); * VRF utilities */ -/* API for configuring VRF backend - * should be called from zebra only +/* + * API for configuring VRF backend */ -extern void vrf_configure_backend(int vrf_backend_netns); +extern int vrf_configure_backend(enum vrf_backend_type backend); extern int vrf_get_backend(void); extern int vrf_is_backend_netns(void); diff --git a/lib/zclient.c b/lib/zclient.c index b32be1c310..673e71c515 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -3031,7 +3031,14 @@ static void zclient_capability_decode(ZAPI_CALLBACK_ARGS) uint8_t mpls_enabled; STREAM_GETL(s, vrf_backend); - vrf_configure_backend(vrf_backend); + + if (vrf_backend < 0 || vrf_configure_backend(vrf_backend)) { + flog_err(EC_LIB_ZAPI_ENCODE, + "%s: Garbage VRF backend type: %d\n", __func__, + vrf_backend); + goto stream_failure; + } + memset(&cap, 0, sizeof(cap)); STREAM_GETC(s, mpls_enabled); From b98edbccf8ab5abf05d334e10328898223ae4263 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Sun, 1 Mar 2020 17:43:53 -0500 Subject: [PATCH 05/13] lib: ensure iface name is null terminated Signed-off-by: Quentin Young --- lib/zclient.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/zclient.c b/lib/zclient.c index 673e71c515..ba4070ffee 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1756,7 +1756,7 @@ static void zclient_vrf_delete(struct zclient *zclient, vrf_id_t vrf_id) static int zclient_interface_add(struct zclient *zclient, vrf_id_t vrf_id) { struct interface *ifp; - char ifname_tmp[INTERFACE_NAMSIZ]; + char ifname_tmp[INTERFACE_NAMSIZ + 1] = {}; struct stream *s = zclient->ibuf; /* Read interface name. */ From 229d0d4edc06e0b5fb38efd08f2c6b6378de2287 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Sun, 1 Mar 2020 17:44:12 -0500 Subject: [PATCH 06/13] lib: don't crash on iface add for unknown vrf If Zebra sends us an interface add notification with a garbage VRF we crash on an assert(vrf_get(vrf_id, NULL)); let's not Signed-off-by: Quentin Young --- lib/zclient.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/zclient.c b/lib/zclient.c index ba4070ffee..e5336d8ec2 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1763,6 +1763,13 @@ static int zclient_interface_add(struct zclient *zclient, vrf_id_t vrf_id) STREAM_GET(ifname_tmp, s, INTERFACE_NAMSIZ); /* Lookup/create interface by name. */ + if (!vrf_get(vrf_id, NULL)) { + zlog_debug( + "Rx'd interface add from Zebra, but VRF %u does not exist", + vrf_id); + return -1; + } + ifp = if_get_by_name(ifname_tmp, vrf_id); zebra_interface_if_set_value(s, ifp); From f17a9d0a0769be32ecde09d63dbee3745d6c1177 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Mon, 2 Mar 2020 18:47:11 -0500 Subject: [PATCH 07/13] lib: more zclient fixes; str termination, vrfs... * Don't crash if we get a request to create an existing VRF * Ensure interface & vrf names are null terminated...again Signed-off-by: Quentin Young --- lib/zclient.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index e5336d8ec2..fd822ba083 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1714,15 +1714,23 @@ stream_failure: static int zclient_vrf_add(struct zclient *zclient, vrf_id_t vrf_id) { struct vrf *vrf; - char vrfname_tmp[VRF_NAMSIZ]; + char vrfname_tmp[VRF_NAMSIZ + 1] = {}; struct vrf_data data; STREAM_GET(&data, zclient->ibuf, sizeof(struct vrf_data)); /* Read interface name. */ STREAM_GET(vrfname_tmp, zclient->ibuf, VRF_NAMSIZ); + if (strlen(vrfname_tmp) == 0) + goto stream_failure; + /* Lookup/create vrf by vrf_id. */ vrf = vrf_get(vrf_id, vrfname_tmp); + + /* Maybe it already exists? */ + if (!vrf && vrf_get(vrf_id, NULL) != NULL) + return 0; + vrf->data.l.table_id = data.l.table_id; memcpy(vrf->data.l.netns_name, data.l.netns_name, NS_NAMSIZ); /* overwrite default vrf */ @@ -1790,7 +1798,7 @@ stream_failure: struct interface *zebra_interface_state_read(struct stream *s, vrf_id_t vrf_id) { struct interface *ifp; - char ifname_tmp[INTERFACE_NAMSIZ]; + char ifname_tmp[INTERFACE_NAMSIZ + 1] = {}; /* Read interface name. */ STREAM_GET(ifname_tmp, s, INTERFACE_NAMSIZ); @@ -1975,6 +1983,10 @@ static int zebra_interface_if_set_value(struct stream *s, struct interface *ifp) /* Read interface's index. */ STREAM_GETL(s, ifp_tmp.ifindex); + + if (ifp_tmp.ifindex < 0) + goto stream_failure; + STREAM_GETC(s, ifp_tmp.status); /* Read interface's value. */ @@ -2304,7 +2316,7 @@ struct interface *zebra_interface_vrf_update_read(struct stream *s, vrf_id_t vrf_id, vrf_id_t *new_vrf_id) { - char ifname[INTERFACE_NAMSIZ]; + char ifname[INTERFACE_NAMSIZ + 1] = {}; struct interface *ifp; vrf_id_t new_id; From 67f81586203a970d89d989aa3cc58a87779de37f Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Mon, 2 Mar 2020 18:50:58 -0500 Subject: [PATCH 08/13] lib: handle failure to change ifindex This fixes a theoretical bug that could occur when trying to change an ifindex on an interface to that of an existing interface. We would remove the interface from the ifindex tree, and change the ifindex, but when we tried to reinsert the interface, the insert would fail. It was impossible to know if this failed due to the insertion / deletion macros capturing the result value of the underlying BSD tree macros. So we would effectively delete the interface. Instead of failing on insert, we just check if the prospective ifindex already exists and return -1 if it does. Macros have been changed to statement expressions so the result can be checked, and bubbled up. Signed-off-by: Quentin Young --- lib/if.c | 26 ++++++++++++++++---- lib/if.h | 75 +++++++++++++++++++++++++++++++++++++------------------- 2 files changed, 71 insertions(+), 30 deletions(-) diff --git a/lib/if.c b/lib/if.c index d4d9c4a5a4..cc964106d0 100644 --- a/lib/if.c +++ b/lib/if.c @@ -582,23 +582,39 @@ struct interface *if_get_by_ifindex(ifindex_t ifindex, vrf_id_t vrf_id) return NULL; } -void if_set_index(struct interface *ifp, ifindex_t ifindex) +int if_set_index(struct interface *ifp, ifindex_t ifindex) { struct vrf *vrf; + if (ifp->ifindex == ifindex) + return 0; + vrf = vrf_get(ifp->vrf_id, NULL); assert(vrf); - if (ifp->ifindex == ifindex) - return; + /* + * If there is already an interface with this ifindex, we will collide + * on insertion, so don't even try. + */ + if (if_lookup_by_ifindex(ifindex, ifp->vrf_id)) + return -1; if (ifp->ifindex != IFINDEX_INTERNAL) IFINDEX_RB_REMOVE(vrf, ifp); ifp->ifindex = ifindex; - if (ifp->ifindex != IFINDEX_INTERNAL) - IFINDEX_RB_INSERT(vrf, ifp) + if (ifp->ifindex != IFINDEX_INTERNAL) { + /* + * This should never happen, since we checked if there was + * already an interface with the desired ifindex at the top of + * the function. Nevertheless. + */ + if (IFINDEX_RB_INSERT(vrf, ifp)) + return -1; + } + + return 0; } void if_set_name(struct interface *ifp, const char *name) diff --git a/lib/if.h b/lib/if.h index 4ca2e79572..ac8d8e70ba 100644 --- a/lib/if.h +++ b/lib/if.h @@ -308,33 +308,58 @@ RB_HEAD(if_index_head, interface); RB_PROTOTYPE(if_index_head, interface, index_entry, if_cmp_index_func) DECLARE_QOBJ_TYPE(interface) -#define IFNAME_RB_INSERT(vrf, ifp) \ - if (RB_INSERT(if_name_head, &vrf->ifaces_by_name, (ifp))) \ - flog_err(EC_LIB_INTERFACE, \ - "%s(%s): corruption detected -- interface with this " \ - "name exists already in VRF %u!", \ - __func__, (ifp)->name, (ifp)->vrf_id); +#define IFNAME_RB_INSERT(vrf, ifp) \ + ({ \ + struct interface *_iz = \ + RB_INSERT(if_name_head, &vrf->ifaces_by_name, (ifp)); \ + if (_iz) \ + flog_err( \ + EC_LIB_INTERFACE, \ + "%s(%s): corruption detected -- interface with this " \ + "name exists already in VRF %u!", \ + __func__, (ifp)->name, (ifp)->vrf_id); \ + _iz; \ + }) -#define IFNAME_RB_REMOVE(vrf, ifp) \ - if (RB_REMOVE(if_name_head, &vrf->ifaces_by_name, (ifp)) == NULL) \ - flog_err(EC_LIB_INTERFACE, \ - "%s(%s): corruption detected -- interface with this " \ - "name doesn't exist in VRF %u!", \ - __func__, (ifp)->name, (ifp)->vrf_id); +#define IFNAME_RB_REMOVE(vrf, ifp) \ + ({ \ + struct interface *_iz = \ + RB_REMOVE(if_name_head, &vrf->ifaces_by_name, (ifp)); \ + if (_iz == NULL) \ + flog_err( \ + EC_LIB_INTERFACE, \ + "%s(%s): corruption detected -- interface with this " \ + "name doesn't exist in VRF %u!", \ + __func__, (ifp)->name, (ifp)->vrf_id); \ + _iz; \ + }) -#define IFINDEX_RB_INSERT(vrf, ifp) \ - if (RB_INSERT(if_index_head, &vrf->ifaces_by_index, (ifp))) \ - flog_err(EC_LIB_INTERFACE, \ - "%s(%u): corruption detected -- interface with this " \ - "ifindex exists already in VRF %u!", \ - __func__, (ifp)->ifindex, (ifp)->vrf_id); -#define IFINDEX_RB_REMOVE(vrf, ifp) \ - if (RB_REMOVE(if_index_head, &vrf->ifaces_by_index, (ifp)) == NULL) \ - flog_err(EC_LIB_INTERFACE, \ - "%s(%u): corruption detected -- interface with this " \ - "ifindex doesn't exist in VRF %u!", \ - __func__, (ifp)->ifindex, (ifp)->vrf_id); +#define IFINDEX_RB_INSERT(vrf, ifp) \ + ({ \ + struct interface *_iz = RB_INSERT( \ + if_index_head, &vrf->ifaces_by_index, (ifp)); \ + if (_iz) \ + flog_err( \ + EC_LIB_INTERFACE, \ + "%s(%u): corruption detected -- interface with this " \ + "ifindex exists already in VRF %u!", \ + __func__, (ifp)->ifindex, (ifp)->vrf_id); \ + _iz; \ + }) + +#define IFINDEX_RB_REMOVE(vrf, ifp) \ + ({ \ + struct interface *_iz = RB_REMOVE( \ + if_index_head, &vrf->ifaces_by_index, (ifp)); \ + if (_iz == NULL) \ + flog_err( \ + EC_LIB_INTERFACE, \ + "%s(%u): corruption detected -- interface with this " \ + "ifindex doesn't exist in VRF %u!", \ + __func__, (ifp)->ifindex, (ifp)->vrf_id); \ + _iz; \ + }) #define FOR_ALL_INTERFACES(vrf, ifp) \ if (vrf) \ @@ -502,7 +527,7 @@ extern struct interface *if_get_by_name(const char *ifname, vrf_id_t vrf_id); extern struct interface *if_get_by_ifindex(ifindex_t ifindex, vrf_id_t vrf_id); /* Sets the index and adds to index list */ -extern void if_set_index(struct interface *ifp, ifindex_t ifindex); +extern int if_set_index(struct interface *ifp, ifindex_t ifindex); /* Sets the name and adds to name list */ extern void if_set_name(struct interface *ifp, const char *name); From 6063ede092b34fd0021852ae661c156eac3b131a Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 18 Mar 2020 12:00:38 -0400 Subject: [PATCH 09/13] lib: improve sanity check on vrf backend value Signed-off-by: Quentin Young --- lib/vrf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vrf.c b/lib/vrf.c index fc5aa8f2b6..14f965ac85 100644 --- a/lib/vrf.c +++ b/lib/vrf.c @@ -595,7 +595,7 @@ int vrf_get_backend(void) int vrf_configure_backend(enum vrf_backend_type backend) { - if (backend > VRF_BACKEND_MAX) + if (backend < 0 || backend >= VRF_BACKEND_MAX) return -1; vrf_backend = backend; From e4f5680d6e15e175895001bec06b9357ec70911a Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 24 Mar 2020 11:38:19 -0400 Subject: [PATCH 10/13] lib: re-add accidentally deleted pfx family set Signed-off-by: Quentin Young --- lib/zclient.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/zclient.c b/lib/zclient.c index fd822ba083..cfce9b1677 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -2188,6 +2188,7 @@ struct connected *zebra_interface_address_read(int type, struct stream *s, /* Fetch interface address. */ STREAM_GETC(s, d.family); + p.family = d.family; plen = prefix_blen(&d); if (zclient_stream_get_prefix(s, &p) != 0) From f7d459250939713b8707fe0fe44d2e7e5dd0276a Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 24 Mar 2020 13:16:06 -0400 Subject: [PATCH 11/13] lib: work around enum issue in old gcc I'd like to keep the explicit check here, but since underlying type of enum is implementation defined, theres some inconsistency using -Wall -Werror in older compilers here Signed-off-by: Quentin Young --- lib/vrf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/vrf.c b/lib/vrf.c index 14f965ac85..e2afa2b231 100644 --- a/lib/vrf.c +++ b/lib/vrf.c @@ -595,8 +595,15 @@ int vrf_get_backend(void) int vrf_configure_backend(enum vrf_backend_type backend) { - if (backend < 0 || backend >= VRF_BACKEND_MAX) + /* Work around issue in old gcc */ + switch (backend) { + case VRF_BACKEND_UNKNOWN: + case VRF_BACKEND_NETNS: + case VRF_BACKEND_VRF_LITE: + break; + default: return -1; + } vrf_backend = backend; vrf_backend_configured = 1; From 41ce53847be14de5e040b02cf2ecb3fe34327b9b Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Wed, 8 Apr 2020 16:29:23 -0400 Subject: [PATCH 12/13] lib: fix if_set_value Something in there is wrong and causing test failures. Moving it back to how it was; we'll stil assert if the message was wrong, just in a different place now. Signed-off-by: Quentin Young --- lib/zclient.c | 147 ++++++++++---------------------------------------- 1 file changed, 29 insertions(+), 118 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index cfce9b1677..c79e2120b4 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -49,8 +49,8 @@ enum event { ZCLIENT_SCHEDULE, ZCLIENT_READ, ZCLIENT_CONNECT }; /* Prototype for event manager. */ static void zclient_event(enum event, struct zclient *); -static int zebra_interface_if_set_value(struct stream *s, - struct interface *ifp); +static void zebra_interface_if_set_value(struct stream *s, + struct interface *ifp); struct zclient_options zclient_options_default = {.receive_notify = false, .synchronous = false}; @@ -1942,137 +1942,48 @@ stream_failure: return NULL; } -static int zebra_interface_if_set_value(struct stream *s, struct interface *ifp) +static void zebra_interface_if_set_value(struct stream *s, + struct interface *ifp) { - int rc; uint8_t link_params_status = 0; - ifindex_t old_ifindex; - - /* - * XXX: - * Copy interface structure to temporary one. This way if stream - * processing fails partway through, we can safely destroy the - * temporary copy without ever affecting the actual interface, which - * would leave us in a "torn write" situation. If everything succeeds, - * we copy the temporary structure back to the real one and destroy the - * temporary structure. - * - * Obviously all fields of the ifp that could be modified need to be - * copied, so any heap blocks need to be deep copied. In both failure - * and success cases, we need to properly free any allocations that - * result from this deep copy. When modifying this code, take care. - */ - struct interface ifp_tmp = {}; - memcpy(&ifp_tmp, ifp, sizeof(struct interface)); - - /* Not safe to use ifp_tmp in the context of an RB tree */ - memset(&ifp_tmp.name_entry, 0x00, sizeof(ifp_tmp.name_entry)); - memset(&ifp_tmp.index_entry, 0x00, sizeof(ifp_tmp.index_entry)); - - bool ifp_has_link_params = (bool)!!ifp->link_params; - - if (ifp_has_link_params) { - if_link_params_get(&ifp_tmp); - memcpy(ifp_tmp.link_params, ifp->link_params, - sizeof(struct if_link_params)); - } - - /* Read params message */ - - old_ifindex = ifp_tmp.ifindex; + ifindex_t old_ifindex, new_ifindex; + old_ifindex = ifp->ifindex; /* Read interface's index. */ - STREAM_GETL(s, ifp_tmp.ifindex); - - if (ifp_tmp.ifindex < 0) - goto stream_failure; - - STREAM_GETC(s, ifp_tmp.status); + STREAM_GETL(s, new_ifindex); + if_set_index(ifp, new_ifindex); + STREAM_GETC(s, ifp->status); /* Read interface's value. */ - STREAM_GETQ(s, ifp_tmp.flags); - STREAM_GETC(s, ifp_tmp.ptm_enable); - STREAM_GETC(s, ifp_tmp.ptm_status); - STREAM_GETL(s, ifp_tmp.metric); - STREAM_GETL(s, ifp_tmp.speed); - STREAM_GETL(s, ifp_tmp.mtu); - STREAM_GETL(s, ifp_tmp.mtu6); - STREAM_GETL(s, ifp_tmp.bandwidth); - STREAM_GETL(s, ifp_tmp.link_ifindex); - STREAM_GETL(s, ifp_tmp.ll_type); - STREAM_GETL(s, ifp_tmp.hw_addr_len); - if (ifp_tmp.hw_addr_len) - STREAM_GET(ifp_tmp.hw_addr, s, - MIN(ifp_tmp.hw_addr_len, INTERFACE_HWADDR_MAX)); + STREAM_GETQ(s, ifp->flags); + STREAM_GETC(s, ifp->ptm_enable); + STREAM_GETC(s, ifp->ptm_status); + STREAM_GETL(s, ifp->metric); + STREAM_GETL(s, ifp->speed); + STREAM_GETL(s, ifp->mtu); + STREAM_GETL(s, ifp->mtu6); + STREAM_GETL(s, ifp->bandwidth); + STREAM_GETL(s, ifp->link_ifindex); + STREAM_GETL(s, ifp->ll_type); + STREAM_GETL(s, ifp->hw_addr_len); + if (ifp->hw_addr_len) + STREAM_GET(ifp->hw_addr, s, + MIN(ifp->hw_addr_len, INTERFACE_HWADDR_MAX)); /* Read Traffic Engineering status */ - STREAM_GETC(s, link_params_status); + link_params_status = stream_getc(s); /* Then, Traffic Engineering parameters if any */ if (link_params_status) { - /* if_link_params_get can XCALLOC if they don't exist, beware */ - struct if_link_params *iflp = if_link_params_get(&ifp_tmp); - if (link_params_set_value(s, iflp) != 0) - goto stream_failure; - } - - /* Success - apply changes to ifp_tmp to ifp */ - - /* - * Note: we cannot directly memcpy() because this will overwrite the - * rb_entry, causing many hours of pain - */ - if (if_set_index(ifp, ifp_tmp.ifindex) < 0) - goto stream_failure; - ifp->status = ifp_tmp.status; - ifp->flags = ifp_tmp.flags; - ifp->ptm_enable = ifp_tmp.ptm_enable; - ifp->ptm_status = ifp_tmp.ptm_status; - ifp->metric = ifp_tmp.metric; - ifp->speed = ifp_tmp.speed; - ifp->mtu = ifp_tmp.mtu; - ifp->mtu6 = ifp_tmp.mtu6; - ifp->bandwidth = ifp_tmp.bandwidth; - ifp->link_ifindex = ifp_tmp.link_ifindex; - ifp->ll_type = ifp_tmp.ll_type; - ifp->hw_addr_len = ifp_tmp.hw_addr_len; - ifp->link_params = ifp_tmp.link_params; - - /* - * If ifp had no link params, and link_params_status was nonzero, then - * if_link_params_get created them. We just copied the pointer to ifp, - * so in this case, we use move semantics, and NULL ifp_tmp.link_params - * after copying. This way we do not free ifp->link_params in our later - * free call by virtue of freeing ifp_tmp.link_params, which would make - * ifp->link_params a dangling pointer. - * - * If ifp had no link params, and link_params_status was zero, then the - * pointer is still NULL, and we MUST NOT copy; memcpy(0, _, x != 0) is - * undefined behavior. Nothing was created, so we do not have to free - * either. - * - * If it did have link params, we duplicated them for modification - * purposes, and need to copy if something was changed. This handles - * all four cases. - */ - if (!ifp_has_link_params && ifp_tmp.link_params != NULL) - ifp_tmp.link_params = NULL; - if (ifp_has_link_params && link_params_status != 0) { - assert(ifp->link_params != NULL && ifp_tmp.link_params != NULL); - memcpy(ifp->link_params, ifp_tmp.link_params, - sizeof(struct if_link_params)); + struct if_link_params *iflp = if_link_params_get(ifp); + link_params_set_value(s, iflp); } nexthop_group_interface_state_change(ifp, old_ifindex); - rc = 0; - goto cleanup; + return; stream_failure: - rc = -1; -cleanup: - /* Free copied resources */ - if_link_params_free(&ifp_tmp); - - return rc; + zlog_err("Could not parse interface values; aborting"); + assert(!"Failed to parse interface values"); } size_t zebra_interface_link_params_write(struct stream *s, From cff0de128d10f1f78144c8c0ca2d2251099d0241 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Mon, 13 Apr 2020 13:24:51 -0400 Subject: [PATCH 13/13] lib: fix SA warning in vrf creation zapi handler Signed-off-by: Quentin Young --- lib/zclient.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index c79e2120b4..5402e9c3c5 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1724,11 +1724,11 @@ static int zclient_vrf_add(struct zclient *zclient, vrf_id_t vrf_id) if (strlen(vrfname_tmp) == 0) goto stream_failure; - /* Lookup/create vrf by vrf_id. */ + /* Lookup/create vrf by name, then vrf_id. */ vrf = vrf_get(vrf_id, vrfname_tmp); - /* Maybe it already exists? */ - if (!vrf && vrf_get(vrf_id, NULL) != NULL) + /* If there's already a VRF with this name, don't create vrf */ + if (!vrf) return 0; vrf->data.l.table_id = data.l.table_id;