Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
Re: bgpd: last bit of rde_attr_parse ibuf cleanup
To:
Theo Buehler <tb@theobuehler.org>
Cc:
tech@openbsd.org
Date:
Tue, 30 Jan 2024 14:16:40 +0100

Download raw body.

Thread
On Mon, Jan 29, 2024 at 11:39:30AM +0100, Theo Buehler wrote:
> On Thu, Jan 25, 2024 at 02:25:51PM +0100, Claudio Jeker wrote:
> > This converts the ATTR_ASPATH and ATTR_AS4_PATH handlers in
> > rde_attr_parse(). To do this the various aspath validation and print
> > functions got some ibuf treatment and the result is IMO nicer then the
> > code before.
> > 
> > Again there are tentacles in bgpctl so handle them as well. This converts
> > IMSG_CTL_SHOW_RIB and adds a bit of a hack to the show_attr() bits --
> > those will be cleaned up soon.
> > 
> > The util.c changes are the ones that need some careful review. The rest of
> > this change is rather simple.
> 
> I'm ok with this as it is, but I have a couple of comments on the
> util.c change:
> 
> > -size_t
> > -aspath_strlen(void *data, uint16_t len)
> > +static ssize_t
> > +aspath_strsize(struct ibuf *in)
> >  {
> > -	uint8_t		*seg;
> > -	int		 total_size;
> > +	struct ibuf	 buf;
> > +	ssize_t		 total_size = 0;
> >  	uint32_t	 as;
> > -	uint16_t	 seg_size;
> >  	uint8_t		 i, seg_type, seg_len;
> >  
> > -	total_size = 0;
> > -	seg = data;
> > -	for (; len > 0; len -= seg_size, seg += seg_size) {
> > -		seg_type = seg[0];
> > -		seg_len = seg[1];
> > -		seg_size = 2 + sizeof(uint32_t) * seg_len;
> > -
> > -		if (seg_type == AS_SET)
> > -			if (total_size != 0)
> > -				total_size += 3;
> > -			else
> > -				total_size += 2;
> > -		else if (total_size != 0)
> > +	ibuf_from_ibuf(&buf, in);
> > +	while (ibuf_size(&buf) > 0) {
> > +		if (ibuf_get_n8(&buf, &seg_type) == -1 ||
> > +		    ibuf_get_n8(&buf, &seg_len) == -1)
> > +			return (-1);
> 
> Should this error on seg_len == 0 like in aspath_verify()?
> (I know it is currently only called after an aspath_verify()).

I guess adding this check does not hurt. The code indeed assumes that no
aspath function is called before aspath_verify() was called. So
aspath_strsize() could depend on the fact that the aspath is valid enough
to not blow up here.

Still adding a check is cheap and I will add it.
 
> > +	while (ibuf_size(&buf) > 0) {
> > +		if (ibuf_get_n8(&buf, &seg_type) == -1 ||
> > +		    ibuf_get_n8(&buf, &seg_len) == -1) {
> > +			error = AS_ERR_LEN;
> 
> Don't think it matters, but this was previously an AS_ERR_BAD,

I think the reported error is a bit more correct with AS_ERR_LEN.
Since the lenght of the attribute is inconsistent with the segment
encoding.
 
> > -u_char *
> > -aspath_inflate(void *data, uint16_t len, uint16_t *newlen)
> > +void *
> > +aspath_inflate(struct ibuf *in, struct ibuf *out)
> >  {
> > -	uint8_t		*seg, *nseg, *ndata;
> > -	uint16_t	 seg_size, olen, nlen;
> > +	struct ibuf	 buf;
> > +	uint8_t		*nseg, *ndata;
> > +	size_t		 seg_size, nlen = 0;
> >  	uint8_t		 seg_len;
> >  
> > +	/* clone buffer so we can pass it twice without altering in */
> > +	ibuf_from_ibuf(&buf, in);
> >  	/* first calculate the length of the aspath */
> > -	seg = data;
> > -	nlen = 0;
> > -	for (olen = len; olen > 0; olen -= seg_size, seg += seg_size) {
> > -		seg_len = seg[1];
> > -		seg_size = 2 + sizeof(uint16_t) * seg_len;
> > -		nlen += 2 + sizeof(uint32_t) * seg_len;
> > -
> > -		if (seg_size > olen) {
> > -			errno = ERANGE;
> > +	while (ibuf_size(&buf) > 0) {
> > +		if (ibuf_skip(&buf, 1) == -1 ||
> > +		    ibuf_get_n8(&buf, &seg_len) == -1)
> >  			return (NULL);
> > -		}
> > +		seg_size = sizeof(uint16_t) * seg_len;
> > +		if (ibuf_skip(&buf, seg_size) == -1)
> > +			return (NULL);
> > +		nlen += 2 + sizeof(uint32_t) * seg_len;
> >  	}
> 
> This is correct and preserves behavior. Would it not be acceptable to be
> lazy and calculate nlen as 2 * ibuf_size(&buf)? This overestimates the
> size by 2 * number of segments. Does that really matter? The out buffer
> is very short lived.
> 
> Another option (a bit of a layer violation) might be to let
> aspath_verify() count the number of segments. This way you could
> calculate the inflated size precisely and save one walk over the buffer.

I agree that just over allocating is probably the way to go. Will change
the code accordingly.
 
> >  
> > -	*newlen = nlen;
> > -	if ((ndata = malloc(nlen)) == NULL)
> > +	if ((nseg = ndata = malloc(nlen)) == NULL)
> >  		return (NULL);
> >  
> > +	ibuf_from_ibuf(&buf, in);
> >  	/* then copy the aspath */
> > -	seg = data;
> > -	for (nseg = ndata; nseg < ndata + nlen; ) {
> > -		*nseg++ = *seg++;
> > -		*nseg++ = seg_len = *seg++;
> > +	while (ibuf_size(&buf) > 0) {
> > +		if (ibuf_get_n8(&buf, nseg++) == -1 ||
> 
> I don't like this manually advancing the nseg buffer. Should ndata not
> be wrapped in an ibuf to which you add the data from buf, padding with
> two zeroes for each segment? This will be a bit more code, but you'd get
> a correct size in a single loop.

Totally changed to code to return a struct ibuf *, so we can use
ibuf_open() and ibuf_add() this makes the code a lot simpler.
 
> > +		    ibuf_get_n8(&buf, &seg_len) == -1) {
> > +			free(ndata);
> > +			return (NULL);
> > +		}
> > +		*nseg++ = seg_len;
> 
> Again, I would probably error on 0 seg_len here.
> 
> >  		for (; seg_len > 0; seg_len--) {
> 
> Might be worth using an extra ibuf for the segment.

Not sure if that makes the code better. I skipped that for now. 

Need to check if aspath_inflate() is actually excercised in regress. It is
a code path that is far less travelled then the default 4byte ASnum one.
-- 
:wq Claudio


Index: bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
diff -u -p -r1.483 bgpd.h
--- bgpd.h	23 Jan 2024 16:13:35 -0000	1.483
+++ bgpd.h	30 Jan 2024 13:08:40 -0000
@@ -1542,20 +1542,19 @@ const char	*log_as(uint32_t);
 const char	*log_rd(uint64_t);
 const char	*log_ext_subtype(int, uint8_t);
 const char	*log_reason(const char *);
+const char	*log_aspath_error(int);
 const char	*log_roa(struct roa *);
 const char	*log_aspa(struct aspa_set *);
 const char	*log_rtr_error(enum rtr_error);
 const char	*log_policy(enum role);
-int		 aspath_snprint(char *, size_t, void *, uint16_t);
-int		 aspath_asprint(char **, void *, uint16_t);
-size_t		 aspath_strlen(void *, uint16_t);
+int		 aspath_asprint(char **, struct ibuf *);
 uint32_t	 aspath_extract(const void *, int);
-int		 aspath_verify(void *, uint16_t, int, int);
+int		 aspath_verify(struct ibuf *, int, int);
 #define		 AS_ERR_LEN	-1
 #define		 AS_ERR_TYPE	-2
 #define		 AS_ERR_BAD	-3
 #define		 AS_ERR_SOFT	-4
-u_char		*aspath_inflate(void *, uint16_t, uint16_t *);
+struct ibuf	*aspath_inflate(struct ibuf *);
 int		 extract_prefix(const u_char *, int, void *, uint8_t, uint8_t);
 int		 nlri_get_prefix(struct ibuf *, struct bgpd_addr *, uint8_t *);
 int		 nlri_get_prefix6(struct ibuf *, struct bgpd_addr *, uint8_t *);
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
diff -u -p -r1.619 rde.c
--- rde.c	25 Jan 2024 11:13:35 -0000	1.619
+++ rde.c	30 Jan 2024 13:10:31 -0000
@@ -1916,13 +1916,6 @@ rde_update_withdraw(struct rde_peer *pee
  */
 
 /* attribute parser specific macros */
-#define UPD_READ(t, p, plen, n) \
-	do { \
-		memcpy(t, p, n); \
-		p += n; \
-		plen += n; \
-	} while (0)
-
 #define CHECK_FLAGS(s, t, m)	\
 	(((s) & ~(ATTR_DEFMASK | (m))) == (t))
 
@@ -1932,12 +1925,10 @@ rde_attr_parse(struct ibuf *buf, struct 
 {
 	struct bgpd_addr nexthop;
 	struct rde_aspath *a = &state->aspath;
-	struct ibuf	 attrbuf, tmpbuf;
-	u_char		*p, *npath;
+	struct ibuf	 attrbuf, tmpbuf, *npath = NULL;
+	size_t		 alen, hlen;
 	uint32_t	 tmp32, zero = 0;
 	int		 error;
-	uint16_t	 nlen;
-	size_t		 attr_len, hlen, plen;
 	uint8_t		 flags, type;
 
 	ibuf_from_ibuf(&attrbuf, buf);
@@ -1945,30 +1936,26 @@ rde_attr_parse(struct ibuf *buf, struct 
 	    ibuf_get_n8(&attrbuf, &type) == -1)
 		goto bad_list;
 
-
 	if (flags & ATTR_EXTLEN) {
-		uint16_t alen;
-		if (ibuf_get_n16(&attrbuf, &alen) == -1)
+		uint16_t attr_len;
+		if (ibuf_get_n16(&attrbuf, &attr_len) == -1)
 			goto bad_list;
-		attr_len = alen;
+		alen = attr_len;
 		hlen = 4;
 	} else {
-		uint8_t alen;
-		if (ibuf_get_n8(&attrbuf, &alen) == -1)
+		uint8_t attr_len;
+		if (ibuf_get_n8(&attrbuf, &attr_len) == -1)
 			goto bad_list;
-		attr_len = alen;
+		alen = attr_len;
 		hlen = 3;
 	}
 
-	if (ibuf_truncate(&attrbuf, attr_len) == -1)
+	if (ibuf_truncate(&attrbuf, alen) == -1)
 		goto bad_list;
 	/* consume the attribute in buf before moving forward */
-	if (ibuf_skip(buf, hlen + attr_len) == -1)
+	if (ibuf_skip(buf, hlen + alen) == -1)
 		goto bad_list;
 
-	p = ibuf_data(&attrbuf);
-	plen = ibuf_size(&attrbuf);
-
 	switch (type) {
 	case ATTR_UNDEF:
 		/* ignore and drop path attributes with a type code of 0 */
@@ -1998,44 +1985,43 @@ rde_attr_parse(struct ibuf *buf, struct 
 	case ATTR_ASPATH:
 		if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0))
 			goto bad_flags;
-		error = aspath_verify(p, attr_len, peer_has_as4byte(peer),
+		if (a->flags & F_ATTR_ASPATH)
+			goto bad_list;
+		error = aspath_verify(&attrbuf, peer_has_as4byte(peer),
 		    peer_accept_no_as_set(peer));
-		if (error == AS_ERR_SOFT) {
-			/*
-			 * soft errors like unexpected segment types are
-			 * not considered fatal and the path is just
-			 * marked invalid.
-			 */
-			a->flags |= F_ATTR_PARSE_ERR;
-		} else if (error != 0) {
+		if (error != 0 && error != AS_ERR_SOFT) {
+			log_peer_warnx(&peer->conf, "bad ASPATH, %s",
+			    log_aspath_error(error));
 			rde_update_err(peer, ERR_UPDATE, ERR_UPD_ASPATH,
 			    NULL);
 			return (-1);
 		}
-		if (a->flags & F_ATTR_ASPATH)
-			goto bad_list;
 		if (peer_has_as4byte(peer)) {
-			npath = p;
-			nlen = attr_len;
+			ibuf_from_ibuf(&tmpbuf, &attrbuf);
 		} else {
-			npath = aspath_inflate(p, attr_len, &nlen);
-			if (npath == NULL)
+			if ((npath = aspath_inflate(&attrbuf)) == NULL)
 				fatal("aspath_inflate");
+			ibuf_from_ibuf(&tmpbuf, npath);
 		}
 		if (error == AS_ERR_SOFT) {
 			char *str;
 
-			aspath_asprint(&str, npath, nlen);
+			/*
+			 * soft errors like unexpected segment types are
+			 * not considered fatal and the path is just
+			 * marked invalid.
+			 */
+			a->flags |= F_ATTR_PARSE_ERR;
+
+			aspath_asprint(&str, &tmpbuf);
 			log_peer_warnx(&peer->conf, "bad ASPATH %s, "
 			    "path invalidated and prefix withdrawn",
 			    str ? str : "(bad aspath)");
 			free(str);
 		}
 		a->flags |= F_ATTR_ASPATH;
-		a->aspath = aspath_get(npath, nlen);
-		if (npath != p)
-			free(npath);
-		plen += attr_len;
+		a->aspath = aspath_get(ibuf_data(&tmpbuf), ibuf_size(&tmpbuf));
+		ibuf_free(npath);
 		break;
 	case ATTR_NEXTHOP:
 		if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0))
@@ -2050,7 +2036,6 @@ rde_attr_parse(struct ibuf *buf, struct 
 		nexthop.aid = AID_INET;
 		if (ibuf_get_h32(&attrbuf, &nexthop.v4.s_addr) == -1)
 			goto bad_len;
-
 		/*
 		 * Check if the nexthop is a valid IP address. We consider
 		 * multicast addresses as invalid.
@@ -2245,12 +2230,11 @@ rde_attr_parse(struct ibuf *buf, struct 
 		if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE,
 		    ATTR_PARTIAL))
 			goto bad_flags;
-		if ((error = aspath_verify(p, attr_len, 1,
+		if ((error = aspath_verify(&attrbuf, 1,
 		    peer_accept_no_as_set(peer))) != 0) {
 			/* As per RFC6793 use "attribute discard" here. */
 			log_peer_warnx(&peer->conf, "bad AS4_PATH, "
 			    "attribute discarded");
-			plen += attr_len;
 			break;
 		}
 		a->flags |= F_ATTR_AS4BYTE_NEW;
@@ -2295,7 +2279,6 @@ rde_attr_parse(struct ibuf *buf, struct 
 		break;
 	}
 
-	(void)plen;	/* XXX make compiler happy for now */
 	return (0);
 
  bad_len:
@@ -2309,7 +2292,6 @@ rde_attr_parse(struct ibuf *buf, struct 
 	return (-1);
 }
 
-#undef UPD_READ
 #undef CHECK_FLAGS
 
 int
Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
diff -u -p -r1.79 util.c
--- util.c	23 Jan 2024 16:13:35 -0000	1.79
+++ util.c	30 Jan 2024 13:08:20 -0000
@@ -32,8 +32,6 @@
 #include "rde.h"
 #include "log.h"
 
-const char	*aspath_delim(uint8_t, int);
-
 const char *
 log_addr(const struct bgpd_addr *addr)
 {
@@ -236,6 +234,26 @@ log_aspa(struct aspa_set *aspa)
 }
 
 const char *
+log_aspath_error(int error)
+{
+	static char buf[20];
+
+	switch (error) {
+	case AS_ERR_LEN:
+		return "inconsitent lenght";
+	case AS_ERR_TYPE:
+		return "unknown segment type";
+	case AS_ERR_BAD:
+		return "invalid encoding";
+	case AS_ERR_SOFT:
+		return "soft failure";
+	default:
+		snprintf(buf, sizeof(buf), "unknown %d", error);
+		return buf;
+	}
+}
+
+const char *
 log_rtr_error(enum rtr_error err)
 {
 	static char buf[20];
@@ -286,7 +304,7 @@ log_policy(enum role role)
 	}
 }
 
-const char *
+static const char *
 aspath_delim(uint8_t seg_type, int closing)
 {
 	static char db[8];
@@ -318,42 +336,38 @@ aspath_delim(uint8_t seg_type, int closi
 	}
 }
 
-int
-aspath_snprint(char *buf, size_t size, void *data, uint16_t len)
+static int
+aspath_snprint(char *buf, size_t size, struct ibuf *in)
 {
-#define UPDATE()				\
-	do {					\
-		if (r < 0)			\
-			return (-1);		\
-		total_size += r;		\
-		if ((unsigned int)r < size) {	\
-			size -= r;		\
-			buf += r;		\
-		} else {			\
-			buf += size;		\
-			size = 0;		\
-		}				\
+#define UPDATE()						\
+	do {							\
+		if (r < 0 || (unsigned int)r >= size)		\
+			return (-1);				\
+		size -= r;					\
+		buf += r;					\
 	} while (0)
-	uint8_t		*seg;
-	int		 r, total_size;
-	uint16_t	 seg_size;
-	uint8_t		 i, seg_type, seg_len;
 
-	total_size = 0;
-	seg = data;
-	for (; len > 0; len -= seg_size, seg += seg_size) {
-		seg_type = seg[0];
-		seg_len = seg[1];
-		seg_size = 2 + sizeof(uint32_t) * seg_len;
+	struct ibuf	data;
+	uint32_t	as;
+	int		r, n = 0;
+	uint8_t		i, seg_type, seg_len;
+
+	ibuf_from_ibuf(&data, in);
+	while (ibuf_size(&data) > 0) {
+		if (ibuf_get_n8(&data, &seg_type) == -1 ||
+		    ibuf_get_n8(&data, &seg_len) == -1 ||
+		    seg_len == 0)
+			return (-1);
 
-		r = snprintf(buf, size, "%s%s",
-		    total_size != 0 ? " " : "",
+		r = snprintf(buf, size, "%s%s", n++ != 0 ? " " : "",
 		    aspath_delim(seg_type, 0));
 		UPDATE();
 
 		for (i = 0; i < seg_len; i++) {
-			r = snprintf(buf, size, "%s",
-			    log_as(aspath_extract(seg, i)));
+			if (ibuf_get_n32(&data, &as) == -1)
+				return -1;
+
+			r = snprintf(buf, size, "%s", log_as(as));
 			UPDATE();
 			if (i + 1 < seg_len) {
 				r = snprintf(buf, size, " ");
@@ -364,73 +378,68 @@ aspath_snprint(char *buf, size_t size, v
 		UPDATE();
 	}
 	/* ensure that we have a valid C-string especially for empty as path */
-	if (size > 0)
-		*buf = '\0';
-
-	return (total_size);
-#undef UPDATE
-}
-
-int
-aspath_asprint(char **ret, void *data, uint16_t len)
-{
-	size_t	slen;
-	int	plen;
-
-	slen = aspath_strlen(data, len) + 1;
-	*ret = malloc(slen);
-	if (*ret == NULL)
-		return (-1);
-
-	plen = aspath_snprint(*ret, slen, data, len);
-	if (plen == -1) {
-		free(*ret);
-		*ret = NULL;
-		return (-1);
-	}
-
+	*buf = '\0';
 	return (0);
+#undef UPDATE
 }
 
-size_t
-aspath_strlen(void *data, uint16_t len)
+static ssize_t
+aspath_strsize(struct ibuf *in)
 {
-	uint8_t		*seg;
-	int		 total_size;
+	struct ibuf	 buf;
+	ssize_t		 total_size = 0;
 	uint32_t	 as;
-	uint16_t	 seg_size;
 	uint8_t		 i, seg_type, seg_len;
 
-	total_size = 0;
-	seg = data;
-	for (; len > 0; len -= seg_size, seg += seg_size) {
-		seg_type = seg[0];
-		seg_len = seg[1];
-		seg_size = 2 + sizeof(uint32_t) * seg_len;
-
-		if (seg_type == AS_SET)
-			if (total_size != 0)
-				total_size += 3;
-			else
-				total_size += 2;
-		else if (total_size != 0)
+	ibuf_from_ibuf(&buf, in);
+	while (ibuf_size(&buf) > 0) {
+		if (ibuf_get_n8(&buf, &seg_type) == -1 ||
+		    ibuf_get_n8(&buf, &seg_len) == -1 ||
+		    seg_len == 0)
+			return (-1);
+
+		if (total_size != 0)
 			total_size += 1;
+		total_size += strlen(aspath_delim(seg_type, 0));
 
 		for (i = 0; i < seg_len; i++) {
-			as = aspath_extract(seg, i);
+			if (ibuf_get_n32(&buf, &as) == -1)
+				return (-1);
 
 			do {
 				total_size++;
 			} while ((as = as / 10) != 0);
-
-			if (i + 1 < seg_len)
-				total_size += 1;
 		}
+		total_size += seg_len - 1;
 
-		if (seg_type == AS_SET)
-			total_size += 2;
+		total_size += strlen(aspath_delim(seg_type, 1));
 	}
-	return (total_size);
+	return (total_size + 1);
+}
+
+int
+aspath_asprint(char **ret, struct ibuf *data)
+{
+	ssize_t	slen;
+
+	if ((slen = aspath_strsize(data)) == -1) {
+		*ret = NULL;
+		errno = EINVAL;
+		return (-1);
+	}
+
+	*ret = malloc(slen);
+	if (*ret == NULL)
+		return (-1);
+
+	if (aspath_snprint(*ret, slen, data) == -1) {
+		free(*ret);
+		*ret = NULL;
+		errno = EINVAL;
+		return (-1);
+	}
+
+	return (0);
 }
 
 /*
@@ -456,32 +465,31 @@ aspath_extract(const void *seg, int pos)
  * Verify that the aspath is correctly encoded.
  */
 int
-aspath_verify(void *data, uint16_t len, int as4byte, int noset)
+aspath_verify(struct ibuf *in, int as4byte, int noset)
 {
-	uint8_t		*seg = data;
-	uint16_t	 seg_size, as_size = 2;
+	struct ibuf	 buf;
+	int		 pos, error = 0;
 	uint8_t		 seg_len, seg_type;
-	int		 error = 0;
 
-	if (len & 1)
+	ibuf_from_ibuf(&buf, in);
+	if (ibuf_size(&buf) & 1) {
 		/* odd length aspath are invalid */
-		return (AS_ERR_BAD);
-
-	if (as4byte)
-		as_size = 4;
-
-	for (; len > 0; len -= seg_size, seg += seg_size) {
-		const uint8_t	*ptr;
-		int		 pos;
+		error = AS_ERR_BAD;
+		goto done;
+	}
 
-		if (len < 2)	/* header length check */
-			return (AS_ERR_BAD);
-		seg_type = seg[0];
-		seg_len = seg[1];
+	while (ibuf_size(&buf) > 0) {
+		if (ibuf_get_n8(&buf, &seg_type) == -1 ||
+		    ibuf_get_n8(&buf, &seg_len) == -1) {
+			error = AS_ERR_LEN;
+			goto done;
+		}
 
-		if (seg_len == 0)
+		if (seg_len == 0) {
 			/* empty aspath segments are not allowed */
-			return (AS_ERR_BAD);
+			error = AS_ERR_BAD;
+			goto done;
+		}
 
 		/*
 		 * BGP confederations should not show up but consider them
@@ -497,70 +505,75 @@ aspath_verify(void *data, uint16_t len, 
 		if (noset && seg_type == AS_SET)
 			error = AS_ERR_SOFT;
 		if (seg_type != AS_SET && seg_type != AS_SEQUENCE &&
-		    seg_type != AS_CONFED_SEQUENCE && seg_type != AS_CONFED_SET)
-			return (AS_ERR_TYPE);
-
-		seg_size = 2 + as_size * seg_len;
-
-		if (seg_size > len)
-			return (AS_ERR_LEN);
+		    seg_type != AS_CONFED_SEQUENCE &&
+		    seg_type != AS_CONFED_SET) {
+			error = AS_ERR_TYPE;
+			goto done;
+		}
 
 		/* RFC 7607 - AS 0 is considered malformed */
-		ptr = seg + 2;
 		for (pos = 0; pos < seg_len; pos++) {
 			uint32_t as;
 
-			memcpy(&as, ptr, as_size);
+			if (as4byte) {
+				if (ibuf_get_n32(&buf, &as) == -1) {
+					error = AS_ERR_LEN;
+					goto done;
+				}
+			} else {
+				uint16_t tmp;
+				if (ibuf_get_n16(&buf, &tmp) == -1) {
+					error = AS_ERR_LEN;
+					goto done;
+				}
+				as = tmp;
+			}
 			if (as == 0)
 				error = AS_ERR_SOFT;
-			ptr += as_size;
 		}
 	}
+
+ done:
 	return (error);	/* aspath is valid but probably not loop free */
 }
 
 /*
  * convert a 2 byte aspath to a 4 byte one.
  */
-u_char *
-aspath_inflate(void *data, uint16_t len, uint16_t *newlen)
+struct ibuf *
+aspath_inflate(struct ibuf *in)
 {
-	uint8_t		*seg, *nseg, *ndata;
-	uint16_t	 seg_size, olen, nlen;
-	uint8_t		 seg_len;
-
-	/* first calculate the length of the aspath */
-	seg = data;
-	nlen = 0;
-	for (olen = len; olen > 0; olen -= seg_size, seg += seg_size) {
-		seg_len = seg[1];
-		seg_size = 2 + sizeof(uint16_t) * seg_len;
-		nlen += 2 + sizeof(uint32_t) * seg_len;
-
-		if (seg_size > olen) {
-			errno = ERANGE;
-			return (NULL);
-		}
-	}
+	struct ibuf	*out;
+	uint16_t	 short_as;
+	uint8_t		 seg_type, seg_len;
 
-	*newlen = nlen;
-	if ((ndata = malloc(nlen)) == NULL)
+	/* allocate enough space for the worst case */
+	if ((out = ibuf_open(ibuf_size(in) * 2)) == NULL)
 		return (NULL);
 
 	/* then copy the aspath */
-	seg = data;
-	for (nseg = ndata; nseg < ndata + nlen; ) {
-		*nseg++ = *seg++;
-		*nseg++ = seg_len = *seg++;
+	while (ibuf_size(in) > 0) {
+		if (ibuf_get_n8(in, &seg_type) == -1 ||
+		    ibuf_get_n8(in, &seg_len) == -1 ||
+		    seg_len == 0)
+			goto fail;
+		if (ibuf_add_n8(out, seg_type) == -1 ||
+		    ibuf_add_n8(out, seg_len) == -1)
+			goto fail;
+
 		for (; seg_len > 0; seg_len--) {
-			*nseg++ = 0;
-			*nseg++ = 0;
-			*nseg++ = *seg++;
-			*nseg++ = *seg++;
+			if (ibuf_get_n16(in, &short_as) == -1)
+				goto fail;
+			if (ibuf_add_n32(out, short_as) == -1)
+				goto fail;
 		}
 	}
 
-	return (ndata);
+	return (out);
+
+fail:
+	ibuf_free(out);
+	return (NULL);
 }
 
 static const u_char	addrmask[] = { 0x00, 0x80, 0xc0, 0xe0, 0xf0,