Index | Thread | Search

From:
Theo Buehler <tb@theobuehler.org>
Subject:
Re: bgpd rewrite rtr pdu parser to new ibuf API
To:
tech@openbsd.org
Date:
Thu, 4 Jan 2024 12:57:36 +0100

Download raw body.

Thread
On Thu, Jan 04, 2024 at 11:36:40AM +0100, Claudio Jeker wrote:
> This diff converts the RTR PDU parser to use the ibuf API.
> More cleanup is possible but lets start with a "minimal" conversion.

Yes, it's big enough.

> The ASPA PDU handling is maybe a bit ugly but that part will change once
> SIDROPS made up their mind about how this PDU should look like.

:)

> This introduces ibuf_get_string() which I plan to add to libutil since I
> used the same function in xlock privsep code. Extracting a string from
> an imsg seems to be somewhat common and this API ensures that the string
> is NUL terminated.

This generally looks good. I think there's one bug due to the removal of
the rv dance in rtr_parse_error(). Apart from that it's just nits as
usual.

> 
> -- 
> :wq Claudio
> 
> Index: rtr_proto.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> diff -u -p -r1.21 rtr_proto.c
> --- rtr_proto.c	3 Jan 2024 16:07:37 -0000	1.21
> +++ rtr_proto.c	4 Jan 2024 10:05:26 -0000
> @@ -266,10 +266,10 @@ rtr_newmsg(struct rtr_session *rs, enum 
>   */
>  static void
>  rtr_send_error(struct rtr_session *rs, enum rtr_error err, char *msg,
> -    void *pdu, size_t len)
> +    struct ibuf *pdu)
>  {
>  	struct ibuf *buf;
> -	size_t mlen = 0;
> +	size_t len = 0, mlen = 0;
>  
>  	rs->last_sent_error = err;
>  	if (msg) {
> @@ -278,14 +278,21 @@ rtr_send_error(struct rtr_session *rs, e
>  	} else
>  		memset(rs->last_sent_msg, 0, sizeof(rs->last_sent_msg));
>  
> +	if (pdu) {

I'd prefer an explicit != NULL (also in the if (msg) above).

> +		ibuf_rewind(pdu);
> +		len = ibuf_size(pdu);
> +	}
> +
>  	buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(uint32_t) + len + mlen,
>  	    err);
>  	if (buf == NULL)
>  		goto fail;
>  	if (ibuf_add_n32(buf, len) == -1)
>  		goto fail;
> -	if (ibuf_add(buf, pdu, len) == -1)
> -		goto fail;
> +	if (pdu) {

same here

> +		if (ibuf_add_ibuf(buf, pdu) == -1)
> +			goto fail;
> +	}
>  	if (ibuf_add_n32(buf, mlen) == -1)
>  		goto fail;
>  	if (ibuf_add(buf, msg, mlen) == -1)
> @@ -311,7 +318,7 @@ rtr_send_reset_query(struct rtr_session 
>  	buf = rtr_newmsg(rs, RESET_QUERY, 0, 0);
>  	if (buf == NULL) {
>  		log_warn("rtr %s: send reset query", log_rtr(rs));
> -		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> +		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL);
>  		return;
>  	}
>  	ibuf_close(&rs->w, buf);
> @@ -333,7 +340,7 @@ rtr_send_serial_query(struct rtr_session
>   fail:
>  	log_warn("rtr %s: send serial query", log_rtr(rs));
>  	ibuf_free(buf);
> -	rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> +	rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL);
>  }
>  
>  /*
> @@ -343,20 +350,21 @@ rtr_send_serial_query(struct rtr_session
>   * and the function return 0.
>   */
>  static int
> -rtr_parse_header(struct rtr_session *rs, void *buf,
> +rtr_parse_header(struct rtr_session *rs, struct ibuf *hdr,
>      size_t *msglen, enum rtr_pdu_type *msgtype)
>  {
>  	struct rtr_header rh;
>  	uint32_t len = 16;	/* default for ERROR_REPORT */
>  	int session_id;
>  
> -	memcpy(&rh, buf, sizeof(rh));
> +	if (ibuf_get(hdr, &rh, sizeof(rh)) == -1)
> +		fatal("%s: ibuf_get", __func__);
>  
>  	if (rh.version != rs->version && rh.type != ERROR_REPORT) {
>   badversion:
>  		log_warnx("rtr %s: received %s message: unexpected version %d",
>  		    log_rtr(rs), log_rtr_type(rh.type), rh.version);
> -		rtr_send_error(rs, UNEXP_PROTOCOL_VERS, NULL, &rh, sizeof(rh));
> +		rtr_send_error(rs, UNEXP_PROTOCOL_VERS, NULL, hdr);
>  		return -1;
>  	}
>  
> @@ -401,8 +409,7 @@ rtr_parse_header(struct rtr_session *rs,
>   toobig:
>  			log_warnx("rtr %s: received %s: msg too big: %zu byte",
>  			    log_rtr(rs), log_rtr_type(rh.type), *msglen);
> -			rtr_send_error(rs, CORRUPT_DATA, "too big",
> -			    &rh, sizeof(rh));
> +			rtr_send_error(rs, CORRUPT_DATA, "too big", hdr);
>  			return -1;
>  		}
>  		if (*msglen < len) {
> @@ -410,8 +417,7 @@ rtr_parse_header(struct rtr_session *rs,
>  			log_warnx("rtr %s: received %s: msg too small: "
>  			    "%zu byte", log_rtr(rs), log_rtr_type(rh.type),
>  			    *msglen);
> -			rtr_send_error(rs, CORRUPT_DATA, "too small",
> -			    &rh, sizeof(rh));
> +			rtr_send_error(rs, CORRUPT_DATA, "too small", hdr);
>  			return -1;
>  		}
>  		/*
> @@ -434,15 +440,14 @@ rtr_parse_header(struct rtr_session *rs,
>  	default:
>  		log_warnx("rtr %s: received unknown message: type %s",
>  		    log_rtr(rs), log_rtr_type(rh.type));
> -		rtr_send_error(rs, UNSUPP_PDU_TYPE, NULL, &rh, sizeof(rh));
> +		rtr_send_error(rs, UNSUPP_PDU_TYPE, NULL, hdr);
>  		return -1;
>  	}
>  
>  	if (len != *msglen) {
>  		log_warnx("rtr %s: received %s: illegal len: %zu byte not %u",
>  		    log_rtr(rs), log_rtr_type(rh.type), *msglen, len);
> -		rtr_send_error(rs, CORRUPT_DATA, "bad length",
> -		    &rh, sizeof(rh));
> +		rtr_send_error(rs, CORRUPT_DATA, "bad length", hdr);
>  		return -1;
>  	}
>  
> @@ -454,8 +459,7 @@ rtr_parse_header(struct rtr_session *rs,
>  		log_warnx("rtr %s: received %s: bad session_id: %d != %d",
>  		    log_rtr(rs), log_rtr_type(rh.type), ntohs(rh.session_id),
>  		    session_id);
> -		rtr_send_error(rs, CORRUPT_DATA, "bad session_id",
> -		    &rh, sizeof(rh));
> +		rtr_send_error(rs, CORRUPT_DATA, "bad session_id", hdr);
>  		return -1;
>  	}
>  
> @@ -463,7 +467,7 @@ rtr_parse_header(struct rtr_session *rs,
>  }
>  
>  static int
> -rtr_parse_notify(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_notify(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	if (rs->state == RTR_STATE_EXCHANGE ||
>  	    rs->state == RTR_STATE_NEGOTIATION) {
> @@ -478,7 +482,7 @@ rtr_parse_notify(struct rtr_session *rs,
>  }
>  
>  static int
> -rtr_parse_cache_response(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_cache_response(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	if (rs->state != RTR_STATE_ESTABLISHED &&
>  	    rs->state != RTR_STATE_NEGOTIATION) {
> @@ -492,39 +496,39 @@ rtr_parse_cache_response(struct rtr_sess
>  }
>  
>  static int
> -rtr_parse_ipv4_prefix(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_ipv4_prefix(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	struct rtr_ipv4 ip4;
>  	struct roa *roa;
>  
> -	if (len != sizeof(struct rtr_header) + sizeof(ip4)) {
> +	if (ibuf_skip(pdu, sizeof(struct rtr_header)) == -1 ||
> +	    ibuf_get(pdu, &ip4, sizeof(ip4)) == -1) {
>  		log_warnx("rtr %s: received %s: bad pdu len",
>  		    log_rtr(rs), log_rtr_type(IPV4_PREFIX));
> -		rtr_send_error(rs, CORRUPT_DATA, "bad len", buf, len);
> +		rtr_send_error(rs, CORRUPT_DATA, "bad len", pdu);
>  		return -1;
>  	}
>  
>  	if (rs->state != RTR_STATE_EXCHANGE) {
>  		log_warnx("rtr %s: received %s: out of context",
>  		    log_rtr(rs), log_rtr_type(IPV4_PREFIX));
> -		rtr_send_error(rs, CORRUPT_DATA, NULL, buf, len);
> +		rtr_send_error(rs, CORRUPT_DATA, NULL, pdu);
>  		return -1;
>  	}
>  
> -	memcpy(&ip4, buf + sizeof(struct rtr_header), sizeof(ip4));
>  	if (ip4.prefixlen > 32 || ip4.maxlen > 32 ||
>  	    ip4.prefixlen > ip4.maxlen) {
>  		log_warnx("rtr: %s: received %s: bad prefixlen / maxlen",
>  		    log_rtr(rs), log_rtr_type(IPV4_PREFIX));
>  		rtr_send_error(rs, CORRUPT_DATA, "bad prefixlen / maxlen",
> -		    buf, len);
> +		    pdu);

Can't this be unwrapped?

>  		return -1;
>  	}
>  
>  	if ((roa = calloc(1, sizeof(*roa))) == NULL) {
>  		log_warn("rtr %s: received %s",
>  		    log_rtr(rs), log_rtr_type(IPV4_PREFIX));
> -		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> +		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL);
>  		return -1;
>  	}
>  	roa->aid = AID_INET;
> @@ -537,7 +541,7 @@ rtr_parse_ipv4_prefix(struct rtr_session
>  		if (RB_INSERT(roa_tree, &rs->roa_set, roa) != NULL) {
>  			log_warnx("rtr %s: received %s: duplicate announcement",
>  			    log_rtr(rs), log_rtr_type(IPV4_PREFIX));
> -			rtr_send_error(rs, DUP_REC_RECV, NULL, buf, len);
> +			rtr_send_error(rs, DUP_REC_RECV, NULL, pdu);
>  			free(roa);
>  			return -1;
>  		}
> @@ -548,7 +552,7 @@ rtr_parse_ipv4_prefix(struct rtr_session
>  		if (r == NULL) {
>  			log_warnx("rtr %s: received %s: unknown withdrawal",
>  			    log_rtr(rs), log_rtr_type(IPV4_PREFIX));
> -			rtr_send_error(rs, UNK_REC_WDRAWL, NULL, buf, len);
> +			rtr_send_error(rs, UNK_REC_WDRAWL, NULL, pdu);
>  			free(roa);
>  			return -1;
>  		}
> @@ -561,39 +565,39 @@ rtr_parse_ipv4_prefix(struct rtr_session
>  }
>  
>  static int
> -rtr_parse_ipv6_prefix(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_ipv6_prefix(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	struct rtr_ipv6 ip6;
>  	struct roa *roa;
>  
> -	if (len != sizeof(struct rtr_header) + sizeof(ip6)) {
> +	if (ibuf_skip(pdu, sizeof(struct rtr_header)) == -1 ||
> +	    ibuf_get(pdu, &ip6, sizeof(ip6)) == -1) {
>  		log_warnx("rtr %s: received %s: bad pdu len",
>  		    log_rtr(rs), log_rtr_type(IPV6_PREFIX));
> -		rtr_send_error(rs, CORRUPT_DATA, "bad len", buf, len);
> +		rtr_send_error(rs, CORRUPT_DATA, "bad len", pdu);
>  		return -1;
>  	}
>  
>  	if (rs->state != RTR_STATE_EXCHANGE) {
>  		log_warnx("rtr %s: received %s: out of context",
>  		    log_rtr(rs), log_rtr_type(IPV6_PREFIX));
> -		rtr_send_error(rs, CORRUPT_DATA, NULL, buf, len);
> +		rtr_send_error(rs, CORRUPT_DATA, NULL, pdu);
>  		return -1;
>  	}
>  
> -	memcpy(&ip6, buf + sizeof(struct rtr_header), sizeof(ip6));
>  	if (ip6.prefixlen > 128 || ip6.maxlen > 128 ||
>  	    ip6.prefixlen > ip6.maxlen) {
>  		log_warnx("rtr: %s: received %s: bad prefixlen / maxlen",
>  		    log_rtr(rs), log_rtr_type(IPV6_PREFIX));
>  		rtr_send_error(rs, CORRUPT_DATA, "bad prefixlen / maxlen",
> -		    buf, len);
> +		    pdu);

unwrap?

>  		return -1;
>  	}
>  
>  	if ((roa = calloc(1, sizeof(*roa))) == NULL) {
>  		log_warn("rtr %s: received %s",
>  		    log_rtr(rs), log_rtr_type(IPV6_PREFIX));
> -		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> +		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL);
>  		return -1;
>  	}
>  	roa->aid = AID_INET6;
> @@ -606,7 +610,7 @@ rtr_parse_ipv6_prefix(struct rtr_session
>  		if (RB_INSERT(roa_tree, &rs->roa_set, roa) != NULL) {
>  			log_warnx("rtr %s: received %s: duplicate announcement",
>  			    log_rtr(rs), log_rtr_type(IPV6_PREFIX));
> -			rtr_send_error(rs, DUP_REC_RECV, NULL, buf, len);
> +			rtr_send_error(rs, DUP_REC_RECV, NULL, pdu);
>  			free(roa);
>  			return -1;
>  		}
> @@ -617,7 +621,7 @@ rtr_parse_ipv6_prefix(struct rtr_session
>  		if (r == NULL) {
>  			log_warnx("rtr %s: received %s: unknown withdrawal",
>  			    log_rtr(rs), log_rtr_type(IPV6_PREFIX));
> -			rtr_send_error(rs, UNK_REC_WDRAWL, NULL, buf, len);
> +			rtr_send_error(rs, UNK_REC_WDRAWL, NULL, pdu);
>  			free(roa);
>  			return -1;
>  		}
> @@ -629,28 +633,32 @@ rtr_parse_ipv6_prefix(struct rtr_session
>  }
>  
>  static int
> -rtr_parse_aspa(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_aspa(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	struct rtr_aspa rtr_aspa;
>  	struct aspa_tree *aspatree;
>  	struct aspa_set *aspa, *a;
> -	size_t offset;
>  	uint16_t cnt, i;
>  
> -	memcpy(&rtr_aspa, buf + sizeof(struct rtr_header), sizeof(rtr_aspa));
> -	offset = sizeof(struct rtr_header) + sizeof(rtr_aspa);
> +	if (ibuf_skip(pdu, sizeof(struct rtr_header)) == -1 ||
> +	    ibuf_get(pdu, &rtr_aspa, sizeof(rtr_aspa)) == -1) {
> +		log_warnx("rtr %s: received %s: bad pdu len",
> +		    log_rtr(rs), log_rtr_type(ASPA));
> +		rtr_send_error(rs, CORRUPT_DATA, "bad len", pdu);
> +		return -1;
> +	}
>  	cnt = ntohs(rtr_aspa.cnt);
> -	if (len != offset + cnt * sizeof(uint32_t)) {
> +	if (ibuf_size(pdu) != cnt * sizeof(uint32_t)) {
>  		log_warnx("rtr %s: received %s: bad pdu len",
>  		    log_rtr(rs), log_rtr_type(ASPA));
> -		rtr_send_error(rs, CORRUPT_DATA, "bad len", buf, len);
> +		rtr_send_error(rs, CORRUPT_DATA, "bad len", pdu);
>  		return -1;
>  	}
>  
>  	if (rs->state != RTR_STATE_EXCHANGE) {
>  		log_warnx("rtr %s: received %s: out of context",
>  		    log_rtr(rs), log_rtr_type(ASPA));
> -		rtr_send_error(rs, CORRUPT_DATA, NULL, buf, len);
> +		rtr_send_error(rs, CORRUPT_DATA, NULL, pdu);
>  		return -1;
>  	}
>  
> @@ -664,7 +672,7 @@ rtr_parse_aspa(struct rtr_session *rs, u
>  	if ((aspa = calloc(1, sizeof(*aspa))) == NULL) {
>  		log_warn("rtr %s: received %s",
>  		    log_rtr(rs), log_rtr_type(ASPA));
> -		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> +		rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL);
>  		return -1;
>  	}
>  	aspa->as = ntohl(rtr_aspa.cas);
> @@ -675,14 +683,17 @@ rtr_parse_aspa(struct rtr_session *rs, u
>  			log_warn("rtr %s: received %s",
>  			    log_rtr(rs), log_rtr_type(ASPA));
>  			rtr_send_error(rs, INTERNAL_ERROR, "out of memory",
> -			    NULL, 0);
> +			    NULL);
>  			return -1;
>  		}
>  		for (i = 0; i < cnt; i++) {
> -			uint32_t tas;
> -			memcpy(&tas, buf + offset + i * sizeof(tas),
> -			    sizeof(tas));
> -			aspa->tas[i] = ntohl(tas);
> +			if (ibuf_get_n32(pdu, &aspa->tas[i]) == -1) {
> +				log_warnx("rtr %s: received %s: bad pdu len",
> +				    log_rtr(rs), log_rtr_type(ASPA));
> +				rtr_send_error(rs, CORRUPT_DATA, "bad len",
> +				    pdu);
> +				return -1;
> +			}
>  		}
>  	}
>  
> @@ -696,7 +707,7 @@ rtr_parse_aspa(struct rtr_session *rs, u
>  				log_warnx("rtr %s: received %s: corrupt tree",
>  				    log_rtr(rs), log_rtr_type(ASPA));
>  				rtr_send_error(rs, INTERNAL_ERROR,
> -				    "corrupt aspa tree", NULL, 0);
> +				    "corrupt aspa tree", NULL);
>  				free_aspa(aspa);
>  				return -1;
>  			}
> @@ -706,7 +717,7 @@ rtr_parse_aspa(struct rtr_session *rs, u
>  		if (a == NULL) {
>  			log_warnx("rtr %s: received %s: unknown withdrawal",
>  			    log_rtr(rs), log_rtr_type(ASPA));
> -			rtr_send_error(rs, UNK_REC_WDRAWL, NULL, buf, len);
> +			rtr_send_error(rs, UNK_REC_WDRAWL, NULL, pdu);
>  			free_aspa(aspa);
>  			return -1;
>  		}
> @@ -719,15 +730,13 @@ rtr_parse_aspa(struct rtr_session *rs, u
>  }
>  
>  static int
> -rtr_parse_end_of_data(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_end_of_data(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	struct rtr_endofdata eod;
>  	uint32_t t;
>  
> -	buf += sizeof(struct rtr_header);
> -	len -= sizeof(struct rtr_header);
> -
> -	if (len != sizeof(eod)) {
> +	if (ibuf_skip(pdu, sizeof(struct rtr_header)) == -1 ||
> +	    ibuf_get(pdu, &eod, sizeof(eod)) == -1) {
>  		log_warnx("rtr %s: received %s: bad pdu len",
>  		    log_rtr(rs), log_rtr_type(END_OF_DATA));
>  		return -1;
> @@ -739,8 +748,6 @@ rtr_parse_end_of_data(struct rtr_session
>  		return -1;
>  	}
>  
> -	memcpy(&eod, buf, sizeof(eod));
> -
>  	rs->serial = ntohl(eod.serial);
>  	/* validate timer values to be in the right range */
>  	t = ntohl(eod.refresh);
> @@ -768,7 +775,7 @@ bad:
>  }
>  
>  static int
> -rtr_parse_cache_reset(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_cache_reset(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	if (rs->state != RTR_STATE_ESTABLISHED) {
>  		log_warnx("rtr %s: received %s: out of context",
> @@ -780,61 +787,60 @@ rtr_parse_cache_reset(struct rtr_session
>  	return 0;
>  }
>  
> +static char *
> +ibuf_get_string(struct ibuf *buf, size_t len)
> +{
> +	char *str;
> +
> +	if (ibuf_size(buf) < len) {
> +		errno = EBADMSG;
> +		return (NULL);
> +	}
> +	str = strndup(ibuf_data(buf), len);
> +	if (str == NULL)
> +		return (NULL);
> +	ibuf_skip(buf, len);
> +	return (str);
> +}
> +
>  /*
>   * Parse an Error Response message. This function behaves a bit different
>   * from other parse functions since on error the connection needs to be
>   * dropped without sending an error response back.
>   */
>  static int
> -rtr_parse_error(struct rtr_session *rs, uint8_t *buf, size_t len)
> +rtr_parse_error(struct rtr_session *rs, struct ibuf *pdu)
>  {
>  	struct rtr_header rh;
> +	struct ibuf err_pdu;
>  	uint32_t pdu_len, msg_len;
> -	uint8_t *msg;
>  	char *str = NULL;
>  	uint16_t errcode;
> -	int rv = -1;
>  
> -	memcpy(&rh, buf, sizeof(rh));
> -	buf += sizeof(struct rtr_header);
> -	len -= sizeof(struct rtr_header);
> +	if (ibuf_get(pdu, &rh, sizeof(rh)) == -1)
> +		goto fail;
>  	errcode = ntohs(rh.session_id);
>  
> -	memcpy(&pdu_len, buf, sizeof(pdu_len));
> -	pdu_len = ntohl(pdu_len);
> -
> -	if (len < pdu_len + sizeof(pdu_len)) {
> -		log_warnx("rtr %s: received %s: bad encapsulated pdu len: %u "
> -		    "byte", log_rtr(rs), log_rtr_type(ERROR_REPORT), pdu_len);
> -		rtr_fsm(rs, RTR_EVNT_RESET_AND_CLOSE);
> -		return -1;
> -	}
> +	if (ibuf_get_n32(pdu, &pdu_len) == -1)
> +		goto fail;
>  
>  	/* for now just ignore the embedded pdu */
> -	buf += pdu_len + sizeof(pdu_len);
> -	len -= pdu_len + sizeof(pdu_len);
> -
> -	memcpy(&msg_len, buf, sizeof(msg_len));
> -	msg_len = ntohl(msg_len);
> +	if (ibuf_get_ibuf(pdu, pdu_len, &err_pdu) == -1)
> +		goto fail;
>  
> -	if (len < msg_len + sizeof(msg_len)) {
> -		log_warnx("rtr %s: received %s: bad msg len: %u byte",
> -		    log_rtr(rs), log_rtr_type(ERROR_REPORT), msg_len);
> -		rtr_fsm(rs, RTR_EVNT_RESET_AND_CLOSE);
> -		return -1;
> -	}
> +	if (ibuf_get_n32(pdu, &msg_len) == -1)
> +		goto fail;
>  
> -	msg = buf + sizeof(msg_len);
>  	if (msg_len != 0)
>  		/* optional error msg, no need to check for failure */

This comment no longer applies. I'd also add braces:

	if (msg_len != 0) {
> -		str = strndup(msg, msg_len);
> +		if ((str = ibuf_get_string(pdu, msg_len)) == NULL)
> +			goto fail;
	}

>  
>  	log_warnx("rtr %s: received error: %s%s%s", log_rtr(rs),
>  	    log_rtr_error(errcode), str ? ": " : "", str ? str : "");
>  
>  	if (errcode == NO_DATA_AVAILABLE) {
>  		rtr_fsm(rs, RTR_EVNT_NO_DATA);
> -		rv = 0;

Getting rid of this rv dance makes the UNSUPP_PROTOCOL_VERS and
RESET_AND_CLOSE no longer return -1, so we won't return from
rtr_process_msg().

>  	} else if (errcode == UNSUPP_PROTOCOL_VERS)
>  		rtr_fsm(rs, RTR_EVNT_UNSUPP_PROTO_VERSION);
>  	else
> @@ -849,7 +855,13 @@ rtr_parse_error(struct rtr_session *rs, 
>  		    sizeof(rs->last_recv_msg));

I'd be tempted to unwrap the two strlcpy and memset calls.

>  
>  	free(str);
> -	return rv;
> +	return 0;
> +
> + fail:
> +	log_warnx("rtr %s: received %s: bad encoding", log_rtr(rs),
> +	    log_rtr_type(ERROR_REPORT));
> +	rtr_fsm(rs, RTR_EVNT_RESET_AND_CLOSE);
> +	return -1;
>  }
>  
>  /*
> @@ -860,61 +872,58 @@ rtr_parse_error(struct rtr_session *rs, 
>  static void
>  rtr_process_msg(struct rtr_session *rs)
>  {
> -	size_t rpos, av, left;
> -	void *rptr;
> +	struct ibuf rbuf, hdr, msg;
>  	size_t msglen;
>  	enum rtr_pdu_type msgtype;
>  
> -	rpos = 0;
> -	av = rs->r.wpos;
> +	ibuf_from_buffer(&rbuf, rs->r.buf, rs->r.wpos);
>  
>  	for (;;) {
> -		if (rpos + sizeof(struct rtr_header) > av)
> +		if (ibuf_size(&rbuf) < sizeof(struct rtr_header))
>  			break;
> -		rptr = rs->r.buf + rpos;
> -		if (rtr_parse_header(rs, rptr, &msglen, &msgtype) == -1)
> +
> +		/* parse header */
> +		ibuf_from_buffer(&hdr, ibuf_data(&rbuf),
> +		    sizeof(struct rtr_header));
> +		if (rtr_parse_header(rs, &hdr, &msglen, &msgtype) == -1)
>  			return;
>  
> -		/* missing data */
> -		if (rpos + msglen > av)
> +		/* extract message */
> +		if (ibuf_get_ibuf(&rbuf, msglen, &msg) == -1)
>  			break;
>  
>  		switch (msgtype) {
>  		case SERIAL_NOTIFY:
> -			if (rtr_parse_notify(rs, rptr, msglen) == -1) {
> -				rtr_send_error(rs, CORRUPT_DATA, NULL,
> -				    rptr, msglen);
> +			if (rtr_parse_notify(rs, &msg) == -1) {
> +				rtr_send_error(rs, CORRUPT_DATA, NULL, &msg);
>  				return;
>  			}
>  			break;
>  		case CACHE_RESPONSE:
> -			if (rtr_parse_cache_response(rs, rptr, msglen) == -1) {
> -				rtr_send_error(rs, CORRUPT_DATA, NULL,
> -				    rptr, msglen);
> +			if (rtr_parse_cache_response(rs, &msg) == -1) {
> +				rtr_send_error(rs, CORRUPT_DATA, NULL, &msg);
>  				return;
>  			}
>  			break;
>  		case IPV4_PREFIX:
> -			if (rtr_parse_ipv4_prefix(rs, rptr, msglen) == -1) {
> +			if (rtr_parse_ipv4_prefix(rs, &msg) == -1) {
>  				return;
>  			}
>  			break;
>  		case IPV6_PREFIX:
> -			if (rtr_parse_ipv6_prefix(rs, rptr, msglen) == -1) {
> +			if (rtr_parse_ipv6_prefix(rs, &msg) == -1) {
>  				return;
>  			}
>  			break;
>  		case END_OF_DATA:
> -			if (rtr_parse_end_of_data(rs, rptr, msglen) == -1) {
> -				rtr_send_error(rs, CORRUPT_DATA, NULL,
> -				    rptr, msglen);
> +			if (rtr_parse_end_of_data(rs, &msg) == -1) {
> +				rtr_send_error(rs, CORRUPT_DATA, NULL, &msg);
>  				return;
>  			}
>  			break;
>  		case CACHE_RESET:
> -			if (rtr_parse_cache_reset(rs, rptr, msglen) == -1) {
> -				rtr_send_error(rs, CORRUPT_DATA, NULL,
> -				    rptr, msglen);
> +			if (rtr_parse_cache_reset(rs, &msg) == -1) {
> +				rtr_send_error(rs, CORRUPT_DATA, NULL, &msg);
>  				return;
>  			}
>  			break;
> @@ -922,27 +931,26 @@ rtr_process_msg(struct rtr_session *rs)
>  			/* silently ignore router key */
>  			break;
>  		case ERROR_REPORT:
> -			if (rtr_parse_error(rs, rptr, msglen) == -1)
> +			if (rtr_parse_error(rs, &msg) == -1) {
>  				/* no need to send back an error */
>  				return;
> +			}
>  			break;
>  		case ASPA:
> -			if (rtr_parse_aspa(rs, rptr, msglen) == -1) {
> +			if (rtr_parse_aspa(rs, &msg) == -1) {
>  				return;
>  			}
>  			break;
>  		default:
>  			log_warnx("rtr %s: received %s: unexpected pdu type",
>  			    log_rtr(rs), log_rtr_type(msgtype));
> -			rtr_send_error(rs, INVALID_REQUEST, NULL, rptr, msglen);
> +			rtr_send_error(rs, INVALID_REQUEST, NULL, &msg);
>  			return;
>  		}
> -		rpos += msglen;
>  	}
>  
> -	left = av - rpos;
> -	memmove(&rs->r.buf, rs->r.buf + rpos, left);
> -	rs->r.wpos = left;
> +	memmove(&rs->r.buf, ibuf_data(&rbuf), ibuf_size(&rbuf));
> +	rs->r.wpos = ibuf_size(&rbuf);
>  }
>  
>  /*
> @@ -968,7 +976,7 @@ rtr_fsm(struct rtr_session *rs, enum rtr
>  				log_warnx("rtr %s: version negotiation failed",
>  				    log_rtr(rs));
>  				rtr_send_error(rs, UNSUPP_PROTOCOL_VERS,
> -				    NULL, NULL, 0);
> +				    NULL, NULL);
>  				return;
>  			}
>  
> @@ -991,11 +999,6 @@ rtr_fsm(struct rtr_session *rs, enum rtr
>  		rtr_recalc();
>  		/* FALLTHROUGH */
>  	case RTR_EVNT_CON_CLOSE:
> -		if (rs->state == RTR_STATE_NEGOTIATION) {
> -			/* consider any close event as a version failure. */
> -			rtr_fsm(rs, RTR_EVNT_UNSUPP_PROTO_VERSION);
> -			break;
> -		}
>  		if (rs->fd != -1) {
>  			/* flush buffers */
>  			msgbuf_clear(&rs->w);
>