From 5b45194603f456e384053fe4e7f334fbcca7fa44 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 May 2024 16:49:49 -0400 Subject: [PATCH 01/12] isisd: Translate level appropriately Sanitizers are finding: isisd/isis_spf.c:2122:22: runtime error: index 2 out of bounds for type '_uint64_t [2]' Comparing the pattern against the rest of the code, 1 should be subtracted. Signed-off-by: Donald Sharp --- isisd/isis_spf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/isisd/isis_spf.c b/isisd/isis_spf.c index 86302076f82d..b9f38c6a070c 100644 --- a/isisd/isis_spf.c +++ b/isisd/isis_spf.c @@ -2124,7 +2124,7 @@ static void isis_run_spf_cb(struct event *thread) } if (have_run) - area->spf_run_count[level]++; + area->spf_run_count[level - 1]++; isis_area_verify_routes(area); From 28515ee1f44a9c0b72781f28bba81aba2c77f039 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 May 2024 20:14:42 -0400 Subject: [PATCH 02/12] *: Cleanup no returns for functions that should have them Signed-off-by: Donald Sharp --- bgpd/bgp_addpath.c | 1 + bgpd/bgp_route.c | 1 + bgpd/bgp_vty.c | 1 + bgpd/bgpd.h | 1 + bgpd/rfapi/rfapi_import.c | 1 + isisd/isis_adjacency.c | 2 ++ isisd/isis_pdu_counter.c | 1 + isisd/isisd.c | 1 + ldpd/lde_lib.c | 2 ++ lib/command_graph.c | 1 + lib/command_match.c | 3 +++ lib/ipaddr.h | 2 ++ lib/northbound.c | 5 +++++ lib/prefix.c | 3 +++ pathd/path_cli.c | 1 + pathd/path_pcep_config.c | 2 ++ pathd/path_pcep_controller.c | 2 ++ pathd/path_pcep_debug.c | 8 ++++++++ pathd/path_pcep_pcc.c | 2 ++ pathd/pathd.c | 3 +++ ripd/rip_nb_state.c | 2 ++ tests/bgpd/test_peer_attr.c | 1 + zebra/router-id.c | 1 + 23 files changed, 47 insertions(+) diff --git a/bgpd/bgp_addpath.c b/bgpd/bgp_addpath.c index f391c138472d..bba77c0b14d0 100644 --- a/bgpd/bgp_addpath.c +++ b/bgpd/bgp_addpath.c @@ -177,6 +177,7 @@ bool bgp_addpath_tx_path(enum bgp_addpath_strat strat, struct bgp_path_info *pi) } assert(!"Reached end of function we should never hit"); + return false; } static void bgp_addpath_flush_type_rn(struct bgp *bgp, afi_t afi, safi_t safi, diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 2a9fc6ce0da2..5c7d570ef5f9 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -12479,6 +12479,7 @@ const struct prefix_rd *bgp_rd_from_dest(const struct bgp_dest *dest, } assert(!"Reached end of function when we were not expecting it"); + return NULL; } /* Display specified route of BGP table. */ diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 828fa711b2f0..42257ce9600f 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -594,6 +594,7 @@ static const char *get_bgp_default_af_flag(afi_t afi, safi_t safi) /* all AFIs are accounted for above, so this shouldn't happen */ assert(!"Reached end of function where we did not expect to"); + return NULL; } int bgp_get_vty(struct bgp **bgp, as_t *as, const char *name, diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index b733be9f0dd8..c92293a84412 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -2632,6 +2632,7 @@ static inline int afindex(afi_t afi, safi_t safi) } assert(!"Reached end of function we should never hit"); + return BGP_AF_MAX; } /* If the peer is not a peer-group but is bound to a peer-group return 1 */ diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 2afcb2f45c8c..33eea6ad6aa8 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -3809,6 +3809,7 @@ rfapiBgpInfoFilteredImportFunction(safi_t safi) } assert(!"Reached end of function when we were not expecting to"); + return rfapiBgpInfoFilteredImportBadSafi; } void rfapiProcessUpdate(struct peer *peer, diff --git a/isisd/isis_adjacency.c b/isisd/isis_adjacency.c index 3ed6fe95f52f..50b0d2ad138c 100644 --- a/isisd/isis_adjacency.c +++ b/isisd/isis_adjacency.c @@ -486,6 +486,7 @@ const char *isis_adj_yang_state(enum isis_adj_state state) } assert(!"Reached end of function where we are not expecting to"); + return "failed"; } void isis_adj_expire(struct event *thread) @@ -946,4 +947,5 @@ int isis_adj_usage2levels(enum isis_adj_usage usage) } assert(!"Reached end of function where we are not expecting to"); + return 0; } diff --git a/isisd/isis_pdu_counter.c b/isisd/isis_pdu_counter.c index a3605a32a162..a2ff3d70543e 100644 --- a/isisd/isis_pdu_counter.c +++ b/isisd/isis_pdu_counter.c @@ -69,6 +69,7 @@ static const char *pdu_counter_index_to_name(enum pdu_counter_index index) } assert(!"Reached end of function where we were not expecting to"); + return "??????"; } void pdu_counter_count(pdu_counter_t counter, uint8_t pdu_type) diff --git a/isisd/isisd.c b/isisd/isisd.c index e67f5fb1c867..0b14c3f2c4ab 100644 --- a/isisd/isisd.c +++ b/isisd/isisd.c @@ -2355,6 +2355,7 @@ static const char *pdu_counter_index_to_name_json(enum pdu_counter_index index) } assert(!"Reached end of function where we are not expecting to"); + return "??????"; } static void common_isis_summary_json(struct json_object *json, diff --git a/ldpd/lde_lib.c b/ldpd/lde_lib.c index 04bff9015856..58ac4f378454 100644 --- a/ldpd/lde_lib.c +++ b/ldpd/lde_lib.c @@ -1008,6 +1008,8 @@ lde_wildcard_apply(struct map *wcard, struct fec *fec, struct lde_map *me) default: fatalx("lde_wildcard_apply: unexpected fec type"); } + + return 0; } /* gabage collector timer: timer to remove dead entries from the LIB */ diff --git a/lib/command_graph.c b/lib/command_graph.c index 20ab6b321b5c..87dc4e0281c8 100644 --- a/lib/command_graph.c +++ b/lib/command_graph.c @@ -274,6 +274,7 @@ static bool cmd_nodes_equal(struct graph_node *ga, struct graph_node *gb) } assert(!"Reached end of function we should never hit"); + return false; } static void cmd_fork_bump_attr(struct graph_node *gn, struct graph_node *join, diff --git a/lib/command_match.c b/lib/command_match.c index 97e6aeb4698a..25f3035b9618 100644 --- a/lib/command_match.c +++ b/lib/command_match.c @@ -547,6 +547,7 @@ static enum match_type min_match_level(enum cmd_token_type type) } assert(!"Reached end of function we should never hit"); + return no_match; } /** @@ -582,6 +583,7 @@ static int score_precedence(enum cmd_token_type type) } assert(!"Reached end of function we should never hit"); + return 10; } /** @@ -712,6 +714,7 @@ static enum match_type match_token(struct cmd_token *token, char *input_token) } assert(!"Reached end of function we should never hit"); + return no_match; } #define IPV4_ADDR_STR "0123456789." diff --git a/lib/ipaddr.h b/lib/ipaddr.h index 888955fba0ab..7cd618af2b33 100644 --- a/lib/ipaddr.h +++ b/lib/ipaddr.h @@ -61,6 +61,7 @@ static inline int ipaddr_family(const struct ipaddr *ip) } assert(!"Reached end of function where we should never hit"); + return AF_UNSPEC; } static inline int str2ipaddr(const char *str, struct ipaddr *ip) @@ -151,6 +152,7 @@ static inline int ipaddr_cmp(const struct ipaddr *a, const struct ipaddr *b) } assert(!"Reached end of function we should never hit"); + return 0; } static inline bool ipaddr_is_zero(const struct ipaddr *ip) diff --git a/lib/northbound.c b/lib/northbound.c index 0bc79d027740..1f32175fc357 100644 --- a/lib/northbound.c +++ b/lib/northbound.c @@ -1058,6 +1058,7 @@ const char *nb_operation_name(enum nb_operation operation) } assert(!"Reached end of function we should never hit"); + return "unknown"; } bool nb_is_operation_allowed(struct nb_node *nb_node, enum nb_operation oper) @@ -2560,6 +2561,7 @@ const char *nb_event_name(enum nb_event event) } assert(!"Reached end of function we should never hit"); + return "unknown"; } const char *nb_cb_operation_name(enum nb_cb_operation operation) @@ -2592,6 +2594,7 @@ const char *nb_cb_operation_name(enum nb_cb_operation operation) } assert(!"Reached end of function we should never hit"); + return "unknown"; } const char *nb_err_name(enum nb_error error) @@ -2618,6 +2621,7 @@ const char *nb_err_name(enum nb_error error) } assert(!"Reached end of function we should never hit"); + return "unknown"; } const char *nb_client_name(enum nb_client client) @@ -2640,6 +2644,7 @@ const char *nb_client_name(enum nb_client client) } assert(!"Reached end of function we should never hit"); + return "unknown"; } static void nb_load_callbacks(const struct frr_yang_module_info *module) diff --git a/lib/prefix.c b/lib/prefix.c index 2485c3e61bdd..ae541aa5124d 100644 --- a/lib/prefix.c +++ b/lib/prefix.c @@ -138,6 +138,7 @@ const char *afi2str_lower(afi_t afi) } assert(!"Reached end of function we should never reach"); + return "bad-value"; } const char *afi2str(afi_t afi) @@ -155,6 +156,7 @@ const char *afi2str(afi_t afi) } assert(!"Reached end of function we should never reach"); + return "bad-value"; } const char *safi2str(safi_t safi) @@ -180,6 +182,7 @@ const char *safi2str(safi_t safi) } assert(!"Reached end of function we should never reach"); + return "unknown"; } /* If n includes p prefix then return 1 else return 0. */ diff --git a/pathd/path_cli.c b/pathd/path_cli.c index e22931c13e1e..425b48321298 100644 --- a/pathd/path_cli.c +++ b/pathd/path_cli.c @@ -1077,6 +1077,7 @@ static const char *objfun_type_name(enum objfun_type type) } assert(!"Reached end of function we should never hit"); + return NULL; } DEFPY_NOSH(show_debugging_pathd, show_debugging_pathd_cmd, diff --git a/pathd/path_pcep_config.c b/pathd/path_pcep_config.c index da7ee89f2fef..7c5131366e7d 100644 --- a/pathd/path_pcep_config.c +++ b/pathd/path_pcep_config.c @@ -483,6 +483,7 @@ status_int_to_ext(enum srte_policy_status status) } assert(!"Reached end of function where we are not expecting to"); + return PCEP_LSP_OPERATIONAL_DOWN; } enum pcep_sr_subobj_nai pcep_nai_type(enum srte_segment_nai_type type) @@ -536,4 +537,5 @@ enum srte_segment_nai_type srte_nai_type(enum pcep_sr_subobj_nai type) } assert(!"Reached end of function where we were not expecting to"); + return SRTE_SEGMENT_NAI_TYPE_NONE; } diff --git a/pathd/path_pcep_controller.c b/pathd/path_pcep_controller.c index a00a11408668..9e0752113ef2 100644 --- a/pathd/path_pcep_controller.c +++ b/pathd/path_pcep_controller.c @@ -1071,6 +1071,7 @@ const char *timer_type_name(enum pcep_ctrl_timer_type type) } assert(!"Reached end of function where we did not expect to"); + return "UNKNOWN"; } const char *timeout_type_name(enum pcep_ctrl_timeout_type type) @@ -1085,4 +1086,5 @@ const char *timeout_type_name(enum pcep_ctrl_timeout_type type) } assert(!"Reached end of function where we did not expect to"); + return "UNKNOWN"; } diff --git a/pathd/path_pcep_debug.c b/pathd/path_pcep_debug.c index 7bff9c7b9c0e..ac9b793822aa 100644 --- a/pathd/path_pcep_debug.c +++ b/pathd/path_pcep_debug.c @@ -80,6 +80,7 @@ const char *pcc_status_name(enum pcc_status status) } assert(!"Reached end of function where we do not expect to"); + return "UNKNOWN"; } const char *pcep_event_type_name(pcep_event_type event_type) @@ -112,6 +113,7 @@ const char *pcep_event_type_name(pcep_event_type event_type) } assert(!"Reached end of function where we do not expect to"); + return "UNKNOWN"; } const char *pcep_error_type_name(enum pcep_error_type error_type) @@ -636,6 +638,7 @@ const char *pcep_message_type_name(enum pcep_message_types pcep_message_type) } assert(!"Reached end of function where we are not expecting to"); + return "UNKNOWN"; } const char *pcep_object_class_name(enum pcep_object_classes obj_class) @@ -692,6 +695,7 @@ const char *pcep_object_class_name(enum pcep_object_classes obj_class) } assert(!"Reached end of function where we are not expecting to"); + return "UNKNOWN"; } const char *pcep_object_type_name(enum pcep_object_classes obj_class, @@ -767,6 +771,7 @@ const char *pcep_lsp_status_name(enum pcep_lsp_operational_status status) } assert(!"Reached end of function where we do not expect to"); + return "UNKNOWN"; } @@ -818,6 +823,7 @@ const char *pcep_tlv_type_name(enum pcep_object_tlv_types tlv_type) } assert(!"Reached end of function where we do not expect to"); + return "UNKNOWN"; } const char *pcep_ro_type_name(enum pcep_ro_subobj_types ro_type) @@ -841,6 +847,7 @@ const char *pcep_ro_type_name(enum pcep_ro_subobj_types ro_type) } assert(!"Reached end of function where we do not expect to"); + return "UNKNOWN"; } const char *pcep_nai_type_name(enum pcep_sr_subobj_nai nai_type) @@ -865,6 +872,7 @@ const char *pcep_nai_type_name(enum pcep_sr_subobj_nai nai_type) } assert(!"Reached end of function where we do not expect to"); + return "UNKNOWN"; } const char *pcep_metric_type_name(enum pcep_metric_types type) diff --git a/pathd/path_pcep_pcc.c b/pathd/path_pcep_pcc.c index f18eff28888a..3f5aa59bba3f 100644 --- a/pathd/path_pcep_pcc.c +++ b/pathd/path_pcep_pcc.c @@ -441,6 +441,7 @@ int pcep_pcc_disable(struct ctrl_state *ctrl_state, struct pcc_state *pcc_state) } assert(!"Reached end of function where we are not expecting to"); + return 0; } void pcep_pcc_sync_path(struct ctrl_state *ctrl_state, @@ -1955,6 +1956,7 @@ static uint32_t hash_nbkey(const struct lsp_nb_key *nbkey) } assert(!"Reached end of function where we were not expecting to"); + return hash; } static int cmp_nbkey(const struct lsp_nb_key *a, const struct lsp_nb_key *b) diff --git a/pathd/pathd.c b/pathd/pathd.c index 6c13503c7dcc..3b55270e1f4b 100644 --- a/pathd/pathd.c +++ b/pathd/pathd.c @@ -1022,6 +1022,7 @@ static uint32_t filter_type_to_flag(enum affinity_filter_type type) } assert(!"Reached end of function we should never hit"); + return 0; } static const char *filter_type_name(enum affinity_filter_type type) @@ -1038,6 +1039,7 @@ static const char *filter_type_name(enum affinity_filter_type type) } assert(!"Reached end of function we should never hit"); + return "unknown"; } /** @@ -1269,6 +1271,7 @@ const char *srte_origin2str(enum srte_protocol_origin origin) } assert(!"Reached end of function we should never hit"); + return "Unknown"; } void path_policy_show_debugging(struct vty *vty) diff --git a/ripd/rip_nb_state.c b/ripd/rip_nb_state.c index fa0d382a0e09..3fb8d88bacbe 100644 --- a/ripd/rip_nb_state.c +++ b/ripd/rip_nb_state.c @@ -392,6 +392,7 @@ struct yang_data *ripd_instance_state_routes_route_next_hop_get_elem( } assert(!"Reached end of function where we do not expect to reach"); + return NULL; } /* @@ -418,6 +419,7 @@ struct yang_data *ripd_instance_state_routes_route_interface_get_elem( } assert(!"Reached end of function where we do not expect to reach"); + return NULL; } /* diff --git a/tests/bgpd/test_peer_attr.c b/tests/bgpd/test_peer_attr.c index 767c41cfee67..4afc9ccea994 100644 --- a/tests/bgpd/test_peer_attr.c +++ b/tests/bgpd/test_peer_attr.c @@ -655,6 +655,7 @@ static const char *str_from_afi(afi_t afi) } assert(!"Reached end of function we should never reach"); + return "bad-value"; } static const char *str_from_attr_type(enum test_peer_attr_type at) diff --git a/zebra/router-id.c b/zebra/router-id.c index 2f251a79e584..54fd37c85f9b 100644 --- a/zebra/router-id.c +++ b/zebra/router-id.c @@ -107,6 +107,7 @@ int router_id_get(afi_t afi, struct prefix *p, struct zebra_vrf *zvrf) } assert(!"Reached end of function we should never hit"); + return -1; } int router_id_set(afi_t afi, struct prefix *p, struct zebra_vrf *zvrf) From ff8b83eb0991fa334a454f3db5a02e3c0f15c3de Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 May 2024 20:23:57 -0400 Subject: [PATCH 03/12] lib: qstrdup can return NULL In fact there are more than several places that we do in fact pass in NULL to get a NULL returned. Signed-off-by: Donald Sharp --- lib/memory.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/memory.h b/lib/memory.h index 65b99a5fc945..03d2ad875550 100644 --- a/lib/memory.h +++ b/lib/memory.h @@ -146,8 +146,7 @@ extern void *qcalloc(struct memtype *mt, size_t size) __attribute__((malloc, _ALLOC_SIZE(2), nonnull(1) _RET_NONNULL)); extern void *qrealloc(struct memtype *mt, void *ptr, size_t size) __attribute__((_ALLOC_SIZE(3), nonnull(1) _RET_NONNULL)); -extern void *qstrdup(struct memtype *mt, const char *str) - __attribute__((malloc, nonnull(1) _RET_NONNULL)); +extern void *qstrdup(struct memtype *mt, const char *str); extern void qcountfree(struct memtype *mt, void *ptr) __attribute__((nonnull(1))); extern void qfree(struct memtype *mt, void *ptr) __attribute__((nonnull(1))); From 7cb8bd8a441712ef3955c2a4eb8f925a2b1cdddb Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 May 2024 20:35:57 -0400 Subject: [PATCH 04/12] lib: Prevent Pointer dereference to NULL The call into zlog_target_clone was passing &zcf->active->zt. From running memory sanitizer we are seeing that it is complaining about member access within null pointer of type... for this value. Since the call into zlog_target_clone checks for NULL for this value, let's just make sure we don't do anything stupid here. Signed-off-by: Donald Sharp --- lib/zlog_5424.c | 3 ++- lib/zlog_targets.c | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/zlog_5424.c b/lib/zlog_5424.c index 4c60d4b40564..53acfecec176 100644 --- a/lib/zlog_5424.c +++ b/lib/zlog_5424.c @@ -742,7 +742,8 @@ static void zlog_5424_cycle(struct zlog_cfg_5424 *zcf, int fd) /* all of this is swapped in by zlog_target_replace() below, * the old target is RCU-freed afterwards. */ - zt = zlog_target_clone(MTYPE_LOG_5424, &zcf->active->zt, + zt = zlog_target_clone(MTYPE_LOG_5424, + zcf->active ? &zcf->active->zt : NULL, sizeof(*zlt)); zlt = container_of(zt, struct zlt_5424, zt); diff --git a/lib/zlog_targets.c b/lib/zlog_targets.c index bbd228f28c7b..db4d1933a0bd 100644 --- a/lib/zlog_targets.c +++ b/lib/zlog_targets.c @@ -208,7 +208,8 @@ static bool zlog_file_cycle(struct zlog_cfg_file *zcf) break; } - zt = zlog_target_clone(MTYPE_LOG_FD, &zcf->active->zt, + zt = zlog_target_clone(MTYPE_LOG_FD, + zcf->active ? &zcf->active->zt : NULL, sizeof(*zlt)); zlt = container_of(zt, struct zlt_fd, zt); @@ -551,7 +552,7 @@ void zlog_syslog_set_prio_min(int prio_min) if (syslog_prio_min != ZLOG_DISABLED) { newztc = zlog_target_clone(MTYPE_LOG_SYSL, - &zlt_syslog->zt, + zlt_syslog ? &zlt_syslog->zt : NULL, sizeof(*newzt)); newzt = container_of(newztc, struct zlt_syslog, zt); newzt->zt.prio_min = prio_min; @@ -561,7 +562,7 @@ void zlog_syslog_set_prio_min(int prio_min) } zlog_target_free(MTYPE_LOG_SYSL, - zlog_target_replace(&zlt_syslog->zt, + zlog_target_replace(zlt_syslog ? &zlt_syslog->zt : NULL, &newzt->zt)); zlt_syslog = newzt; From e7184da6e1d77aba6ccc0c46ddf0faf6bb3cd507 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 May 2024 21:04:30 -0400 Subject: [PATCH 05/12] lib: Fix Null pointer derenference that happened to not crash lib/yang.c:248:3: runtime error: member access within null pointer of type 'struct lysc_node_action' lib/yang.c:254:3: runtime error: member access within null pointer of type 'struct lysc_node_notif' If this data structure happens to ever be moved around we'll start crashing. Let's just fix it. Signed-off-by: Donald Sharp --- lib/yang.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/yang.c b/lib/yang.c index 44459df4a50a..84da9c0cb1b6 100644 --- a/lib/yang.c +++ b/lib/yang.c @@ -245,17 +245,25 @@ int yang_snodes_iterate(const struct lys_module *module, yang_iterate_cb cb, if (ret == YANG_ITER_STOP) return ret; } - LY_LIST_FOR (&module_iter->compiled->rpcs->node, snode) { - ret = yang_snodes_iterate_subtree(snode, module, cb, - flags, arg); - if (ret == YANG_ITER_STOP) - return ret; + if (module_iter->compiled->rpcs) { + LY_LIST_FOR (&module_iter->compiled->rpcs->node, snode) { + ret = yang_snodes_iterate_subtree(snode, module, + cb, flags, + arg); + if (ret == YANG_ITER_STOP) + return ret; + } } - LY_LIST_FOR (&module_iter->compiled->notifs->node, snode) { - ret = yang_snodes_iterate_subtree(snode, module, cb, - flags, arg); - if (ret == YANG_ITER_STOP) - return ret; + + if (module_iter->compiled->notifs) { + LY_LIST_FOR (&module_iter->compiled->notifs->node, + snode) { + ret = yang_snodes_iterate_subtree(snode, module, + cb, flags, + arg); + if (ret == YANG_ITER_STOP) + return ret; + } } } From 9cc1f270884e802cc8049a86fe812813ffa464bc Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 May 2024 21:05:40 -0400 Subject: [PATCH 06/12] tests: Fix not long enough time out Running the mgmt_oper tests under heavy load would occassionally cause the test to fail. The retry mechanism would kick in and the test would have succeeded. Let's extend the timer. Signed-off-by: Donald Sharp --- tests/topotests/mgmt_oper/oper.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/topotests/mgmt_oper/oper.py b/tests/topotests/mgmt_oper/oper.py index 934093aeee0e..5f746e7a0a03 100644 --- a/tests/topotests/mgmt_oper/oper.py +++ b/tests/topotests/mgmt_oper/oper.py @@ -135,7 +135,7 @@ def get_ip_networks(super_prefix, count): return tuple(network.subnets(count_log2))[0:count] -@retry(retry_timeout=30, initial_wait=0.1) +@retry(retry_timeout=60, initial_wait=0.1) def check_kernel(r1, super_prefix, count, add, is_blackhole, vrf, matchvia): network = ipaddress.ip_network(super_prefix) vrfstr = f" vrf {vrf}" if vrf else "" @@ -167,7 +167,7 @@ def addrgen(a, count, step=1): a += step -@retry(retry_timeout=30, initial_wait=0.1) +@retry(retry_timeout=60, initial_wait=0.1) def check_kernel_32(r1, start_addr, count, vrf, step=1): start = ipaddress.ip_address(start_addr) vrfstr = f" vrf {vrf}" if vrf else "" From 019f1bc10620e5605ff77b77f296ae5b245f54ca Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 23 May 2024 06:55:42 -0400 Subject: [PATCH 07/12] lib: Cleanup bitfield to use unsigned values Getting these messages: ./bgp_lu_topo2.test_bgp_lu2/R1/bgpd.err:bgpd/bgp_labelpool.c:310:3: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' ./bgp_lu_topo2.test_bgp_lu2/R4/bgpd.err:bgpd/bgp_labelpool.c:310:3: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' ./bgp_lu_topo2.test_bgp_lu2/R4/bgpd.err:bgpd/bgp_labelpool.c:497:5: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' ./bgp_lu_topo2.test_bgp_lu2/R4/bgpd.err:bgpd/bgp_labelpool.c:499:5: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' Signed-off-by: Donald Sharp --- lib/bitfield.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/bitfield.h b/lib/bitfield.h index 3fda627b7440..f8be74cfb8e1 100644 --- a/lib/bitfield.h +++ b/lib/bitfield.h @@ -83,11 +83,11 @@ DECLARE_MTYPE(BITFIELD); * return an id to bitfield v */ #define bf_release_index(v, id) \ - (v).data[bf_index(id)] &= ~(1 << (bf_offset(id))) + (v).data[bf_index(id)] &= ~(1U << (bf_offset(id))) /* check if an id is in use */ #define bf_test_index(v, id) \ - ((v).data[bf_index(id)] & (1 << (bf_offset(id)))) + ((v).data[bf_index(id)] & (1U << (bf_offset(id)))) /* check if the bit field has been setup */ #define bf_is_inited(v) ((v).data) @@ -110,7 +110,7 @@ DECLARE_MTYPE(BITFIELD); #define bf_set_bit(v, b) \ do { \ size_t w = bf_index(b); \ - (v).data[w] |= 1 << (bf_offset(b)); \ + (v).data[w] |= 1U << (bf_offset(b)); \ (v).n += ((v).data[w] == WORD_MAX); \ if ((v).n == (v).m) { \ (v).m = (v).m + 1; \ From 54621f6d372261760ee86028e5b00656e777ec17 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 23 May 2024 06:56:52 -0400 Subject: [PATCH 08/12] lib: Cleanup bitfield issues found Found this problem: ./isis_te_topo1.test_isis_te_topo1/r1/isisd.err:lib/link_state.c:330:8: runtime error: left shift of 1073741824 by 1 places cannot be represented in type 'int' Clean it up. Signed-off-by: Donald Sharp --- lib/link_state.h | 64 ++++++++++++++++++++++++------------------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/lib/link_state.h b/lib/link_state.h index c54e2ec6d904..a51b502b043f 100644 --- a/lib/link_state.h +++ b/lib/link_state.h @@ -140,38 +140,38 @@ struct ls_node { }; /* Link State flags to indicate which Attribute parameters are valid */ -#define LS_ATTR_UNSET 0x00000000 -#define LS_ATTR_NAME 0x00000001 -#define LS_ATTR_METRIC 0x00000002 -#define LS_ATTR_TE_METRIC 0x00000004 -#define LS_ATTR_ADM_GRP 0x00000008 -#define LS_ATTR_LOCAL_ADDR 0x00000010 -#define LS_ATTR_NEIGH_ADDR 0x00000020 -#define LS_ATTR_LOCAL_ADDR6 0x00000040 -#define LS_ATTR_NEIGH_ADDR6 0x00000080 -#define LS_ATTR_LOCAL_ID 0x00000100 -#define LS_ATTR_NEIGH_ID 0x00000200 -#define LS_ATTR_MAX_BW 0x00000400 -#define LS_ATTR_MAX_RSV_BW 0x00000800 -#define LS_ATTR_UNRSV_BW 0x00001000 -#define LS_ATTR_REMOTE_AS 0x00002000 -#define LS_ATTR_REMOTE_ADDR 0x00004000 -#define LS_ATTR_REMOTE_ADDR6 0x00008000 -#define LS_ATTR_DELAY 0x00010000 -#define LS_ATTR_MIN_MAX_DELAY 0x00020000 -#define LS_ATTR_JITTER 0x00040000 -#define LS_ATTR_PACKET_LOSS 0x00080000 -#define LS_ATTR_AVA_BW 0x00100000 -#define LS_ATTR_RSV_BW 0x00200000 -#define LS_ATTR_USE_BW 0x00400000 -#define LS_ATTR_ADJ_SID 0x01000000 -#define LS_ATTR_BCK_ADJ_SID 0x02000000 -#define LS_ATTR_ADJ_SID6 0x04000000 -#define LS_ATTR_BCK_ADJ_SID6 0x08000000 -#define LS_ATTR_SRLG 0x10000000 -#define LS_ATTR_EXT_ADM_GRP 0x20000000 -#define LS_ATTR_ADJ_SRV6SID 0x40000000 -#define LS_ATTR_BCK_ADJ_SRV6SID 0x80000000 +#define LS_ATTR_UNSET 0x00000000U +#define LS_ATTR_NAME 0x00000001U +#define LS_ATTR_METRIC 0x00000002U +#define LS_ATTR_TE_METRIC 0x00000004U +#define LS_ATTR_ADM_GRP 0x00000008U +#define LS_ATTR_LOCAL_ADDR 0x00000010U +#define LS_ATTR_NEIGH_ADDR 0x00000020U +#define LS_ATTR_LOCAL_ADDR6 0x00000040U +#define LS_ATTR_NEIGH_ADDR6 0x00000080U +#define LS_ATTR_LOCAL_ID 0x00000100U +#define LS_ATTR_NEIGH_ID 0x00000200U +#define LS_ATTR_MAX_BW 0x00000400U +#define LS_ATTR_MAX_RSV_BW 0x00000800U +#define LS_ATTR_UNRSV_BW 0x00001000U +#define LS_ATTR_REMOTE_AS 0x00002000U +#define LS_ATTR_REMOTE_ADDR 0x00004000U +#define LS_ATTR_REMOTE_ADDR6 0x00008000U +#define LS_ATTR_DELAY 0x00010000U +#define LS_ATTR_MIN_MAX_DELAY 0x00020000U +#define LS_ATTR_JITTER 0x00040000U +#define LS_ATTR_PACKET_LOSS 0x00080000U +#define LS_ATTR_AVA_BW 0x00100000U +#define LS_ATTR_RSV_BW 0x00200000U +#define LS_ATTR_USE_BW 0x00400000U +#define LS_ATTR_ADJ_SID 0x01000000U +#define LS_ATTR_BCK_ADJ_SID 0x02000000U +#define LS_ATTR_ADJ_SID6 0x04000000U +#define LS_ATTR_BCK_ADJ_SID6 0x08000000U +#define LS_ATTR_SRLG 0x10000000U +#define LS_ATTR_EXT_ADM_GRP 0x20000000U +#define LS_ATTR_ADJ_SRV6SID 0x40000000U +#define LS_ATTR_BCK_ADJ_SRV6SID 0x80000000U /* Link State Attributes */ struct ls_attributes { From b502f3491de504524ad0b28f23201c0550831773 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 23 May 2024 06:59:39 -0400 Subject: [PATCH 09/12] eigrpd: Fix bit shift problem Found this problem: ./eigrp_topo1.test_eigrp_topo1/r3/eigrpd.err:eigrpd/eigrp_packet.c:1113:35: runtime error: left shift of 192 by 24 places cannot be represented in type 'int' Fix it. Signed-off-by: Donald Sharp --- eigrpd/eigrp_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eigrpd/eigrp_packet.c b/eigrpd/eigrp_packet.c index 963d229bc105..78e5d9f2fcc2 100644 --- a/eigrpd/eigrp_packet.c +++ b/eigrpd/eigrp_packet.c @@ -1110,7 +1110,7 @@ struct TLV_IPv4_Internal_type *eigrp_read_ipv4_tlv(struct stream *s) tlv->prefix_length = stream_getc(s); - destination_tmp = stream_getc(s) << 24; + destination_tmp = (uint32_t)stream_getc(s) << 24; if (tlv->prefix_length > 8) destination_tmp |= stream_getc(s) << 16; if (tlv->prefix_length > 16) From 1d383d47527cffdf70733bc8a4d943cdb2bca3e7 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 23 May 2024 07:20:29 -0400 Subject: [PATCH 10/12] zebra: Fix misaligned long long unsigned int Error message: ./multicast_pim_dr_nondr_test.test_pim_dr_nondr_with_ospf_topo2/r5/zebra.err:zebra/rt_netlink.c:1142:15: runtime error: load of misaligned address 0x7bb40000a064 for type 'long long unsigned int', which requires 8 byte alignment Fix it. Signed-off-by: Donald Sharp --- zebra/rt_netlink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index 01b527ea808e..2c67d61569e6 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -1138,8 +1138,10 @@ static int netlink_route_change_read_multicast(struct nlmsghdr *h, *(struct in6_addr *)RTA_DATA(tb[RTA_DST]); } - if (tb[RTA_EXPIRES]) - m->lastused = *(unsigned long long *)RTA_DATA(tb[RTA_EXPIRES]); + if (tb[RTA_EXPIRES]) { + uint32_t temporary = *(uint32_t *)RTA_DATA(tb[RTA_EXPIRES]); + m->lastused = temporary; + } if (tb[RTA_MULTIPATH]) { struct rtnexthop *rtnh = From fadfc7b3e0ef516e47173c9273df90d2e246e5d6 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 23 May 2024 12:15:46 -0400 Subject: [PATCH 11/12] bgpd: Make peer->max_packet_size atomic This value is being set and read at the same time according to the thread sanitizer WARNING: ThreadSanitizer: data race (pid=2914253) Read of size 2 at 0x7ba800011b10 by thread T2: #0 validate_header bgpd/bgp_io.c:601 (bgpd+0x60c5e0) #1 read_ibuf_work bgpd/bgp_io.c:177 (bgpd+0x608ffe) #2 bgp_process_reads bgpd/bgp_io.c:261 (bgpd+0x609880) #3 event_call lib/event.c:2011 (libfrr.so.0+0x59168d) #4 fpt_run lib/frr_pthread.c:369 (libfrr.so.0+0x35154e) #5 frr_pthread_inner lib/frr_pthread.c:178 (libfrr.so.0+0x34fef6) Previous write of size 2 at 0x7ba800011b10 by main thread: #0 bgp_open_option_parse bgpd/bgp_open.c:1469 (bgpd+0xb5006f) #1 bgp_open_receive bgpd/bgp_packet.c:2100 (bgpd+0x6b3f5c) #2 bgp_process_packet bgpd/bgp_packet.c:4019 (bgpd+0x6c9549) #3 event_call lib/event.c:2011 (libfrr.so.0+0x59168d) #4 frr_run lib/libfrr.c:1217 (libfrr.so.0+0x3b04a9) #5 main bgpd/bgp_main.c:548 (bgpd+0x49aa3d) Location is heap block of size 24328 at 0x7ba80000c000 allocated by main thread: #0 calloc ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:667 (libtsan.so.2+0x3fdd2) #1 qcalloc lib/memory.c:105 (libfrr.so.0+0x3f2784) #2 peer_new bgpd/bgpd.c:1517 (bgpd+0x955024) #3 peer_create bgpd/bgpd.c:1941 (bgpd+0x95c908) #4 peer_remote_as bgpd/bgpd.c:2211 (bgpd+0x9614a6) #5 peer_remote_as_vty bgpd/bgp_vty.c:4788 (bgpd+0x881239) #6 neighbor_remote_as bgpd/bgp_vty.c:4869 (bgpd+0x881a28) #7 cmd_execute_command_real lib/command.c:1002 (libfrr.so.0+0x2b53a2) #8 cmd_execute_command_strict lib/command.c:1111 (libfrr.so.0+0x2b5e0b) #9 command_config_read_one_line lib/command.c:1271 (libfrr.so.0+0x2b6972) #10 config_from_file lib/command.c:1324 (libfrr.so.0+0x2b7035) #11 vty_read_file lib/vty.c:2607 (libfrr.so.0+0x5c0d19) #12 vty_read_config lib/vty.c:2853 (libfrr.so.0+0x5c1f37) #13 frr_config_read_in lib/libfrr.c:981 (libfrr.so.0+0x3ae76a) #14 event_call lib/event.c:2011 (libfrr.so.0+0x59168d) #15 frr_run lib/libfrr.c:1217 (libfrr.so.0+0x3b04a9) #16 main bgpd/bgp_main.c:548 (bgpd+0x49aa3d) Thread T2 'bgpd_io' (tid=2914257, running) created by main thread at: #0 pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001 (libtsan.so.2+0x63a59) #1 frr_pthread_run lib/frr_pthread.c:197 (libfrr.so.0+0x3500da) #2 bgp_pthreads_run bgpd/bgpd.c:8490 (bgpd+0x9d7716) #3 main bgpd/bgp_main.c:547 (bgpd+0x49a9c8) Fix this. Signed-off-by: Donald Sharp --- bgpd/bgp_attr.c | 5 ++++- bgpd/bgp_fsm.c | 5 ++++- bgpd/bgp_io.c | 8 ++++++-- bgpd/bgp_open.c | 10 +++++----- bgpd/bgp_packet.c | 10 +++++----- bgpd/bgp_updgrp.c | 18 ++++++++++++------ bgpd/bgp_updgrp_packet.c | 6 +++--- bgpd/bgpd.c | 5 +++-- bgpd/bgpd.h | 2 +- 9 files changed, 43 insertions(+), 26 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index ac5d08b6fe6e..6eb416f80d54 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -3627,7 +3627,10 @@ enum bgp_attr_parse_ret bgp_attr_parse(struct peer *peer, struct attr *attr, * a stack buffer, since they perform bounds checking * and we are working with untrusted data. */ - unsigned char ndata[peer->max_packet_size]; + uint32_t max_packet_size = + atomic_load_explicit(&peer->max_packet_size, + memory_order_relaxed); + unsigned char ndata[max_packet_size]; memset(ndata, 0x00, sizeof(ndata)); size_t lfl = diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 98ebf5138541..3010c280ffa8 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -211,7 +211,10 @@ static struct peer *peer_xfer_conn(struct peer *from_peer) from_peer->last_major_event = last_maj_evt; peer->remote_id = from_peer->remote_id; peer->last_reset = from_peer->last_reset; - peer->max_packet_size = from_peer->max_packet_size; + atomic_store_explicit(&peer->max_packet_size, + atomic_load_explicit(&from_peer->max_packet_size, + memory_order_relaxed), + memory_order_relaxed); BGP_GR_ROUTER_DETECT_AND_SEND_CAPABILITY_TO_ZEBRA(peer->bgp, peer->bgp->peer); diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c index b07e69ac31bb..a6be738e24b6 100644 --- a/bgpd/bgp_io.c +++ b/bgpd/bgp_io.c @@ -183,7 +183,8 @@ static int read_ibuf_work(struct peer_connection *connection) pktsize = ntohs(pktsize); /* if this fails we are seriously screwed */ - assert(pktsize <= connection->peer->max_packet_size); + assert(pktsize <= atomic_load_explicit(&connection->peer->max_packet_size, + memory_order_relaxed)); /* * If we have that much data, chuck it into its own @@ -561,6 +562,7 @@ static bool validate_header(struct peer_connection *connection) uint16_t size; uint8_t type; struct ringbuf *pkt = connection->ibuf_work; + uint32_t max_packet_size; static const uint8_t m_correct[BGP_MARKER_SIZE] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, @@ -598,7 +600,9 @@ static bool validate_header(struct peer_connection *connection) } /* Minimum packet length check. */ - if ((size < BGP_HEADER_SIZE) || (size > peer->max_packet_size) + max_packet_size = atomic_load_explicit(&peer->max_packet_size, + memory_order_relaxed); + if ((size < BGP_HEADER_SIZE) || (size > max_packet_size) || (type == BGP_MSG_OPEN && size < BGP_MSG_OPEN_MIN_SIZE) || (type == BGP_MSG_UPDATE && size < BGP_MSG_UPDATE_MIN_SIZE) || (type == BGP_MSG_NOTIFY && size < BGP_MSG_NOTIFY_MIN_SIZE) diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index 945076709c5c..3fed45496914 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -1461,11 +1461,11 @@ int bgp_open_option_parse(struct peer *peer, uint16_t length, } /* Extended Message Support */ - peer->max_packet_size = - (CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_RCV) - && CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_ADV)) - ? BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE - : BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE; + atomic_store_explicit(&peer->max_packet_size, + (CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_RCV) + && CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_ADV)) + ? BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE + : BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE, memory_order_relaxed); /* Check that roles are corresponding to each other */ if (bgp_role_violation(peer)) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 842fd1734ad3..ee63c0005763 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -175,7 +175,7 @@ static struct stream *bgp_update_packet_eor(struct peer *peer, afi_t afi, zlog_debug("send End-of-RIB for %s to %s", get_afi_safi_str(afi, safi, false), peer->host); - s = stream_new(peer->max_packet_size); + s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed)); /* Make BGP update packet. */ bgp_packet_set_marker(s, BGP_MSG_UPDATE); @@ -922,7 +922,7 @@ static void bgp_notify_send_internal(struct peer_connection *connection, /* ============================================== */ /* Allocate new stream. */ - s = stream_new(peer->max_packet_size); + s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed)); /* Make notify packet. */ bgp_packet_set_marker(s, BGP_MSG_NOTIFY); @@ -963,7 +963,7 @@ static void bgp_notify_send_internal(struct peer_connection *connection, */ if (use_curr && peer->curr) { size_t packetsize = stream_get_endp(peer->curr); - assert(packetsize <= peer->max_packet_size); + assert(packetsize <= atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed)); if (peer->last_reset_cause) stream_free(peer->last_reset_cause); peer->last_reset_cause = stream_dup(peer->curr); @@ -1113,7 +1113,7 @@ void bgp_route_refresh_send(struct peer *peer, afi_t afi, safi_t safi, /* Convert AFI, SAFI to values for packet. */ bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi); - s = stream_new(peer->max_packet_size); + s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed)); /* Make BGP update packet. */ if (CHECK_FLAG(peer->cap, PEER_CAP_REFRESH_RCV)) @@ -1233,7 +1233,7 @@ void bgp_capability_send(struct peer *peer, afi_t afi, safi_t safi, /* Convert AFI, SAFI to values for packet. */ bgp_map_afi_safi_int2iana(afi, safi, &pkt_afi, &pkt_safi); - s = stream_new(peer->max_packet_size); + s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed)); /* Make BGP update packet. */ bgp_packet_set_marker(s, BGP_MSG_CAPABILITY); diff --git a/bgpd/bgp_updgrp.c b/bgpd/bgp_updgrp.c index b717793a4568..479b1db392eb 100644 --- a/bgpd/bgp_updgrp.c +++ b/bgpd/bgp_updgrp.c @@ -69,6 +69,8 @@ static void sync_init(struct update_subgroup *subgrp, struct update_group *updgrp) { struct peer *peer = UPDGRP_PEER(updgrp); + uint16_t max_packet_size = atomic_load_explicit(&peer->max_packet_size, + memory_order_relaxed); subgrp->sync = XCALLOC(MTYPE_BGP_SYNCHRONISE, sizeof(struct bgp_synchronize)); @@ -93,9 +95,9 @@ static void sync_init(struct update_subgroup *subgrp, * bounds * checking for every single attribute as we construct an UPDATE. */ - subgrp->work = stream_new(peer->max_packet_size + subgrp->work = stream_new(max_packet_size + BGP_MAX_PACKET_SIZE_OVERFLOW); - subgrp->scratch = stream_new(peer->max_packet_size); + subgrp->scratch = stream_new(max_packet_size); } static void sync_delete(struct update_subgroup *subgrp) @@ -134,7 +136,9 @@ static void conf_copy(struct peer *dst, struct peer *src, afi_t afi, dst->flags = src->flags; dst->af_flags[afi][safi] = src->af_flags[afi][safi]; dst->pmax_out[afi][safi] = src->pmax_out[afi][safi]; - dst->max_packet_size = src->max_packet_size; + atomic_store_explicit(&dst->max_packet_size, + atomic_load_explicit(&src->max_packet_size, memory_order_relaxed), + memory_order_relaxed); XFREE(MTYPE_BGP_PEER_HOST, dst->host); dst->host = XSTRDUP(MTYPE_BGP_PEER_HOST, src->host); @@ -361,7 +365,8 @@ static unsigned int updgrp_hash_key_make(const void *p) key); key = jhash_1word(peer->v_routeadv, key); key = jhash_1word(peer->change_local_as, key); - key = jhash_1word(peer->max_packet_size, key); + key = jhash_1word(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed), + key); key = jhash_1word(peer->pmax_out[afi][safi], key); @@ -478,7 +483,7 @@ static unsigned int updgrp_hash_key_make(const void *p) peer->addpath_paths_limit[afi][safi].receive); zlog_debug( "%pBP Update Group Hash: max packet size: %u pmax_out: %u Peer Group: %s rmap out: %s", - peer, peer->max_packet_size, peer->pmax_out[afi][safi], + peer, atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed), peer->pmax_out[afi][safi], peer->group ? peer->group->name : "(NONE)", ROUTE_MAP_OUT_NAME(filter) ? ROUTE_MAP_OUT_NAME(filter) : "(NONE)"); @@ -929,7 +934,8 @@ static int update_group_show_walkcb(struct update_group *updgrp, void *arg) : ""); if (peer) vty_out(vty, " Max packet size: %d\n", - peer->max_packet_size); + atomic_load_explicit(&peer->max_packet_size, + memory_order_relaxed)); } if (subgrp->peer_count > 0) { if (ctx->uj) { diff --git a/bgpd/bgp_updgrp_packet.c b/bgpd/bgp_updgrp_packet.c index 6e30d4f8464a..8815ce64fa69 100644 --- a/bgpd/bgp_updgrp_packet.c +++ b/bgpd/bgp_updgrp_packet.c @@ -902,7 +902,7 @@ struct bpacket *subgroup_update_packet(struct update_subgroup *subgrp) subgrp->update_group->id, subgrp->id, (stream_get_endp(packet) - stream_get_getp(packet)), - peer->max_packet_size, num_pfx); + atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed), num_pfx); pkt = bpacket_queue_add(SUBGRP_PKTQ(subgrp), packet, &vecarr); stream_reset(s); stream_reset(snlri); @@ -1139,7 +1139,7 @@ void subgroup_default_update_packet(struct update_subgroup *subgrp, tx_id_buf, attrstr); } - s = stream_new(peer->max_packet_size); + s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed)); /* Make BGP update packet. */ bgp_packet_set_marker(s, BGP_MSG_UPDATE); @@ -1226,7 +1226,7 @@ void subgroup_default_withdraw_packet(struct update_subgroup *subgrp) tx_id_buf); } - s = stream_new(peer->max_packet_size); + s = stream_new(atomic_load_explicit(&peer->max_packet_size, memory_order_relaxed)); /* Make BGP update packet. */ bgp_packet_set_marker(s, BGP_MSG_UPDATE); diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 44cd24565a96..87d55a0f5d7c 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1551,7 +1551,8 @@ struct peer *peer_new(struct bgp *bgp) peer->local_role = ROLE_UNDEFINED; peer->remote_role = ROLE_UNDEFINED; peer->password = NULL; - peer->max_packet_size = BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE; + atomic_store_explicit(&peer->max_packet_size, BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE, + memory_order_relaxed); /* Set default flags. */ FOREACH_AFI_SAFI (afi, safi) { @@ -1646,7 +1647,7 @@ void peer_xfer_config(struct peer *peer_dst, struct peer *peer_src) peer_dst->rmap_type = peer_src->rmap_type; peer_dst->local_role = peer_src->local_role; - peer_dst->max_packet_size = peer_src->max_packet_size; + atomic_store_explicit(&peer_dst->max_packet_size, atomic_load_explicit(&peer_src->max_packet_size, memory_order_relaxed), memory_order_relaxed); /* Timers */ peer_dst->holdtime = peer_src->holdtime; diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index c92293a84412..b9a6f09095fb 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -1868,7 +1868,7 @@ struct peer { char *domainname; /* Extended Message Support */ - uint16_t max_packet_size; + _Atomic uint16_t max_packet_size; /* Conditional advertisement */ bool advmap_config_change[AFI_MAX][SAFI_MAX]; From 946c20ac627fcc5d979149c9b6566001e63aa84b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 23 May 2024 12:17:09 -0400 Subject: [PATCH 12/12] lib: Thread is being used and set at the same time The call to set thread is being used and set at the same time in various pthreads in the code. This should not be happening. Let's fix it. Signed-off-by: Donald Sharp --- lib/event.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/event.c b/lib/event.c index f4aa7c58b9d2..1fec13e80dfc 100644 --- a/lib/event.c +++ b/lib/event.c @@ -1515,13 +1515,7 @@ void event_cancel_async(struct event_loop *master, struct event **thread, { assert(!(thread && eventobj) && (thread || eventobj)); - if (thread && *thread) - frrtrace(9, frr_libfrr, event_cancel_async, master, - (*thread)->xref->funcname, (*thread)->xref->xref.file, - (*thread)->xref->xref.line, NULL, (*thread)->u.fd, - (*thread)->u.val, (*thread)->arg, - (*thread)->u.sands.tv_sec); - else + if (!thread) frrtrace(9, frr_libfrr, event_cancel_async, master, NULL, NULL, 0, NULL, 0, 0, eventobj, 0); @@ -1531,6 +1525,14 @@ void event_cancel_async(struct event_loop *master, struct event **thread, master->canceled = false; if (thread) { + if (*thread) + frrtrace(9, frr_libfrr, event_cancel_async, + master, (*thread)->xref->funcname, + (*thread)->xref->xref.file, + (*thread)->xref->xref.line, NULL, + (*thread)->u.fd, (*thread)->u.val, + (*thread)->arg, + (*thread)->u.sands.tv_sec); struct cancel_req *cr = XCALLOC(MTYPE_TMP, sizeof(struct cancel_req)); cr->threadref = thread;