From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 843434696C3 for ; Mon, 13 Apr 2020 16:25:08 +0300 (MSK) Date: Mon, 13 Apr 2020 16:24:16 +0300 From: Sergey Bronnikov Message-ID: <20200413132416.GB27992@pony.bronevichok.ru> References: <20200413103454.82207-1-sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2] relay: fix segfault on replica transition from anonymous List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko Cc: tml 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 написал(а): > > > > 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 > > --- > > 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@