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@

diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c
index 659d521..65bee17 100644 (file)
--- 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 <henning@openbsd.org>
@@ -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)
 {