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

[Serge Petrenko <sergepetrenko@tarantool.org>]

14.10.2020 17:26, Cyrill Gorcunov пишет:
> We should never trust the request data it can
> carry anything, thus lets make sure we're not
> decoding some trash into a string.

Hi! Thanks for the patch!

Both fixes look good to me. Let's wait for what Vlad has to say, though,
since he's the author of this code.

Please find one suggestion below.

>
> 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));
>   	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)
> +{
diff --git a/src/box/raft.c b/src/box/raft.c
index 44a76154f..d57d3ce67 100644
--- a/src/box/raft.c
+++ b/src/box/raft.c
@@ -63,10 +63,7 @@ struct raft raft = {
         .election_timeout = 5,
  };

-/**
- * When decoding we should never trust that there
- * is a valid data incomes.
- */
+/** When decoding we should never trust that the input is valid. */
  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));
>   		assert(rc >= 0);
>   		pos += rc;
>   		size -= rc;
> diff --git a/src/box/raft.h b/src/box/raft.h
> index be77a5473..3c007789b 100644
> --- a/src/box/raft.h
> +++ b/src/box/raft.h
> @@ -70,6 +70,11 @@ struct raft_request;
>   struct vclock;
>   
>   enum raft_state {
> +	/**
> +	 * Unknown state, never valid, for sequence completeness
> +	 * and to print out wrong state from incoming packets.
> +	 */
> +	RAFT_STATE_NONE = 0,
>   	/**
>   	 * Can't write. Can only accept data from a leader. Node in this state
>   	 * either monitors an existing leader, or there is an on-going election
> @@ -86,7 +91,11 @@ enum raft_state {
>   	RAFT_STATE_LEADER = 3,
>   };
>   
> -extern const char *raft_state_strs[];
> +/**
> + * Decode raft state into string representation.
> + */
> +const char *
> +raft_state_str(uint32_t state);
>   
>   struct raft {
>   	/** Instance ID of leader of the current term. */

-- 
Serge Petrenko