From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B2738469719 for ; Thu, 27 Feb 2020 09:48:05 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id r14so1218690lfm.5 for ; Wed, 26 Feb 2020 22:48:05 -0800 (PST) Date: Thu, 27 Feb 2020 09:48:03 +0300 From: Konstantin Osipov Message-ID: <20200227064803.GD29715@atlas> References: <172faa01-c31c-76e6-bb45-066f44ffc73d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <172faa01-c31c-76e6-bb45-066f44ffc73d@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 3/4] replication: implement an instance id filter for relay List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: kirichenkoga@gmail.com, tarantool-patches@dev.tarantool.org * Vladislav Shpilevoy [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