Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix up from a bunch of ubsan issues found. #16074

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions bgpd/bgp_addpath.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion bgpd/bgp_attr.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@choppsv1 choppsv1 May 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think this may mess with the data cache and so could be rather expensive as a result. Can we instead simply guarantee that peer's are allocated with the required alignment to guarantee ((uint32_t)&peer->max_packet_size) % 4) == 0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm max_packet_size is actually a uin16_t. This is probably a threading error I guess, and not an alignment one. Can you try changing max_packet_size to int rather than using the atomic function and see if that fixes the error?

unsigned char ndata[max_packet_size];

memset(ndata, 0x00, sizeof(ndata));
size_t lfl =
Expand Down
5 changes: 4 additions & 1 deletion bgpd/bgp_fsm.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 6 additions & 2 deletions bgpd/bgp_io.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions bgpd/bgp_open.c
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
10 changes: 5 additions & 5 deletions bgpd/bgp_packet.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions bgpd/bgp_route.c
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
18 changes: 12 additions & 6 deletions bgpd/bgp_updgrp.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);


Expand Down Expand Up @@ -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)");
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions bgpd/bgp_updgrp_packet.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions bgpd/bgp_vty.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 3 additions & 2 deletions bgpd/bgpd.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion bgpd/bgpd.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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 */
Expand Down
1 change: 1 addition & 0 deletions bgpd/rfapi/rfapi_import.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion eigrpd/eigrp_packet.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions isisd/isis_adjacency.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}
1 change: 1 addition & 0 deletions isisd/isis_pdu_counter.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion isisd/isis_spf.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
1 change: 1 addition & 0 deletions isisd/isisd.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions ldpd/lde_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
6 changes: 3 additions & 3 deletions lib/bitfield.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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; \
Expand Down
1 change: 1 addition & 0 deletions lib/command_graph.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions lib/command_match.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -582,6 +583,7 @@ static int score_precedence(enum cmd_token_type type)
}

assert(!"Reached end of function we should never hit");
return 10;
}

/**
Expand Down Expand Up @@ -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."
Expand Down
Loading
Loading