Re: [Tarantool-patches] [PATCH v2] relay: fix segfault on replica transition from anonymous

[Serge Petrenko <sergepetrenko@tarantool.org>]

Sergey, would you kindly check this patch with your reproducer?
I failed to reproduce the issue on my side.

--
Serge Petrenko
sergepetrenko@tarantool.org




> 13 апр. 2020 г., в 13:34, Serge Petrenko <sergepetrenko@tarantool.org> написал(а):
> 
> relay_subscribe_f sets a recovery trigger notifying tx when a full log
> is read and gc consumer corresponding to the replica may be advanced.
> Since anonymous replicas do not have gc consumers, the trigger isn't
> added for them. However, on relay exit, the trigger deletion depends
> on replica->anon flag. This is buggy in case relay stalls on exit due to
> replica disconnect. Replica has time to reconnect and register as a
> normal instance, hence its replica->anon flag will be false by the time
> we check whether to clear triggers or not, effectively making us to
> clear unset triggers and segfault.
> 
> Fix this by initializing the triggers with trigger_create(), which
> allows a trigger_clear() call, even if the triggers are not set, and
> omit the replica->anon check.
> 
> Closes #4731
> 
> Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> https://github.com/tarantool/tarantool/tree/sp/gh-4731-anon-segfault-v2
> https://github.com/tarantool/tarantool/issues/4731
> 
> src/box/relay.cc | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index c634348a4..5f34753fc 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -580,9 +580,8 @@ relay_subscribe_f(va_list ap)
> 	 * Not needed for anonymous replicas, since they
> 	 * aren't registered with gc at all.
> 	 */
> -	struct trigger on_close_log = {
> -		RLIST_LINK_INITIALIZER, relay_on_close_log_f, relay, NULL
> -	};
> +	struct trigger on_close_log;
> +	trigger_create(&on_close_log, relay_on_close_log_f, relay, NULL);
> 	if (!relay->replica->anon)
> 		trigger_add(&r->on_close_log, &on_close_log);
> 
> @@ -662,9 +661,12 @@ relay_subscribe_f(va_list ap)
> 	diag_log();
> 	say_crit("exiting the relay loop");
> 
> -	/* Clear garbage collector trigger and WAL watcher. */
> -	if (!relay->replica->anon)
> -		trigger_clear(&on_close_log);
> +	/*
> +	 * Clear garbage collector trigger and WAL watcher.
> +	 * trigger_clear() does nothing in case the triggers
> +	 * aren't set (the replica is anonymous).
> +	 */
> +	trigger_clear(&on_close_log);
> 	wal_clear_watcher(&relay->wal_watcher, cbus_process);
> 
> 	/* Join ack reader fiber. */
> -- 
> 2.21.1 (Apple Git-122.3)
>