[Tarantool-patches] [PATCH v4 3/4] replication: implement an instance id filter for relay
Konstantin Osipov
kostja.osipov at gmail.com
Thu Feb 27 09:48:03 MSK 2020
* Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [20/02/27 09:42]:
> > + unsigned int id_filter)
>
> 3. Nit - it would be better to have it as uint32_t explicitly.
> Becuase max id count is 32. Unsigned int does not have size
> guarantees, formally speaking. It is at least 16 bits, no more
> info.
We use unsigned int in struct vclock, I think vclock.h also needs
a follow up then.
> > + if (filter_size) {
>
> 5. I wouldn't make it optional in encoding, since this is sent
> really rare, but could simplify the code a bit. However, up to
> you.
>
> Also, won't it break replication from an old version? You
> will send subscribe with this new key, and the old instance
> should ignore it. Does it? I don't remember.
>
> If the old instance would ignore it, it means that the bug
> still can explode when replicating from an old version, right?
> I don't know how to fix that, but if it is true, we need to
> document that, at least.
This is true, however, it only happens at reconfiguration,
you're not supposed to actively use a heterogeneous cluster, only
upgrade it (bootstrap).
>
> > + *id_filter |= 1 << mp_decode_uint(&d);
>
> 7. If someone would send a big ID (a program, pretending to be a Tarantool
> instance), it would cause unsigned bit shift overflow, which is undefined
> behaviour. Lets check that it is not bigger than 31.
>
> However this won't really help much. This code will crash even if I will
> just send a truncated packet. From what I see.
>
> Up to you whether you want to fix the bit shift.
Good catch, this would be a CVE then and a huge publicity issue.
There should be no crashes when parsing malformed requests.
>
--
Konstantin Osipov, Moscow, Russia
More information about the Tarantool-patches
mailing list