From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com [209.85.167.68]) (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 3C233469719 for ; Wed, 28 Oct 2020 01:36:33 +0300 (MSK) Received: by mail-lf1-f68.google.com with SMTP id a9so4392952lfc.7 for ; Tue, 27 Oct 2020 15:36:33 -0700 (PDT) Date: Wed, 28 Oct 2020 01:36:30 +0300 From: Cyrill Gorcunov Message-ID: <20201027223630.GC2093@grain> References: <20201014142617.235813-1-gorcunov@gmail.com> <20201014142617.235813-3-gorcunov@gmail.com> <02839650-d213-98d2-1cbf-7fb95d52102c@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <02839650-d213-98d2-1cbf-7fb95d52102c@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] raft: decode even invalid states of raft List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml On Tue, Oct 27, 2020 at 11:18:18PM +0100, Vladislav Shpilevoy wrote: > > lbox_info_election(struct lua_State *L) > > { > > lua_createtable(L, 0, 4); > > - lua_pushstring(L, raft_state_strs[raft.state]); > > + lua_pushstring(L, raft_state_str(raft.state)); > > 1. Raft.state is always valid. It can't be NONE. If a request with a > bad state is received, it just should be rejected with a protocol > error. You miss the main point -- the state string decoder *must* accept any value instead of relaying on some other code which should filter bad states. I even put a comment in the code, letme repeat enum raft_state { + /** + * Unknown state, never valid, for sequence completeness + * and to print out wrong state from incoming packets. + */ + RAFT_STATE_NONE = 0, > > /** > > * Check if Raft is completely synced with disk. Meaning all its critical values > > * are in WAL. Only in that state the node can become a leader or a candidate. > > @@ -284,7 +297,7 @@ raft_request_to_string(const struct raft_request *req) > > } > > if (req->state != 0) { > > rc = snprintf(pos, size, ", state: %s", > > - raft_state_strs[req->state]); > > + raft_state_str(req->state)); > > 2. By doing so you didn't fix the actual issue - the state is still applied in > raft_process_msg(). Moreover, in debug build it will crash, because there is > a switch-case by the received state, which does panic() on an unknown state. And how it is related to the request to make raft state decode safe? Seriously I don't get it -- the raft state string was a global variable now it is a function which implies it simply must be ready for *any* bloody state, even invalid ones. > > The invalid state should be filtered out in the beginning of raft_process_msg(), > like all the other fields checked there for sanity. State is checked here, but > only for not being 0. Need to check for the max value too, that is it. No, either provide a decode function which can take any argument or make it static and never export to another tarantool code. I took the first approach. Anyway, this is your code so I won't insist, up to you.