Re: [Tarantool-patches] [PATCH] relay: fix use after free in subscribe_f

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Wed, May 12, 2021 at 02:39:07PM +0300, Serge Petrenko wrote:
> relay_subscribe_f() remembered old recovery pointer, which might be
> replaced by relay_restart_recovery() if a raft message is delivered during
> cbus_process() loop in relay_send_is_raft_enabled().
> 
> Fix the issue by moving variable initialization below
> relay_send_is_raft_enabled()
> 
> Closes #6031
> ---
> https://github.com/tarantool/tarantool/issues/6031
> https://github.com/tarantool/tarantool/tree/sp/gh-6031-use-after-free
> 
>  src/box/relay.cc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index ff43c2fc7..32d3a58dd 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -741,7 +741,6 @@ static int
>  relay_subscribe_f(va_list ap)
>  {
>  	struct relay *relay = va_arg(ap, struct relay *);
> -	struct recovery *r = relay->r;
>  
>  	coio_enable();
>  	relay_set_cord_name(relay->io.fd);
> @@ -756,6 +755,8 @@ relay_subscribe_f(va_list ap)
>  	if (!relay->replica->anon)
>  		relay_send_is_raft_enabled(relay, &raft_enabler, true);
>  
> +	struct recovery *r = relay->r;

Could you please add a comment why it is important to fetch `relay->r`
at exactly this stage. Something like

	/*
	 * Fetching relay->r should be done after
	 * cbus processing since the pointer may
	 * be updated undeneath.
	 */
	struct recovery *r = relay->r;

Or something like this. Because commits messages are good but we
read the code in first place and this very nontrivial moment.