Re: [Tarantool-patches] [PATCH v4 3/4] replication: implement an instance id filter for relay

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Vladislav Shpilevoy <v.shpilevoy@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