On Tue, Feb 14, 2017 at 02:02:46PM +0100, Peter Hessler wrote:
> 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.
I'd like to add that some malloc features like canary checks (malloc
flag C) are done on free. That should be an incentive to fix
memory leaks, even on short running programs,
-Otto
>
>
> :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.
