From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 A385645C30B for ; Sun, 13 Dec 2020 20:15:42 +0300 (MSK) From: Vladislav Shpilevoy Date: Sun, 13 Dec 2020 18:15:28 +0100 Message-Id: <3ff242ba644867ee41dcb6efcf7210153b2d43a4.1607879643.git.v.shpilevoy@tarantool.org> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [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: tarantool-patches@dev.tarantool.org, sergepetrenko@tarantool.org 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 --- 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 *** -- 2.24.3 (Apple Git-128)