[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: bgpd: cleanup optparamlen handling in session_open
From:       Theo Buehler <tb () theobuehler ! org>
Date:       2023-10-27 11:51:54
Message-ID: ZTukWoniO3fGqdaW () theobuehler ! org
[Download RAW message or body]

On Fri, Oct 27, 2023 at 01:06:31PM +0200, Claudio Jeker wrote:
> In the big ibuf API refactor I also broke the optparamlen handling
> by using one variable for two things.
> 
> All the size handling in session_open() can be simplified since
> ibuf_size() is cheap to call.
> 
> I think the result is cleaner than the code before. It is still somewhat
> funky because there are a fair amount of conditions to cover now.

There's no way to avoid catching some funk if the spec has a groovy
legacy heritage...

Diff makes sense and matches my understanding of that RFC's hack.

ok

> 
> -- 
> :wq Claudio
> 
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.452
> diff -u -p -r1.452 session.c
> --- session.c	27 Oct 2023 09:40:27 -0000	1.452
> +++ session.c	27 Oct 2023 11:00:13 -0000
> @@ -1471,8 +1471,9 @@ session_open(struct peer *p)
>  {
>  	struct bgp_msg		*buf;
>  	struct ibuf		*opb;
> -	uint16_t		 len, optparamlen = 0, holdtime;
> -	uint8_t			 i, op_type;
> +	size_t			 optparamlen;
> +	uint16_t		 holdtime;
> +	uint8_t			 i;
>  	int			 errs = 0, extlen = 0;
>  	int			 mpcapa = 0;
>  
> @@ -1556,16 +1557,16 @@ session_open(struct peer *p)
>  	if (optparamlen == 0) {
>  		/* nothing */
>  	} else if (optparamlen + 2 >= 255) {
> -		/* RFC9072: 2 byte length instead of 1 + 3 byte extra header */
> -		optparamlen += sizeof(op_type) + 2 + 3;
> +		/* RFC9072: use 255 as magic size and request extra header */
>  		optparamlen = 255;
>  		extlen = 1;
>  	} else {
> -		optparamlen += sizeof(op_type) + 1;
> +		/* regular capabilities header */
> +		optparamlen += 2;
>  	}
>  
> -	len = MSGSIZE_OPEN_MIN + optparamlen;
> -	if (errs || (buf = session_newmsg(OPEN, len)) == NULL) {
> +	if (errs || (buf = session_newmsg(OPEN,
> +	    MSGSIZE_OPEN_MIN + optparamlen)) == NULL) {
>  		ibuf_free(opb);
>  		bgp_fsm(p, EVNT_CON_FATAL);
>  		return;
> @@ -1584,20 +1585,19 @@ session_open(struct peer *p)
>  	errs += ibuf_add_n8(buf->buf, optparamlen);
>  
>  	if (extlen) {
> -		/* write RFC9072 extra header */
> +		/* RFC9072 extra header which spans over the capabilities hdr */
>  		errs += ibuf_add_n8(buf->buf, OPT_PARAM_EXT_LEN);
> -		errs += ibuf_add_n16(buf->buf, optparamlen - 3);
> +		errs += ibuf_add_n16(buf->buf, ibuf_size(opb) + 1 + 2);
>  	}
>  
>  	if (optparamlen) {
>  		errs += ibuf_add_n8(buf->buf, OPT_PARAM_CAPABILITIES);
>  
> -		optparamlen = ibuf_size(opb);
>  		if (extlen) {
>  			/* RFC9072: 2-byte extended length */
> -			errs += ibuf_add_n16(buf->buf, optparamlen);
> +			errs += ibuf_add_n16(buf->buf, ibuf_size(opb));
>  		} else {
> -			errs += ibuf_add_n8(buf->buf, optparamlen);
> +			errs += ibuf_add_n8(buf->buf, ibuf_size(opb));
>  		}
>  		errs += ibuf_add_buf(buf->buf, opb);
>  	}
> 

[prev in list] [next in list] [prev in thread] [next in thread]