[Tarantool-patches] [PATCH] relay: fix use after free in subscribe_f
Cyrill Gorcunov
gorcunov at gmail.com
Wed May 12 14:48:38 MSK 2021
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.
More information about the Tarantool-patches
mailing list