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

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

[Serge Petrenko]

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)

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

[Serge Petrenko]

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)
> 

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

[Sergey Bronnikov]

Hi,

thanks for the fix!

On 13:37 Mon 13 Apr , Serge Petrenko wrote:
> Sergey, would you kindly check this patch with your reproducer?
> I failed to reproduce the issue on my side.

reproducer passed 200 times out of 200 runs.

> --
> 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)
> > 
> 

-- 
sergeyb@

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

[Kirill Yukhin]

Hello,

On 13 апр 13:34, Serge Petrenko wrote:
> 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

I've checked your patch into 2.3 and master.

--
Regards, Kirill Yukhin