On Mon, Aug 06, 2018 at 03:50:41PM +0200, Claudio Jeker wrote:
> On Mon, Aug 06, 2018 at 03:34:10PM +0200, Claudio Jeker wrote:
> > On Mon, Aug 06, 2018 at 03:26:16PM +0200, Pierre Emeriaud wrote:
> > > Not sure about this, I guess if you're using 'network bulk add|del'
> > > you're supposed to know what you're doing and not shoot yourself in
> > > the foot, but here it goes:
> > >
> > > echo | bgpctl network bulk del
> > > crashes bgpd. On 6.3 and current as of today at least.
> > >
> > > $ doas bgpd -dvf bgpd.conf
> > > startup
> > > rereading config
> > > route decision engine ready
> > > session engine ready
> > > myas = "64751"
> > > ktable_new: rdomain_0 for rtableid 0
> > > listening on 127.0.0.1
> > > SE reconfigured
> > > RDE reconfigured
> > >
> > > #### bgpd started, time to run 'echo | bgpctl network bulk del'
> > >
> > > fatal in RDE: pt_fill: unknown af
> > > peer closed imsg connection
> > > main: Lost connection to RDE
> > > peer closed imsg connection
> > > SE: Lost connection to parent
> > > session engine exiting
> > > kernel routing table 0 (Loc-RIB) decoupled
> > > ktable_destroy: freeing ktable Loc-RIB rtableid 0
> > > waiting for children to terminate
> > > terminating
> > >
> > >
> > > This somewhat fixes the empty line issue, but it's still possible to
> > > crash with any other string not beeing an ip prefix. A better
> > > workaround would be parse prefixes beforehand, or maybe just don't be
> > > a moron and don't feed bgpctl with anything stupid.
> > >
> > > Index: bgpctl.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> > > retrieving revision 1.210
> > > diff -u -p -r1.210 bgpctl.c
> > > --- bgpctl.c 29 Jul 2018 13:02:01 -0000 1.210
> > > +++ bgpctl.c 6 Aug 2018 13:11:23 -0000
> > > @@ -1978,6 +1978,9 @@ network_bulk(struct parse_result *res)
> > > /* Don't process commented entries */
> > > if (strchr(b, '#') != NULL)
> > > break;
> > > + /* Don't process empty lines */
> > > + if (strcmp(b, "\0") == 0)
> > > + break;
> > > bzero(&net, sizeof(net));
> > > parse_prefix(b, strlen(b), &h, &len);
> > > net.prefix = h;
> > >
> >
> > Thanks for the diff and the report. I will look into this.
> >
>
> Here a diff to protect bgpd a bit better. It uses the same logic as for
> the IMSG_NETWORK_ADD case and does a very basic check of the prefix passed
> in.
>
> Skipping empty lines in bgpctl is also something we should do but that
> code needs some rework.
>
I looked at the way we parse those files and I refactored it a fair
amount. I think this new version is more what we actually want.
People using the bulk loading feature (e.g. for bgpd spamd) please test.
--
:wq Claudio
Index: bgpctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.210
diff -u -p -r1.210 bgpctl.c
--- bgpctl.c 29 Jul 2018 13:02:01 -0000 1.210
+++ bgpctl.c 7 Aug 2018 08:09:13 -0000
@@ -1956,53 +1956,48 @@ network_bulk(struct parse_result *res)
struct network_config net;
struct filter_set *s = NULL;
struct bgpd_addr h;
- char *b, *buf, *lbuf;
- size_t slen;
+ char *line = NULL;
+ size_t linesize = 0;
+ ssize_t linelen;
u_int8_t len;
FILE *f;
- if ((f = fdopen(STDIN_FILENO, "r")) != NULL) {
- while ((buf = fgetln(f, &slen))) {
- lbuf = NULL;
- if (buf[slen - 1] == '\n')
- buf[slen - 1] = '\0';
- else {
- if ((lbuf = malloc(slen + 1)) == NULL)
- err(1, NULL);
- memcpy(lbuf, buf, slen);
- lbuf[slen] = '\0';
- buf = lbuf;
- }
+ if ((f = fdopen(STDIN_FILENO, "r")) == NULL)
+ err(1, "Failed to open stdin\n");
- while ((b = strsep(&buf, " \t")) != NULL) {
- /* Don't process commented entries */
- if (strchr(b, '#') != NULL)
- break;
- bzero(&net, sizeof(net));
- parse_prefix(b, strlen(b), &h, &len);
- net.prefix = h;
- net.prefixlen = len;
+ while ((linelen = getline(&line, &linesize, f)) != -1) {
+ char *b, *buf = line;
+ while ((b = strsep(&buf, " \t\n")) != NULL) {
+ if (*b == '\0') /* skip empty tokens */
+ continue;
+ /* Stop processing after a comment */
+ if (*b == '#')
+ break;
+ bzero(&net, sizeof(net));
+ if (parse_prefix(b, strlen(b), &h, &len) != 1)
+ errx(1, "bad prefix: %s", b);
+ net.prefix = h;
+ net.prefixlen = len;
- if (res->action == NETWORK_BULK_ADD) {
- imsg_compose(ibuf, IMSG_NETWORK_ADD,
- 0, 0, -1, &net, sizeof(net));
- TAILQ_FOREACH(s, &res->set, entry) {
- imsg_compose(ibuf,
- IMSG_FILTER_SET,
- 0, 0, -1, s, sizeof(*s));
- }
- imsg_compose(ibuf, IMSG_NETWORK_DONE,
- 0, 0, -1, NULL, 0);
- } else
- imsg_compose(ibuf, IMSG_NETWORK_REMOVE,
- 0, 0, -1, &net, sizeof(net));
- }
- free(lbuf);
+ if (res->action == NETWORK_BULK_ADD) {
+ imsg_compose(ibuf, IMSG_NETWORK_ADD,
+ 0, 0, -1, &net, sizeof(net));
+ TAILQ_FOREACH(s, &res->set, entry) {
+ imsg_compose(ibuf,
+ IMSG_FILTER_SET,
+ 0, 0, -1, s, sizeof(*s));
+ }
+ imsg_compose(ibuf, IMSG_NETWORK_DONE,
+ 0, 0, -1, NULL, 0);
+ } else
+ imsg_compose(ibuf, IMSG_NETWORK_REMOVE,
+ 0, 0, -1, &net, sizeof(net));
}
- fclose(f);
- } else {
- err(1, "Failed to open stdin\n");
}
+ free(line);
+ if (ferror(f))
+ err(1, "getline");
+ fclose(f);
}
void
