From: Claudio Jeker Subject: Re: bgpd: make imsg_rde void and fail hard To: Theo Buehler Cc: tech@openbsd.org Date: Wed, 26 Feb 2025 17:52:13 +0100 On Wed, Feb 26, 2025 at 05:44:44PM +0100, Theo Buehler wrote: > On Wed, Feb 26, 2025 at 05:37:42PM +0100, Claudio Jeker wrote: > > On Wed, Feb 26, 2025 at 05:30:38PM +0100, Theo Buehler wrote: > > > On Wed, Feb 26, 2025 at 05:18:58PM +0100, Claudio Jeker wrote: > > > > While working on all the other bits in the session engine I came to the > > > > conclusion that if imsg_rde() fails the game is over. So just admit to > > > > that and fatal in that case. > > > > > > > > This simplifies all the callers since there is no need to check the return > > > > value. This also allows some other functions to be come void. > > > > > > ok > > > > > > Should session_handle_{update,rrefresh}() also become void? Wouldn't > > > it be more consistent if the return (0) was pushed to parse_update() > > > and parse_rrefresh()? > > > > Maybe we should make them all void including parse_update() and > > parse_rrefresh(). The return value of parse_rrefresh is not checked and > > the code around parse_update() is most probably wrong for the error case. > > In the end when a parse error happens we need to generate a notification > > and properly throw an error in the FSM but there is no need to bubble up > > the error. > > Right, that makes sense. That would look like this. -- :wq Claudio Index: session.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.c,v diff -u -p -r1.523 session.c --- session.c 26 Feb 2025 16:39:18 -0000 1.523 +++ session.c 26 Feb 2025 16:50:36 -0000 @@ -1016,19 +1016,17 @@ get_alternate_addr(struct bgpd_addr *loc freeifaddrs(ifap); } -int +void session_handle_update(struct peer *peer, struct ibuf *msg) { /* pass the message verbatim to the rde. */ imsg_rde(IMSG_UPDATE, peer->conf.id, ibuf_data(msg), ibuf_size(msg)); - return (0); } -int +void session_handle_rrefresh(struct peer *peer, struct route_refresh *rr) { imsg_rde(IMSG_REFRESH, peer->conf.id, rr, sizeof(*rr)); - return (0); } void Index: session.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.h,v diff -u -p -r1.190 session.h --- session.h 26 Feb 2025 16:39:18 -0000 1.190 +++ session.h 26 Feb 2025 16:50:36 -0000 @@ -335,8 +335,8 @@ void get_alternate_addr(struct bgpd_ad struct peer *getpeerbydesc(struct bgpd_config *, const char *); struct peer *getpeerbyip(struct bgpd_config *, struct sockaddr *); struct peer *getpeerbyid(struct bgpd_config *, uint32_t); -int session_handle_update(struct peer *, struct ibuf *); -int session_handle_rrefresh(struct peer *, struct route_refresh *); +void session_handle_update(struct peer *, struct ibuf *); +void session_handle_rrefresh(struct peer *, struct route_refresh *); void session_graceful_restart(struct peer *); void session_graceful_flush(struct peer *, uint8_t, const char *); void session_mrt_dump_state(struct peer *, enum session_state, Index: session_bgp.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v diff -u -p -r1.2 session_bgp.c --- session_bgp.c 26 Feb 2025 16:39:18 -0000 1.2 +++ session_bgp.c 26 Feb 2025 16:50:36 -0000 @@ -1079,13 +1079,13 @@ parse_open(struct peer *peer, struct ibu return (0); } -static int +static void parse_update(struct peer *peer, struct ibuf *msg) { - return session_handle_update(peer, msg); + session_handle_update(peer, msg); } -static int +static void parse_rrefresh(struct peer *peer, struct ibuf *msg) { struct route_refresh rr; @@ -1114,7 +1114,7 @@ parse_rrefresh(struct peer *peer, struct session_notification_data(peer, ERR_HEADER, ERR_HDR_LEN, &datalen, sizeof(datalen)); bgp_fsm(peer, EVNT_CON_FATAL, NULL); - return (-1); + return; } peer->stats.refresh_rcvd_req++; break; @@ -1129,7 +1129,7 @@ parse_rrefresh(struct peer *peer, struct session_notification(peer, ERR_RREFRESH, ERR_RR_INV_LEN, msg); bgp_fsm(peer, EVNT_CON_FATAL, NULL); - return (-1); + return; } if (subtype == ROUTE_REFRESH_BEGIN_RR) peer->stats.refresh_rcvd_borr++; @@ -1139,7 +1139,7 @@ parse_rrefresh(struct peer *peer, struct default: log_peer_warnx(&peer->conf, "peer sent bad refresh, " "bad subtype %d", subtype); - return (0); + return; } } else { /* force subtype to default */ @@ -1151,18 +1151,18 @@ parse_rrefresh(struct peer *peer, struct if (afi2aid(afi, safi, &aid) == -1) { log_peer_warnx(&peer->conf, "peer sent bad refresh, " "invalid afi/safi pair"); - return (0); + return; } if (!peer->capa.neg.refresh && !peer->capa.neg.enhanced_rr) { log_peer_warnx(&peer->conf, "peer sent unexpected refresh"); - return (0); + return; } rr.aid = aid; rr.subtype = subtype; - return session_handle_rrefresh(peer, &rr); + session_handle_rrefresh(peer, &rr); } static void @@ -1757,10 +1757,7 @@ bgp_fsm(struct peer *peer, enum session_ break; case EVNT_RCVD_UPDATE: start_timer_holdtime(peer); - if (parse_update(peer, msg)) - change_state(peer, STATE_IDLE, event); - else - start_timer_holdtime(peer); + parse_update(peer, msg); break; case EVNT_RCVD_NOTIFICATION: parse_notification(peer, msg);