From bdf0c8d0009e5cb2679a91d1a23472b0711ed8da Mon Sep 17 00:00:00 2001 From: claudio Date: Wed, 19 Sep 2018 13:09:30 +0000 Subject: [PATCH] Wrap sending imsg to the RDE in a function and make sure that the ibuf to the RDE is valid. The SE is stopping all sessions on exit and so session_stop() is called which will send an imsg to the RDE which is no longer there. Instead of fixing just one call fix all. Now the SE should no longer crash when the RDE crashes. OK sthen@ --- usr.sbin/bgpd/session.c | 78 ++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c index 659d52157ac..65bee17c1d9 100644 --- a/usr.sbin/bgpd/session.c +++ b/usr.sbin/bgpd/session.c @@ -1,4 +1,4 @@ -/* $OpenBSD: session.c,v 1.366 2018/09/04 12:00:29 claudio Exp $ */ +/* $OpenBSD: session.c,v 1.367 2018/09/19 13:09:30 claudio Exp $ */ /* * Copyright (c) 2003, 2004, 2005 Henning Brauer @@ -94,6 +94,7 @@ int capa_neg_calc(struct peer *); void session_dispatch_imsg(struct imsgbuf *, int, u_int *); void session_up(struct peer *); void session_down(struct peer *); +int imsg_rde(int, u_int32_t, void *, u_int16_t); void session_demote(struct peer *, int); int session_link_state_is_up(int, int, int); @@ -1379,8 +1380,7 @@ session_sendmsg(struct bgp_msg *msg, struct peer *p) ibuf_close(&p->wbuf, msg->buf); if (!p->throttled && p->wbuf.queued > SESS_MSG_HIGH_MARK) { - if (imsg_compose(ibuf_rde, IMSG_XOFF, p->conf.id, 0, -1, - NULL, 0) == -1) + if (imsg_rde(IMSG_XOFF, p->conf.id, NULL, 0) == -1) log_peer_warn(&p->conf, "imsg_compose XOFF"); p->throttled = 1; } @@ -1671,16 +1671,16 @@ session_graceful_restart(struct peer *p) for (i = 0; i < AID_MAX; i++) { if (p->capa.neg.grestart.flags[i] & CAPA_GR_PRESENT) { - if (imsg_compose(ibuf_rde, IMSG_SESSION_STALE, - p->conf.id, 0, -1, &i, sizeof(i)) == -1) + if (imsg_rde(IMSG_SESSION_STALE, p->conf.id, + &i, sizeof(i)) == -1) return (-1); log_peer_warnx(&p->conf, "graceful restart of %s, keeping routes", aid2str(i)); p->capa.neg.grestart.flags[i] |= CAPA_GR_RESTARTING; } else if (p->capa.neg.mp[i]) { - if (imsg_compose(ibuf_rde, IMSG_SESSION_FLUSH, - p->conf.id, 0, -1, &i, sizeof(i)) == -1) + if (imsg_rde(IMSG_SESSION_FLUSH, p->conf.id, + &i, sizeof(i)) == -1) return (-1); log_peer_warnx(&p->conf, "graceful restart of %s, flushing routes", @@ -1704,8 +1704,8 @@ session_graceful_stop(struct peer *p) if (p->capa.neg.grestart.flags[i] & CAPA_GR_RESTARTING) { log_peer_warnx(&p->conf, "graceful restart of %s, " "time-out, flushing", aid2str(i)); - if (imsg_compose(ibuf_rde, IMSG_SESSION_FLUSH, - p->conf.id, 0, -1, &i, sizeof(i)) == -1) + if (imsg_rde(IMSG_SESSION_FLUSH, p->conf.id, + &i, sizeof(i)) == -1) return (-1); } p->capa.neg.grestart.flags[i] &= ~CAPA_GR_RESTARTING; @@ -1771,8 +1771,7 @@ session_dispatch_msg(struct pollfd *pfd, struct peer *p) return (1); } if (p->throttled && p->wbuf.queued < SESS_MSG_LOW_MARK) { - if (imsg_compose(ibuf_rde, IMSG_XON, p->conf.id, 0, -1, - NULL, 0) == -1) + if (imsg_rde(IMSG_XON, p->conf.id, NULL, 0) == -1) log_peer_warn(&p->conf, "imsg_compose XON"); p->throttled = 0; } @@ -2183,8 +2182,7 @@ parse_update(struct peer *peer) p += MSGSIZE_HEADER; /* header is already checked */ datalen -= MSGSIZE_HEADER; - if (imsg_compose(ibuf_rde, IMSG_UPDATE, peer->conf.id, 0, -1, p, - datalen) == -1) + if (imsg_rde(IMSG_UPDATE, peer->conf.id, p, datalen) == -1) return (-1); return (0); @@ -2221,8 +2219,7 @@ parse_refresh(struct peer *peer) return (0); } - if (imsg_compose(ibuf_rde, IMSG_REFRESH, peer->conf.id, 0, -1, &aid, - sizeof(aid)) == -1) + if (imsg_rde(IMSG_REFRESH, peer->conf.id, &aid, sizeof(aid)) == -1) return (-1); return (0); @@ -2551,8 +2548,8 @@ capa_neg_calc(struct peer *p) if (negflags & CAPA_GR_RESTARTING) { if (!(p->capa.peer.grestart.flags[i] & CAPA_GR_FORWARD)) { - if (imsg_compose(ibuf_rde, IMSG_SESSION_FLUSH, - p->conf.id, 0, -1, &i, sizeof(i)) == -1) + if (imsg_rde(IMSG_SESSION_FLUSH, p->conf.id, + &i, sizeof(i)) == -1) return (-1); log_peer_warnx(&p->conf, "graceful restart of " "%s, not restarted, flushing", aid2str(i)); @@ -2657,9 +2654,8 @@ session_dispatch_imsg(struct imsgbuf *ibuf, int idx, u_int *listener_cnt) /* sync the RDE in case we keep the peer */ if (reconf == RECONF_KEEP) { - if (imsg_compose(ibuf_rde, IMSG_SESSION_ADD, - p->conf.id, 0, -1, &p->conf, - sizeof(struct peer_config)) == -1) + if (imsg_rde(IMSG_SESSION_ADD, p->conf.id, + &p->conf, sizeof(struct peer_config)) == -1) fatalx("imsg_compose error"); if (p->conf.template) { /* apply the conf to all clones */ @@ -2670,10 +2666,8 @@ session_dispatch_imsg(struct imsgbuf *ibuf, int idx, u_int *listener_cnt) session_template_clone(np, NULL, np->conf.id, np->conf.remote_as); - if (imsg_compose(ibuf_rde, - IMSG_SESSION_ADD, - np->conf.id, 0, -1, - &np->conf, + if (imsg_rde(IMSG_SESSION_ADD, + np->conf.id, &np->conf, sizeof(struct peer_config)) == -1) fatalx("imsg_compose error"); @@ -2965,9 +2959,8 @@ session_dispatch_imsg(struct imsgbuf *ibuf, int idx, u_int *listener_cnt) timer_stop(p, Timer_RestartTimeout); /* signal back to RDE to cleanup stale routes */ - if (imsg_compose(ibuf_rde, - IMSG_SESSION_RESTARTED, imsg.hdr.peerid, 0, - -1, &aid, sizeof(aid)) == -1) + if (imsg_rde(IMSG_SESSION_RESTARTED, + imsg.hdr.peerid, &aid, sizeof(aid)) == -1) fatal("imsg_compose: " "IMSG_SESSION_RESTARTED"); } @@ -3188,8 +3181,14 @@ session_down(struct peer *peer) { bzero(&peer->capa.neg, sizeof(peer->capa.neg)); peer->stats.last_updown = time(NULL); - if (imsg_compose(ibuf_rde, IMSG_SESSION_DOWN, peer->conf.id, 0, -1, - NULL, 0) == -1) + /* + * session_down is called in the exit code path so check + * if the RDE is still around, if not there is no need to + * send the message. + */ + if (ibuf_rde == NULL) + return; + if (imsg_rde(IMSG_SESSION_DOWN, peer->conf.id, NULL, 0) == -1) fatalx("imsg_compose error"); } @@ -3198,7 +3197,7 @@ session_up(struct peer *p) { struct session_up sup; - if (imsg_compose(ibuf_rde, IMSG_SESSION_ADD, p->conf.id, 0, -1, + if (imsg_rde(IMSG_SESSION_ADD, p->conf.id, &p->conf, sizeof(p->conf)) == -1) fatalx("imsg_compose error"); @@ -3209,8 +3208,7 @@ session_up(struct peer *p) sup.short_as = p->short_as; memcpy(&sup.capa, &p->capa.neg, sizeof(sup.capa)); p->stats.last_updown = time(NULL); - if (imsg_compose(ibuf_rde, IMSG_SESSION_UP, p->conf.id, 0, -1, - &sup, sizeof(sup)) == -1) + if (imsg_rde(IMSG_SESSION_UP, p->conf.id, &sup, sizeof(sup)) == -1) fatalx("imsg_compose error"); } @@ -3224,6 +3222,11 @@ imsg_ctl_parent(int type, u_int32_t peerid, pid_t pid, void *data, int imsg_ctl_rde(int type, pid_t pid, void *data, u_int16_t datalen) { + if (ibuf_rde_ctl == NULL) { + log_warnx("Can't send message %u to RDE, ctl pipe closed", + type); + return (0); + } /* * Use control socket to talk to RDE to bypass the queue of the * regular imsg socket. @@ -3231,6 +3234,17 @@ imsg_ctl_rde(int type, pid_t pid, void *data, u_int16_t datalen) return (imsg_compose(ibuf_rde_ctl, type, 0, pid, -1, data, datalen)); } +int +imsg_rde(int type, uint32_t peerid, void *data, u_int16_t datalen) +{ + if (ibuf_rde == NULL) { + log_warnx("Can't send message %u to RDE, pipe closed", type); + return (0); + } + + return (imsg_compose(ibuf_rde, type, peerid, 0, -1, data, datalen)); +} + void session_demote(struct peer *p, int level) { -- 2.20.1