From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 98DF5469719 for ; Wed, 28 Oct 2020 01:18:20 +0300 (MSK) References: <20201014142617.235813-1-gorcunov@gmail.com> <20201014142617.235813-3-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: <02839650-d213-98d2-1cbf-7fb95d52102c@tarantool.org> Date: Tue, 27 Oct 2020 23:18:18 +0100 MIME-Version: 1.0 In-Reply-To: <20201014142617.235813-3-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Cyrill Gorcunov , Serge Petrenko Cc: tml Hi! Thanks for the patch! See 2 comments below. On 14.10.2020 16:26, Cyrill Gorcunov wrote: > We should never trust the request data it can > carry anything, thus lets make sure we're not > decoding some trash into a string. > > Signed-off-by: Cyrill Gorcunov > --- > src/box/lua/info.c | 2 +- > src/box/raft.c | 29 +++++++++++++++++++++-------- > src/box/raft.h | 11 ++++++++++- > 3 files changed, 32 insertions(+), 10 deletions(-) > > diff --git a/src/box/lua/info.c b/src/box/lua/info.c > index cac3fd475..92d48c96c 100644 > --- a/src/box/lua/info.c > +++ b/src/box/lua/info.c > @@ -582,7 +582,7 @@ static int > 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. > lua_setfield(L, -2, "state"); > luaL_pushuint64(L, raft.volatile_term); > lua_setfield(L, -2, "term"); > diff --git a/src/box/raft.c b/src/box/raft.c > index 9de77b0ec..44a76154f 100644 > --- a/src/box/raft.c > +++ b/src/box/raft.c > @@ -44,13 +44,6 @@ > */ > #define RAFT_RANDOM_ELECTION_FACTOR 0.1 > > -const char *raft_state_strs[] = { > - NULL, > - "follower", > - "candidate", > - "leader", > -}; > - > /** Raft state of this instance. */ > struct raft raft = { > .leader = 0, > @@ -70,6 +63,26 @@ struct raft raft = { > .election_timeout = 5, > }; > > +/** > + * When decoding we should never trust that there > + * is a valid data incomes. > + */ > +const char * > +raft_state_str(uint32_t state) > +{ > + static const char *str[] = { > + [RAFT_STATE_NONE] = "none", > + [RAFT_STATE_FOLLOWER] = "follower", > + [RAFT_STATE_CANDIDATE] = "candidate", > + [RAFT_STATE_LEADER] = "leader", > + }; > + > + if (state < lengthof(str)) > + return str[state]; > + > + return "unknown"; > +}; > + > /** > * 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. 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.