On 2017 Feb 14 (Tue) at 23:26:32 +1100 (+1100), Jonathan Gray wrote:
:On Tue, Feb 14, 2017 at 01:04:40PM +0100, Sebastian Benoit wrote:
:> Jonathan Gray([email protected]) on 2017.02.14 20:23:51 +1100:
:> > On Tue, Feb 14, 2017 at 10:00:17AM +0100, Peter Hessler wrote:
:> > > On 2017 Feb 14 (Tue) at 15:39:45 +1100 (+1100), Jonathan Gray wrote:
:> > > :The bgpctl parser for large communities makes invalid assumptions about
:> > > :the string passed into parse_largecommunity() and also seems to leak
:> > > :the memory returned by strdup in the same function.
:> > > :
:> > > :(gdb) run show rib large-community 1:1
:> > > :Starting program: /usr/obj/usr.sbin/bgpctl/bgpctl show rib
large-community 1:1
:> > > :
:> > > :Program received signal SIGBUS, Bus error.
:> > > :getlargecommunity (
:> > > : s=0x9940329f0b9b6d6e <Address 0x9940329f0b9b6d6e out of bounds>)
:> > > : at /usr/src/usr.sbin/bgpctl/parser.c:1022
:> > > :1022 if (strcmp(s, "*") == 0)
:> > > :(gdb) p s
:> > > :$1 = 0x9940329f0b9b6d6e <Address 0x9940329f0b9b6d6e out of bounds>
:> > > :(gdb) bt
:> > > :#0 getlargecommunity (
:> > > : s=0x9940329f0b9b6d6e <Address 0x9940329f0b9b6d6e out of bounds>)
:> > > : at /usr/src/usr.sbin/bgpctl/parser.c:1022
:> > >
:> > > Easy fix. Pre-initialize the array to NULL, then check if they are set.
:> > > Also, check to see if we got too many ':'.
:> > >
:> > > OK?
:> >
:> > Would strsep be a better fit here?
:>
:>
:> while ((p != NULL) && (i < 3)) {
:> val = strsep(&p, ":");
:> if (*val == '\0')
:> errx(1, "Invalid Large-Community syntax");
:> array[i++] = val;
:> }
:>
:> if ((p != NULL) || !(array[0] && array[1] && array[2]))
:> errx(1, "Invalid Large-Community syntax");
:>
:>
:> and actually the if (*val == '\0') ...
:> can be removed, because that will just result in array[x] being NULL:
:
:What about the memory leak?
:
:Anyway ok jsg@ to commit your version or something like the below.
:
I'm OK with both versions, even though bgpctl is a short running program
I prefer the non-memory leak version.
:Index: parser.c
:===================================================================
:RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
:retrieving revision 1.76
:diff -u -p -r1.76 parser.c
:--- parser.c 13 Feb 2017 14:48:44 -0000 1.76
:+++ parser.c 14 Feb 2017 12:16:46 -0000
:@@ -1033,21 +1033,26 @@ int
: parse_largecommunity(const char *word, struct parse_result *r)
: {
: struct filter_set *fs;
:- char *p = strdup(word);
:- char *array[3];
:+ char *p, *po = strdup(word);
:+ char *array[3] = { NULL, NULL, NULL };
:+ char *val;
: int64_t as, ld1, ld2;
: int i = 0;
:
:- while (p != NULL) {
:- array[i++] = p;
:- p = strchr(p, ':');
:- if (p)
:- *p++ = 0;
:+ p = po;
:+ while ((p != NULL) && (i < 3)) {
:+ val = strsep(&p, ":");
:+ array[i++] = val;
: }
:
:+ if ((p != NULL) || !(array[0] && array[1] && array[2]))
:+ errx(1, "Invalid Large-Community syntax");
:+
: as = getlargecommunity(array[0]);
: ld1 = getlargecommunity(array[1]);
: ld2 = getlargecommunity(array[2]);
:+
:+ free(po);
:
: if ((fs = calloc(1, sizeof(struct filter_set))) == NULL)
: err(1, NULL);
--
Finagle's Creed:
Science is true. Don't be misled by facts.
