[Tarantool-patches] [PATCH 0/2] raft: a few fixes

[Tarantool-patches] [PATCH 0/2] raft: a few fixes

[Cyrill Gorcunov]

Vlad, Sergey, here are a few nitpicks. Feel free to
ignore just came to mind while been reading the code.

The issue with potential nil dereference for raft worker
should be addressed separately on top of the Vlad fixes.

Cyrill Gorcunov (2):
  raft: raft_request_to_string -- don't hardcode size
  raft: decode even invalid states of raft

 src/box/lua/info.c |  2 +-
 src/box/raft.c     | 31 ++++++++++++++++++++++---------
 src/box/raft.h     | 11 ++++++++++-
 3 files changed, 33 insertions(+), 11 deletions(-)

-- 
2.26.2

[Tarantool-patches] [PATCH 1/2] raft: raft_request_to_string -- don't hardcode size

[Cyrill Gorcunov]

The size should be matched to the real size of a buffer,
otherwise it is a room for mistake.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/raft.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/raft.c b/src/box/raft.c
index 0b6c373e8..9de77b0ec 100644
--- a/src/box/raft.c
+++ b/src/box/raft.c
@@ -268,8 +268,8 @@ static const char *
 raft_request_to_string(const struct raft_request *req)
 {
 	assert(req->term != 0);
-	int size = 1024;
 	char buf[1024];
+	int size = sizeof(buf);
 	char *pos = buf;
 	int rc = snprintf(pos, size, "{term: %llu",
 			  (unsigned long long)req->term);
-- 
2.26.2

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

[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.

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)
+{
+	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. */
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 0/2] raft: a few fixes

[Cyrill Gorcunov]

On Wed, Oct 14, 2020 at 05:26:15PM +0300, Cyrill Gorcunov wrote:
> Vlad, Sergey, here are a few nitpicks. Feel free to
> ignore just came to mind while been reading the code.
> 
> The issue with potential nil dereference for raft worker
> should be addressed separately on top of the Vlad fixes.

Forgot to mention it is gorcunov/raft-fixes branch

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

[Serge Petrenko]

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

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

[Cyrill Gorcunov]

On Thu, Oct 15, 2020 at 12:43:38PM +0300, Serge Petrenko wrote:
> 
> -/**
> - * When decoding we should never trust that there
> - * is a valid data incomes.
> - */
> +/** When decoding we should never trust that the input is valid. */

Sure, will update if Vlad is agree in general.

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

[Vladislav Shpilevoy]

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.

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

[Cyrill Gorcunov]

On Tue, Oct 27, 2020 at 11:18:18PM +0100, Vladislav Shpilevoy wrote:
> >  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.

You miss the main point -- the state string decoder *must* accept
any value instead of relaying on some other code which should filter
bad states. I even put a comment in the code, letme repeat

 enum raft_state {
+	/**
+	 * Unknown state, never valid, for sequence completeness
+	 * and to print out wrong state from incoming packets.
+	 */
+	RAFT_STATE_NONE = 0,

> >  /**
> >   * 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.

And how it is related to the request to make raft state decode safe? Seriously
I don't get it -- the raft state string was a global variable now it is a function
which implies it simply must be ready for *any* bloody state, even invalid ones.

> 
> 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.

No, either provide a decode function which can take any argument or make it
static and never export to another tarantool code. I took the first approach.

Anyway, this is your code so I won't insist, up to you.