On Mon, Nov 19, 2018 at 09:45:55AM +0100, Claudio Jeker wrote: > On Sun, Nov 18, 2018 at 11:40:40PM +0100, Remi Locherer wrote: > > Hi, > > > > from the discussion I understand nobody rejects the functionality. > > > > To ease the review here again the diff (with incorporated feedback from > > anton@ (redundant parens)). > > > > Any comments or OKs? > > Not sure about this, since control_setup() is called by reconfigure(). > So it is possible that bgpd will terminate while reloading the config. > I would prefer if control_setup() would return an error that is passed > back up to reconfigure() instead of the fatalx() call. In that case bgpd > will quit on startup but return a failure on reload bit keep on running > with the old config. This would also allow to do the same check on the > restricted socket. I also added some minor comments below.
Thank you for the explanation! I didn't pay attention to the different return values of reconfigure(). This helps to extend the "safety belt" also to listen addresses. The new diff adds this to the last one: - on startup: stop if a listen address or control socket is already in use - on reload: handle used listen addresses and control sockets similar to configuration errors. - remove control_cleanup() since it can remove control sockets of other processes even if it detected them. See also https://marc.info/?l=openbsd-tech&m=154092081205195&w=2 for a discussion about not removing sockets. cvs diff: Diffing . Index: bgpd.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v retrieving revision 1.204 diff -u -p -r1.204 bgpd.c --- bgpd.c 29 Sep 2018 08:11:11 -0000 1.204 +++ bgpd.c 19 Nov 2018 14:59:44 -0000 @@ -348,8 +348,6 @@ BROKEN if (pledge("stdio rpath wpath cpa free(p); } - control_cleanup(conf->csock); - control_cleanup(conf->rcsock); carp_demote_shutdown(); kr_shutdown(conf->fib_priority, conf->default_tableid); pftable_clear_all(); @@ -453,10 +451,15 @@ reconfigure(char *conffile, struct bgpd_ reconfpending = 0; return (1); } + + if (prepare_listeners(conf) == -1) { + reconfpending = 0; + return (1); + } + expand_networks(conf); cflags = conf->flags; - prepare_listeners(conf); /* start reconfiguration */ if (imsg_compose(ibuf_se, IMSG_RECONF_CONF, 0, 0, -1, @@ -473,8 +476,10 @@ reconfigure(char *conffile, struct bgpd_ la->fd = -1; } - if (control_setup(conf) == -1) - return (-1); + if (control_setup(conf) == -1) { + reconfpending = 0; + return (1); + } /* adjust fib syncing on reload */ ktable_preload(); @@ -934,11 +939,12 @@ control_setup(struct bgpd_config *conf) /* control socket is outside chroot */ if (!cname || strcmp(cname, conf->csock)) { if (cname) { - control_cleanup(cname); free(cname); } if ((cname = strdup(conf->csock)) == NULL) fatal("strdup"); + if (control_check(cname) == -1) + return (-1); if ((fd = control_init(0, cname)) == -1) fatalx("control socket setup failed"); if (control_listen(fd) == -1) @@ -950,16 +956,16 @@ control_setup(struct bgpd_config *conf) } if (!conf->rcsock) { /* remove restricted socket */ - control_cleanup(rcname); free(rcname); rcname = NULL; } else if (!rcname || strcmp(rcname, conf->rcsock)) { if (rcname) { - control_cleanup(rcname); free(rcname); } if ((rcname = strdup(conf->rcsock)) == NULL) fatal("strdup"); + if (control_check(rcname) == -1) + return (-1); if ((fd = control_init(1, rcname)) == -1) fatalx("control socket setup failed"); if (control_listen(fd) == -1) Index: bgpd.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v retrieving revision 1.354 diff -u -p -r1.354 bgpd.h --- bgpd.h 14 Nov 2018 14:03:36 -0000 1.354 +++ bgpd.h 19 Nov 2018 14:33:01 -0000 @@ -1142,7 +1142,6 @@ void set_pollfd(struct pollfd *, struc int handle_pollfd(struct pollfd *, struct imsgbuf *); /* control.c */ -void control_cleanup(const char *); int control_imsg_relay(struct imsg *); /* config.c */ Index: config.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/config.c,v retrieving revision 1.78 diff -u -p -r1.78 config.c --- config.c 14 Nov 2018 17:24:01 -0000 1.78 +++ config.c 19 Nov 2018 21:49:12 -0000 @@ -384,7 +384,7 @@ host_ip(const char *s, struct bgpd_addr return (1); } -void +int prepare_listeners(struct bgpd_config *conf) { struct listen_addr *la, *next; @@ -459,9 +459,11 @@ prepare_listeners(struct bgpd_config *co close(la->fd); TAILQ_REMOVE(conf->listen_addrs, la, entry); free(la); - continue; + return (-1); } } + + return (0); } int Index: control.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/control.c,v retrieving revision 1.90 diff -u -p -r1.90 control.c --- control.c 11 Aug 2017 16:02:53 -0000 1.90 +++ control.c 19 Nov 2018 15:09:03 -0000 @@ -38,6 +38,32 @@ void control_result(struct ctl_conn *, ssize_t imsg_read_nofd(struct imsgbuf *); int +control_check(char *path) +{ + struct sockaddr_un sun; + int fd; + + bzero(&sun, sizeof(sun)); + sun.sun_family = AF_UNIX; + strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); + + if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { + log_warn("%s: socket", __func__); + return (-1); + } + + if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) { + log_warnx("control socket %s already in use", path); + close(fd); + return (-1); + } + + close(fd); + + return (0); +} + +int control_init(int restricted, char *path) { struct sockaddr_un sun; @@ -108,13 +134,6 @@ void control_shutdown(int fd) { close(fd); -} - -void -control_cleanup(const char *path) -{ - if (path) - unlink(path); } unsigned int Index: session.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.h,v retrieving revision 1.125 diff -u -p -r1.125 session.h --- session.h 24 Oct 2018 08:26:37 -0000 1.125 +++ session.h 19 Nov 2018 12:55:31 -0000 @@ -246,10 +246,11 @@ int carp_demote_set(char *, int); /* config.c */ int merge_config(struct bgpd_config *, struct bgpd_config *, struct peer *); -void prepare_listeners(struct bgpd_config *); +int prepare_listeners(struct bgpd_config *); int get_mpe_label(struct rdomain *); /* control.c */ +int control_check(char *); int control_init(int, char *); int control_listen(int); void control_shutdown(int);
