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

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

[Serge Petrenko via Tarantool-patches]

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;
+
 	/*
 	 * Setup garbage collection trigger.
 	 * Not needed for anonymous replicas, since they
-- 
2.30.1 (Apple Git-130)

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

[Cyrill Gorcunov via Tarantool-patches]

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.

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

[Vladislav Shpilevoy via Tarantool-patches]

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.

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

[Serge Petrenko via Tarantool-patches]

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

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

[Serge Petrenko via Tarantool-patches]

12.05.2021 14:48, Cyrill Gorcunov пишет:
> 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.

Hi! Thanks for the review.
Vlad suggested to inline relay->r. It has only 2 usage places after all.

I agree this was nontrivial. It's better with the inline, I think.

-- 
Serge Petrenko

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

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the fixes!

LGTM.

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

[Cyrill Gorcunov via Tarantool-patches]

On Thu, May 13, 2021 at 01:34:55PM +0300, Serge Petrenko wrote:
> 
> Author: Serge Petrenko <sergepetrenko@tarantool.org>
> Date:   Wed May 12 12:58:34 2021 +0300
> 
>     relay: fix use after free in subscribe_f
> 

Ack

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

[Kirill Yukhin via Tarantool-patches]

Hello,

On 12 май 14:39, Serge Petrenko via Tarantool-patches 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

I've checked your patch into 2.8 and master.

--
Regards, Kirill Yukhin