On 2023/05/25 15:06, Claudio Jeker wrote:
> sthen@ reported a bgpd SE crash to me and after inspection of the report
> it looks like he managed to trigger a mistake in session_process_msg().
> When for example a NOTIFICATION message is received then the state change
> clears the rbuf. Now normally the for loop starts over afterwards and the
> if (p->rbuf == NULL) at the top causes the function to return.
> In his case the peer had enough messages queued that the if (++processed >
> MSG_PROCESS_LIMIT) limit triggered after the NOTIFICATION was processed.
> This hits a break from the for loop and the code at the end of the
> function adjusting the rbuf trips over the NULL rbuf pointer.
>
> This can be fixed in many ways, I decided to just check at the end of the
> loop that rbuf is still valid.
Thanks for looking into this. OK.
> Triggering this bug is not trivial so it is hard test but the problem is
> obvious.
indeed, I don't think I have hit this one at all before.
Running sessions over routes maintained by ospf6d seems a fairly
good way to trigger a number of issues ;)
> :wq Claudio
>
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.444
> diff -u -p -r1.444 session.c
> --- session.c 5 May 2023 07:28:08 -0000 1.444
> +++ session.c 25 May 2023 09:32:11 -0000
> @@ -1998,6 +1998,8 @@ session_process_msg(struct peer *p)
> }
> }
>
> + if (p->rbuf == NULL)
> + return;
> if (rpos < av) {
> left = av - rpos;
> memmove(&p->rbuf->buf, p->rbuf->buf + rpos, left);
>
