Re: [Tarantool-patches] [PATCH 2/2] raft: decode even invalid states of raft

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 <gorcunov@gmail.com>
> ---
>  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.