Download raw body.
bgpd: last bit of rde_attr_parse ibuf cleanup
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()).
> +
> + 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 +463,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;
Don't think it matters, but this was previously an AS_ERR_BAD,
> + 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,69 +503,86 @@ 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)
> +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.
>
> - *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.
> + 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.
> *nseg++ = 0;
> *nseg++ = 0;
> - *nseg++ = *seg++;
> - *nseg++ = *seg++;
> + if (ibuf_get_n8(&buf, nseg++) == -1 ||
> + ibuf_get_n8(&buf, nseg++) == -1) {
> + free(ndata);
> + return (NULL);
> + }
> }
> }
>
> + ibuf_from_buffer(out, ndata, nlen);
> return (ndata);
> }
>
>
bgpd: last bit of rde_attr_parse ibuf cleanup