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

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

12.05.2021 23:25, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
>> 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
>> @@ -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;
>> +
> There is another cbus_process() on line 808. Won't it lead to the same issue
> if recovery would be restarted? I see it is for version < 1.7.4 so probably
> not. Another option would be to simply inline relay->r in its usage places
> and not remember it into a variable.
>
> Anyway LGTM. Up to you if want to inline.

Thanks for the review!

Yep, let's inline this.

I've also added a changelog entry.

The patch's changed a lot so here's the whole new version.

===========================================

Author: Serge Petrenko <sergepetrenko@tarantool.org>
Date:   Wed May 12 12:58:34 2021 +0300

     relay: fix use after free in subscribe_f

     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 removing the alias altogether and referencing relay->r
     directly to prevent any further mistakes.

     Closes #6031

diff --git a/changelogs/unreleased/gh-6031-relay-use-after-free.md 
b/changelogs/unreleased/gh-6031-relay-use-after-free.md
new file mode 100644
index 000000000..910b29614
--- /dev/null
+++ b/changelogs/unreleased/gh-6031-relay-use-after-free.md
@@ -0,0 +1,3 @@
+## bugfix/replication
+
+* Fix use after free in relay thread when using elections (gh-6031).
diff --git a/src/box/relay.cc b/src/box/relay.cc
index ff43c2fc7..5a21a4499 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);
@@ -764,7 +763,7 @@ relay_subscribe_f(va_list ap)
         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);
+               trigger_add(&relay->r->on_close_log, &on_close_log);

         /* Setup WAL watcher for sending new rows to the replica. */
         wal_set_watcher(&relay->wal_watcher, relay->endpoint.name,
@@ -816,7 +815,7 @@ relay_subscribe_f(va_list ap)
                         continue;
                 struct vclock *send_vclock;
                 if (relay->version_id < version_id(1, 7, 4))
-                       send_vclock = &r->vclock;
+                       send_vclock = &relay->r->vclock;
                 else
                         send_vclock = &relay->recv_vclock;

===========================================

-- 
Serge Petrenko