Re: [Tarantool-patches] [PATCH 6/8] raft: fix ignorance of bad state receipt

[Serge Petrenko <sergepetrenko@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