From: Cyrill Gorcunov <gorcunov@gmail.com> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] raft: decode even invalid states of raft Date: Wed, 28 Oct 2020 01:36:30 +0300 [thread overview] Message-ID: <20201027223630.GC2093@grain> (raw) In-Reply-To: <02839650-d213-98d2-1cbf-7fb95d52102c@tarantool.org> 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.
next prev parent reply other threads:[~2020-10-27 22:36 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-14 14:26 [Tarantool-patches] [PATCH 0/2] raft: a few fixes Cyrill Gorcunov 2020-10-14 14:26 ` [Tarantool-patches] [PATCH 1/2] raft: raft_request_to_string -- don't hardcode size Cyrill Gorcunov 2020-10-14 14:26 ` [Tarantool-patches] [PATCH 2/2] raft: decode even invalid states of raft Cyrill Gorcunov 2020-10-15 9:43 ` Serge Petrenko 2020-10-15 9:46 ` Cyrill Gorcunov 2020-10-27 22:18 ` Vladislav Shpilevoy 2020-10-27 22:36 ` Cyrill Gorcunov [this message] 2020-10-15 8:44 ` [Tarantool-patches] [PATCH 0/2] raft: a few fixes Cyrill Gorcunov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201027223630.GC2093@grain \ --to=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/2] raft: decode even invalid states of raft' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox