From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 A669B45C304 for ; Wed, 16 Dec 2020 16:07:01 +0300 (MSK) References: <3ff242ba644867ee41dcb6efcf7210153b2d43a4.1607879643.git.v.shpilevoy@tarantool.org> From: Serge Petrenko Message-ID: Date: Wed, 16 Dec 2020 16:06:59 +0300 MIME-Version: 1.0 In-Reply-To: <3ff242ba644867ee41dcb6efcf7210153b2d43a4.1607879643.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 6/8] raft: fix ignorance of bad state receipt List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org 13.12.2020 20:15, Vladislav Shpilevoy пишет: > raft_process_msg() only validated that the state is specified. But > it didn't check if the state is inside of the allowed value range. > > Such messages were considered valid, and even their other fields > were accepted. For instance, an invalid message could bump term. > > It is safer to reject such messages. > > Part of #5303 > --- LGTM. > src/lib/raft/raft.c | 4 ++-- > src/lib/raft/raft.h | 1 + > test/unit/raft.c | 10 +++++++++- > test/unit/raft.result | 4 +++- > 4 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c > index df34d81dc..ab007a462 100644 > --- a/src/lib/raft/raft.c > +++ b/src/lib/raft/raft.c > @@ -309,8 +309,8 @@ raft_process_msg(struct raft *raft, const struct raft_msg *req, uint32_t source) > say_info("RAFT: message %s from %u", raft_msg_to_string(req), source); > assert(source > 0); > assert(source != raft->self); > - if (req->term == 0 || req->state == 0) { > - diag_set(RaftError, "Raft term and state can't be zero"); > + if (req->term == 0 || req->state == 0 || req->state >= raft_state_MAX) { > + diag_set(RaftError, "Invalid term or state"); > return -1; > } > if (req->state == RAFT_STATE_CANDIDATE && > diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h > index 7a3db79c1..e447f6634 100644 > --- a/src/lib/raft/raft.h > +++ b/src/lib/raft/raft.h > @@ -85,6 +85,7 @@ enum raft_state { > RAFT_STATE_CANDIDATE = 2, > /** Election was successful. The node accepts write requests. */ > RAFT_STATE_LEADER = 3, > + raft_state_MAX, > }; > > /** > diff --git a/test/unit/raft.c b/test/unit/raft.c > index 1ed8b7af7..b97d9d0aa 100644 > --- a/test/unit/raft.c > +++ b/test/unit/raft.c > @@ -247,7 +247,7 @@ raft_test_recovery(void) > static void > raft_test_bad_msg(void) > { > - raft_start_test(7); > + raft_start_test(9); > struct raft_msg msg; > struct raft_node node; > struct vclock vclock; > @@ -286,6 +286,14 @@ raft_test_bad_msg(void) > }; > is(raft_node_process_msg(&node, &msg, 2), -1, "term can't be 0"); > > + msg = (struct raft_msg){ > + .state = 10000, > + .term = 10, > + .vote = 2, > + }; > + is(raft_node_process_msg(&node, &msg, 2), -1, "bad state"); > + is(node.raft.term, 1, "term from the bad message wasn't used"); > + > raft_node_destroy(&node); > raft_finish_test(); > } > diff --git a/test/unit/raft.result b/test/unit/raft.result > index 2398bd71c..3fa2682c8 100644 > --- a/test/unit/raft.result > +++ b/test/unit/raft.result > @@ -45,7 +45,7 @@ ok 1 - subtests > ok 2 - subtests > *** raft_test_recovery: done *** > *** raft_test_bad_msg *** > - 1..7 > + 1..9 > ok 1 - state can't be 0 > ok 2 - term from the bad message wasn't used > ok 3 - node can't be a candidate but vote for another node > @@ -53,6 +53,8 @@ ok 2 - subtests > ok 5 - node can't be a candidate without vclock > ok 6 - term from the bad message wasn't used > ok 7 - term can't be 0 > + ok 8 - bad state > + ok 9 - term from the bad message wasn't used > ok 3 - subtests > *** raft_test_bad_msg: done *** > *** raft_test_vote *** -- Serge Petrenko