[Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches]

When new raft message comes in from the network we need
to be sure that the payload is suitable for processing,
in particular `raft_msg::state` must be valid because
our code logic depends on it.

Since the `raft_msg::state` is declared as enum the
its processing is implementation defined an a compiler
might treat it as unsigned or signed int. In the latter
case the `if` statement won't be taken which will lead
to undefined behaviour. So

1) Use explicit unsigned type conversion to make sure
   the `state` requested is valid;
2) Use panic() instead of unreacheable() macro;
3) Extend testing.

Closes #6067

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue https://github.com/tarantool/tarantool/issues/6067
branch gorcunov/gh-6067-raft-state-verify

 src/lib/raft/raft.c   |  7 +++++--
 test/unit/raft.c      | 10 +++++++++-
 test/unit/raft.result |  4 +++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
index eacdddb7e..409e983f0 100644
--- a/src/lib/raft/raft.c
+++ b/src/lib/raft/raft.c
@@ -309,7 +309,9 @@ 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 || req->state >= raft_state_MAX) {
+
+	if (req->term == 0 || req->state == 0 ||
+	    (unsigned)req->state >= raft_state_MAX) {
 		diag_set(RaftError, "Invalid term or state");
 		return -1;
 	}
@@ -406,7 +408,8 @@ raft_process_msg(struct raft *raft, const struct raft_msg *req, uint32_t source)
 			raft_sm_become_leader(raft);
 			break;
 		default:
-			unreachable();
+			panic("RAFT: unreacheable state hit");
+			break;
 		}
 	}
 	if (req->state != RAFT_STATE_LEADER) {
diff --git a/test/unit/raft.c b/test/unit/raft.c
index 6369c42d3..b9bc49b78 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(9);
+	raft_start_test(11);
 	struct raft_msg msg;
 	struct raft_node node;
 	struct vclock vclock;
@@ -294,6 +294,14 @@ raft_test_bad_msg(void)
 	is(raft_node_process_msg(&node, &msg, 2), -1, "bad state");
 	is(node.raft.term, 1, "term from the bad message wasn't used");
 
+	msg = (struct raft_msg){
+		.state = -1,
+		.term = 10,
+		.vote = 2,
+	};
+	is(raft_node_process_msg(&node, &msg, 2), -1, "bad negative 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 598a7e760..f89cd1658 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..9
+    1..11
     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
@@ -55,6 +55,8 @@ ok 2 - subtests
     ok 7 - term can't be 0
     ok 8 - bad state
     ok 9 - term from the bad message wasn't used
+    ok 10 - bad negative state
+    ok 11 - term from the bad message wasn't used
 ok 3 - subtests
 	*** raft_test_bad_msg: done ***
 	*** raft_test_vote ***
-- 
2.31.1

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

On 25.06.2021 12:07, Cyrill Gorcunov via Tarantool-patches wrote:
> When new raft message comes in from the network we need
> to be sure that the payload is suitable for processing,
> in particular `raft_msg::state` must be valid because
> our code logic depends on it.
> 
> Since the `raft_msg::state` is declared as enum the
> its processing is implementation defined an a compiler
> might treat it as unsigned or signed int. In the latter
> case the `if` statement won't be taken which will lead
> to undefined behaviour. So
> 
> 1) Use explicit unsigned type conversion to make sure
>    the `state` requested is valid;
> 2) Use panic() instead of unreacheable() macro;
> 3) Extend testing.
> 
> Closes #6067

Unfortunately, the patch does not fix much. Firstly,
the state is decoded from MessagePack as mp_decode_uint().
It can't be negative originally by design.

Secondly, and most importantly, the state is decoded as
uint64_t raft_request.state, but it is saved without checks
into enum struct raft_msg.state in box_raft_request_to_msg().

So the current patch does not change almost anything I
suppose? Because if I send ((UINT32_MAX << 31) | 0x01), it
will work. Because the upper 32 bits would be truncated
(assuming the enum is 32 bits), and your checks are done too
late to notice it.

Correct? If so, then I would propose to fix the issue entirely,
if you want to work on that.

> diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
> index eacdddb7e..409e983f0 100644
> --- a/src/lib/raft/raft.c
> +++ b/src/lib/raft/raft.c
> @@ -309,7 +309,9 @@ 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 || req->state >= raft_state_MAX) {
> +
> +	if (req->term == 0 || req->state == 0 ||
> +	    (unsigned)req->state >= raft_state_MAX) {

Please, perform a normal < 0 comparison. No need for unsigned cast
cast.

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Vladislav Shpilevoy via Tarantool-patches]

> Please, perform a normal < 0 comparison. No need for unsigned cast
> cast.

'cast cast' -> 'cast hack', sorry.

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Jun 25, 2021 at 11:49:10PM +0200, Vladislav Shpilevoy wrote:
> > Closes #6067
> 
> Unfortunately, the patch does not fix much. Firstly,
> the state is decoded from MessagePack as mp_decode_uint().
> It can't be negative originally by design.

No, Vlad, this is not how signed\unsigned numbers are processed.
The decoder doesn't care about two complements at all, we just
force it to read 8 bytes as unsigned long integer and that's all.
And it is up to _us_ as programmers how to treat this 8 bytes
next: either we continue using it as an unsigned integer and
generated instructions won't be the ones which consiged "sign flag",
or we treat it as signed integer in the code so the sign flag
will be either "used" in instructions results or extended up
to register size. IOW, if we decode a number as unsigned integer
it doesn't mean it can't be used as a signed one later.

> Secondly, and most importantly, the state is decoded as
> uint64_t raft_request.state, but it is saved without checks
> into enum struct raft_msg.state in box_raft_request_to_msg().

I suppose this is done because box_raft_request_to_msg a kind of
general wrapper which doesn't care much about contents validation
but simply represent the data in a new structure? When stage
of processing starts (ie raft_process_msg helper) we validate
each member, so I think this is a good architecture to investigate
contents in raft_process_msg, but if you prefer to use
box_raft_request_to_msg instead I would make it so.

> So the current patch does not change almost anything I
> suppose? Because if I send ((UINT32_MAX << 31) | 0x01), it
> will work. Because the upper 32 bits would be truncated
> (assuming the enum is 32 bits), and your checks are done too
> late to notice it.
> 
> Correct? If so, then I would propose to fix the issue entirely,
> if you want to work on that.

I think I didnt put more technical details in commit message,
let me try to explain in a more verbose way: the enum is stored
as integer type but standart doesn't require it to be signed
or unsigned, so it is implementation defined. Currently on my
gcc-11 series the if statement consider @state member as
_unsigned_ integer thus basically my patch does absolutely
nothing, the code remains the same on asm level. Still a compiler
has all rights to use a signed "state" and in this case our
`if` won't hit leading to undefined behaviour in case if the
"state" is in form (0x80000000 | x) and what is worse we won't
even able to figure out what has happened.

We can emulate this behaviour by using

raft_process_msg
	...
	if ((int)req->state < 0) {
		panic("RAFT: req->state > 0 -> %d", (int)req->state >= (int)raft_state_MAX);
		assert(req->state > 0);
	}

in result if we pass -1u as state in the test we will have
[001] +2021-06-26 15:52:58.610 [57492] main/101/main F> RAFT: req->state > 0 -> 0

thus if compiler use signed int type as internal representation then
(int)req->state >= (int)raft_state_MAX won't take the branch because
it is a negative value.

Surely the patch won't catch integer overflows since we trim the
data as you noted above because, gimme some time to think about,
I really like how our box_raft_request_to_msg helpers are done
and if it possible would love to not change them much: they are
simple and short now.

> > diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
> > index eacdddb7e..409e983f0 100644
> > --- a/src/lib/raft/raft.c
> > +++ b/src/lib/raft/raft.c
> > @@ -309,7 +309,9 @@ 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 || req->state >= raft_state_MAX) {
> > +
> > +	if (req->term == 0 || req->state == 0 ||
> > +	    (unsigned)req->state >= raft_state_MAX) {
> 
> Please, perform a normal < 0 comparison. No need for unsigned cast
> cast.

We need an explicit cast here because as I said the internal
implementation is compiler defined.

On the other hands I played with a number of gcc and llvm compilers
and for this code they all does correct comparision (as unsigned
integers) so maybe we could just leave it as is, simply close
the bug as won't implement and spend time on more important things?
I don't mind actually, so up to you just say me what you think.

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Vladislav Shpilevoy via Tarantool-patches]

On 26.06.2021 15:34, Cyrill Gorcunov wrote:
> On Fri, Jun 25, 2021 at 11:49:10PM +0200, Vladislav Shpilevoy wrote:
>>> Closes #6067
>>
>> Unfortunately, the patch does not fix much. Firstly,
>> the state is decoded from MessagePack as mp_decode_uint().
>> It can't be negative originally by design.
> 
> No, Vlad, this is not how signed\unsigned numbers are processed.

Yeah, I know how the numbers are processed. But right here it
does not matter. The state initially appears from mp_decode_uint()
which returns uint64_t. Hence it is positive initially. Always.
You can't make mp_decode_uint() < 0 true without any casts.
It becomes truncated later.

>> Secondly, and most importantly, the state is decoded as
>> uint64_t raft_request.state, but it is saved without checks
>> into enum struct raft_msg.state in box_raft_request_to_msg().
> 
> I suppose this is done because box_raft_request_to_msg a kind of
> general wrapper which doesn't care much about contents validation
> but simply represent the data in a new structure?

Yes.

> When stage
> of processing starts (ie raft_process_msg helper) we validate
> each member, so I think this is a good architecture to investigate
> contents in raft_process_msg,

You can't validate it during decoding properly, because the decoder
does not and should not depend on the raft library. It does not
know which states are valid. It only decodes the fields into numbers.

> but if you prefer to use
> box_raft_request_to_msg instead I would make it so.

It would be good to keep box_raft_request_to_msg() simple. But it is
not a requirement.

>> So the current patch does not change almost anything I
>> suppose? Because if I send ((UINT32_MAX << 31) | 0x01), it
>> will work. Because the upper 32 bits would be truncated
>> (assuming the enum is 32 bits), and your checks are done too
>> late to notice it.
>>
>> Correct? If so, then I would propose to fix the issue entirely,
>> if you want to work on that.
> 
> I think I didnt put more technical details in commit message,
> let me try to explain in a more verbose way: the enum is stored
> as integer type but standart doesn't require it to be signed
> or unsigned, so it is implementation defined. Currently on my
> gcc-11 series the if statement consider @state member as
> _unsigned_ integer thus basically my patch does absolutely
> nothing, the code remains the same on asm level. Still a compiler
> has all rights to use a signed "state" and in this case our
> `if` won't hit leading to undefined behaviour in case if the
> "state" is in form (0x80000000 | x) and what is worse we won't
> even able to figure out what has happened.
> 
> We can emulate this behaviour by using
> 
> raft_process_msg
> 	...
> 	if ((int)req->state < 0) {
> 		panic("RAFT: req->state > 0 -> %d", (int)req->state >= (int)raft_state_MAX);
> 		assert(req->state > 0);
> 	}
> 
> in result if we pass -1u as state in the test we will have
> [001] +2021-06-26 15:52:58.610 [57492] main/101/main F> RAFT: req->state > 0 -> 0
> 
> thus if compiler use signed int type as internal representation then
> (int)req->state >= (int)raft_state_MAX won't take the branch because
> it is a negative value.

The very fact you needed to put so much effort into the explanation
of a simple negative number check means it is a huge overkill. Please,
compare with < 0 explicitly. If the compiler will store the enum as uint,
it will drop your condition as unreachable anyway.

> Surely the patch won't catch integer overflows since we trim the
> data as you noted above because, gimme some time to think about,
> I really like how our box_raft_request_to_msg helpers are done
> and if it possible would love to not change them much: they are
> simple and short now.

You can try to fix it by changing state to uint64_t in raft_request (now
it is uint32_t), then you need to check for the valid values in
box_raft_request_to_msg(). Or you can patch raft_msg to store the state as
uint64_t. So the value wouldn't be truncated anywhere and raft_process_msg()
could check its correctness properly.

>>> diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
>>> index eacdddb7e..409e983f0 100644
>>> --- a/src/lib/raft/raft.c
>>> +++ b/src/lib/raft/raft.c
>>> @@ -309,7 +309,9 @@ 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 || req->state >= raft_state_MAX) {
>>> +
>>> +	if (req->term == 0 || req->state == 0 ||
>>> +	    (unsigned)req->state >= raft_state_MAX) {
>>
>> Please, perform a normal < 0 comparison. No need for unsigned cast
>> cast.
> 
> We need an explicit cast here because as I said the internal
> implementation is compiler defined.

It has nothing to do with the compiler-specific implementation.
It is simply over-engineering. You cast value to unsigned to
check if it is negative. WTF? Lets do a normal comparison.

> On the other hands I played with a number of gcc and llvm compilers
> and for this code they all does correct comparision (as unsigned
> integers) so maybe we could just leave it as is, simply close
> the bug as won't implement and spend time on more important things?
> I don't mind actually, so up to you just say me what you think.

I don't mind fixing the issue. But now your patch does not fix it,
unfortunately. If you want to fix it - go on. But it must be truly
fixed then.

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches]

On Sun, Jun 27, 2021 at 04:12:01PM +0200, Vladislav Shpilevoy wrote:
...
> > 
> > thus if compiler use signed int type as internal representation then
> > (int)req->state >= (int)raft_state_MAX won't take the branch because
> > it is a negative value.
> 
> The very fact you needed to put so much effort into the explanation
> of a simple negative number check means it is a huge overkill. Please,
> compare with < 0 explicitly. If the compiler will store the enum as uint,
> it will drop your condition as unreachable anyway.

I put such detailed description because the proper easier fix is to convert
the value to unsigned int _explicitly_ (due to implicit type conversion
compiler does and it is implementation defined, you simply don't know
what exactly compiler does under the hood) and compare it with max,
but I've obviously failed to convince you. Never mind.

> >>
> >> Please, perform a normal < 0 comparison. No need for unsigned cast
> >> cast.
> > 
> > We need an explicit cast here because as I said the internal
> > implementation is compiler defined.
> 
> It has nothing to do with the compiler-specific implementation.

The whole issue is about compiler internals.

> It is simply over-engineering. You cast value to unsigned to
> check if it is negative. WTF? Lets do a normal comparison.

I don't get your "wtf" here, what is wrong with using two complement
properties?

> > On the other hands I played with a number of gcc and llvm compilers
> > and for this code they all does correct comparision (as unsigned
> > integers) so maybe we could just leave it as is, simply close
> > the bug as won't implement and spend time on more important things?
> > I don't mind actually, so up to you just say me what you think.
> 
> I don't mind fixing the issue. But now your patch does not fix it,
> unfortunately. If you want to fix it - go on. But it must be truly
> fixed then.

I rather switch to another bug. The patch fixes exactly what it should,
nothing more, nothing less. But this is your code in first place thus
if you don't like the way I patched it, I suppose it is quite fine
and there is nothing to argue about because the code author usually
selects what better fits his architecture. And taking into account
that modern compilers doesn't cause problems here we can safely
live with it.

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches]

Vlad, how about the below version? The branch is the same,
gorcunov/gh-6067-raft-state-verify
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] raft: more precise verification of incoming request state

When new raft message comes in from the network we need
to be sure that the payload is suitable for processing,
in particular `raft_msg::state` must be valid because
our code logic depends on it.

Since the `raft_msg::state` is declared as enum the
its processing is implementation defined an a compiler
might treat it as unsigned or signed int. In the latter
case the `if` statement won't be taken which will lead
to undefined behaviour. So

1) When packet is decoded lets check the values are
   not trimmed, in particular vote and state shall
   fit uint32_t storage;
2) In case if compiler treats enums as signed integers
   we start using potential underflows via LE operator;
2) Use panic() instead of unreacheable() macro;
3) Extend testing.

Closes #6067

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xrow.c        | 16 ++++++++++++++--
 src/lib/raft/raft.c   |  6 ++++--
 test/unit/raft.c      | 10 +++++++++-
 test/unit/raft.result |  4 +++-
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/src/box/xrow.c b/src/box/xrow.c
index 16cb2484c..75f5c94af 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1055,6 +1055,7 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
 		if (mp_typeof(*pos) != MP_UINT)
 			goto bad_msgpack;
 		uint64_t key = mp_decode_uint(&pos);
+		uint64_t val;
 		switch (key) {
 		case IPROTO_RAFT_TERM:
 			if (mp_typeof(*pos) != MP_UINT)
@@ -1064,12 +1065,17 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
 		case IPROTO_RAFT_VOTE:
 			if (mp_typeof(*pos) != MP_UINT)
 				goto bad_msgpack;
-			r->vote = mp_decode_uint(&pos);
+			val = mp_decode_uint(&pos);
+			if (val > UINT_MAX)
+				goto bad_vote;
+			r->vote = val;
 			break;
 		case IPROTO_RAFT_STATE:
 			if (mp_typeof(*pos) != MP_UINT)
 				goto bad_msgpack;
-			r->state = mp_decode_uint(&pos);
+			if (val > UINT_MAX)
+				goto bad_state;
+			r->state = val;
 			break;
 		case IPROTO_RAFT_VCLOCK:
 			r->vclock = vclock;
@@ -1088,6 +1094,12 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
 bad_msgpack:
 	xrow_on_decode_err(begin, end, ER_INVALID_MSGPACK, "raft body");
 	return -1;
+bad_vote:
+	xrow_on_decode_err(begin, end, ER_INVALID_MSGPACK, "raft vote overflow");
+	return -1;
+bad_state:
+	xrow_on_decode_err(begin, end, ER_INVALID_MSGPACK, "raft state overflow");
+	return -1;
 }
 
 int
diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
index eacdddb7e..769b1a6ef 100644
--- a/src/lib/raft/raft.c
+++ b/src/lib/raft/raft.c
@@ -309,7 +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 || req->state >= raft_state_MAX) {
+
+	if (req->term == 0 || req->state <= 0 || req->state >= raft_state_MAX) {
 		diag_set(RaftError, "Invalid term or state");
 		return -1;
 	}
@@ -406,7 +407,8 @@ raft_process_msg(struct raft *raft, const struct raft_msg *req, uint32_t source)
 			raft_sm_become_leader(raft);
 			break;
 		default:
-			unreachable();
+			panic("RAFT: unreacheable state hit");
+			break;
 		}
 	}
 	if (req->state != RAFT_STATE_LEADER) {
diff --git a/test/unit/raft.c b/test/unit/raft.c
index 6369c42d3..b9bc49b78 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(9);
+	raft_start_test(11);
 	struct raft_msg msg;
 	struct raft_node node;
 	struct vclock vclock;
@@ -294,6 +294,14 @@ raft_test_bad_msg(void)
 	is(raft_node_process_msg(&node, &msg, 2), -1, "bad state");
 	is(node.raft.term, 1, "term from the bad message wasn't used");
 
+	msg = (struct raft_msg){
+		.state = -1,
+		.term = 10,
+		.vote = 2,
+	};
+	is(raft_node_process_msg(&node, &msg, 2), -1, "bad negative 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 598a7e760..f89cd1658 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..9
+    1..11
     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
@@ -55,6 +55,8 @@ ok 2 - subtests
     ok 7 - term can't be 0
     ok 8 - bad state
     ok 9 - term from the bad message wasn't used
+    ok 10 - bad negative state
+    ok 11 - term from the bad message wasn't used
 ok 3 - subtests
 	*** raft_test_bad_msg: done ***
 	*** raft_test_vote ***
-- 
2.31.1

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the fixes!

See 4 comments below.

1. The build does not work:
/Users/gerold/Work/Repositories/tarantool/src/box/xrow.c:1076:8: error: variable 'val' is uninitialized when used here [-Werror,-Wuninitialized]
                        if (val > UINT_MAX)
                            ^~~
/Users/gerold/Work/Repositories/tarantool/src/box/xrow.c:1058:15: note: initialize the variable 'val' to silence this warning
                uint64_t val;
                            ^
                             = 0

> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 16cb2484c..75f5c94af 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -1064,12 +1065,17 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
>  		case IPROTO_RAFT_VOTE:
>  			if (mp_typeof(*pos) != MP_UINT)
>  				goto bad_msgpack;
> -			r->vote = mp_decode_uint(&pos);
> +			val = mp_decode_uint(&pos);
> +			if (val > UINT_MAX)
> +				goto bad_vote;
> +			r->vote = val;
>  			break;
>  		case IPROTO_RAFT_STATE:
>  			if (mp_typeof(*pos) != MP_UINT)
>  				goto bad_msgpack;
> -			r->state = mp_decode_uint(&pos);

2. You deleted the state decode. I assume not a single replication
test passes now, correct?

> +			if (val > UINT_MAX)

3. State and vote have uint32_t type. Please, use UINT32_MAX.

> +				goto bad_state;
> +			r->state = val;
>  			break;
>  		case IPROTO_RAFT_VCLOCK:
>  			r->vclock = vclock;
> diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
> index eacdddb7e..769b1a6ef 100644
> --- a/src/lib/raft/raft.c
> +++ b/src/lib/raft/raft.c
> @@ -309,7 +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 || req->state >= raft_state_MAX) {
> +
> +	if (req->term == 0 || req->state <= 0 || req->state >= raft_state_MAX) {

4. Still, you assume you can safely assign uint32_t value to enum raft_state.
I don't think it is a good idea. What if the enum someday will become 1 byte?
Lets not rely on its size. What was wrong with turning the enum into uint32/64
like I proposed before?

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Jul 07, 2021 at 11:25:45PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> See 4 comments below.
> 
> 1. The build does not work:
> /Users/gerold/Work/Repositories/tarantool/src/box/xrow.c:1076:8: error: variable 'val' is uninitialized when used here [-Werror,-Wuninitialized]
>                         if (val > UINT_MAX)
>                             ^~~
> /Users/gerold/Work/Repositories/tarantool/src/box/xrow.c:1058:15: note: initialize the variable 'val' to silence this warning
>                 uint64_t val;
>                             ^
>                              = 0

Thanks! You know, I don't get why compiler is complaining here, since
we use @val only after assignment

		val = mp_decode_uint(&pos);
		if (val > UINT_MAX)
			goto bad_vote;
		r->vote = val;

so this is pretty weird. I'll update of course to make it compilabe for
your instance but to be honest I don't understand this.

> 
> > diff --git a/src/box/xrow.c b/src/box/xrow.c
> > index 16cb2484c..75f5c94af 100644
> > --- a/src/box/xrow.c
> > +++ b/src/box/xrow.c
> > @@ -1064,12 +1065,17 @@ xrow_decode_raft(const struct xrow_header *row, struct raft_request *r,
> >  		case IPROTO_RAFT_VOTE:
> >  			if (mp_typeof(*pos) != MP_UINT)
> >  				goto bad_msgpack;
> > -			r->vote = mp_decode_uint(&pos);
> > +			val = mp_decode_uint(&pos);
> > +			if (val > UINT_MAX)
> > +				goto bad_vote;
> > +			r->vote = val;
> >  			break;
> >  		case IPROTO_RAFT_STATE:
> >  			if (mp_typeof(*pos) != MP_UINT)
> >  				goto bad_msgpack;
> > -			r->state = mp_decode_uint(&pos);
> 
> 2. You deleted the state decode. I assume not a single replication
> test passes now, correct?

Nope :) I write it back a bit later once verification is complete.
I ran the test locally before sending the patch.

> 
> > +			if (val > UINT_MAX)
> 
> 3. State and vote have uint32_t type. Please, use UINT32_MAX.

UINT32_MAX is just an extension over UINT_MAX but sure, will update.

> 
> > +				goto bad_state;
> > +			r->state = val;

Here is it written back once we know that trimming value to u32 is safe.

> > --- a/src/lib/raft/raft.c
> > +++ b/src/lib/raft/raft.c
> > @@ -309,7 +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 || req->state >= raft_state_MAX) {
> > +
> > +	if (req->term == 0 || req->state <= 0 || req->state >= raft_state_MAX) {
> 
> 4. Still, you assume you can safely assign uint32_t value to enum raft_state.
> I don't think it is a good idea. What if the enum someday will become 1 byte?

It won't. This will violate the C language standart. Enum has to have int type.
In case if there some rare architecture where sizeof(int) = 1 then enum size
will be our least problem I guarantee.

> Lets not rely on its size. What was wrong with turning the enum into uint32/64
> like I proposed before?

Actually there is nothing wrong with using uint instead, I thought keeping
it as a former enum will be less intrusive. But sure thing, if you prefer
uint I'll make it so. Gimme some time to prepare a patch then (tomorrow
I think).

Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Jul 08, 2021 at 12:59:33AM +0300, Cyrill Gorcunov wrote:
> > 2. You deleted the state decode. I assume not a single replication
> > test passes now, correct?
> 
> Nope :) I write it back a bit later once verification is complete.
> I ran the test locally before sending the patch.

Vlad, I think I know what happened: there was a typo which I fixed
and commited locally but didn't force pushed out. Just realized it.
I updated repo. But since we gonna switch to uintX instead of enum
I think it doesn't worth to resend the patch.

[Tarantool-patches] [PATCH v3] raft: change request state to uint64_t

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Jul 08, 2021 at 01:05:00AM +0300, Cyrill Gorcunov wrote:
> On Thu, Jul 08, 2021 at 12:59:33AM +0300, Cyrill Gorcunov wrote:
> > > 2. You deleted the state decode. I assume not a single replication
> > > test passes now, correct?
> > 
> > Nope :) I write it back a bit later once verification is complete.
> > I ran the test locally before sending the patch.
> 
> Vlad, I think I know what happened: there was a typo which I fixed
> and commited locally but didn't force pushed out. Just realized it.
> I updated repo. But since we gonna switch to uintX instead of enum
> I think it doesn't worth to resend the patch.

I force pushed an update
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Fri, 25 Jun 2021 13:03:12 +0300
Subject: [PATCH v3] raft: change request state to uint64_t

When new raft message comes in from the network we need
to be sure that the payload is suitable for processing,
in particular `raft_msg::state` must be valid because
our code logic depends on it.

For this sake make `raft_msg::state` being uint64_t
which allows to an easier processing of the state
field verification.

Same time use panic() instead of unreacheable() macro
because the test for valid state must be enabled all
the time.

Closes #6067

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xrow.h        |  2 +-
 src/lib/raft/raft.c   |  5 +++--
 src/lib/raft/raft.h   |  4 ++--
 test/unit/raft.c      | 10 +++++++++-
 test/unit/raft.result |  4 +++-
 5 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/box/xrow.h b/src/box/xrow.h
index 055fd31e1..9f61bfd47 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -288,7 +288,7 @@ xrow_decode_synchro(const struct xrow_header *row, struct synchro_request *req);
 struct raft_request {
 	uint64_t term;
 	uint32_t vote;
-	uint32_t state;
+	uint64_t state;
 	const struct vclock *vclock;
 };
 
diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
index eacdddb7e..ef11ef89f 100644
--- a/src/lib/raft/raft.c
+++ b/src/lib/raft/raft.c
@@ -44,7 +44,7 @@
  * a valid data incomes.
  */
 const char *
-raft_state_str(uint32_t state)
+raft_state_str(uint64_t state)
 {
 	static const char *str[] = {
 		[0]			= "invalid (0)",
@@ -406,7 +406,8 @@ raft_process_msg(struct raft *raft, const struct raft_msg *req, uint32_t source)
 			raft_sm_become_leader(raft);
 			break;
 		default:
-			unreachable();
+			panic("RAFT: unreacheable state hit");
+			break;
 		}
 	}
 	if (req->state != RAFT_STATE_LEADER) {
diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h
index fae30b03d..223e0509f 100644
--- a/src/lib/raft/raft.h
+++ b/src/lib/raft/raft.h
@@ -92,7 +92,7 @@ enum raft_state {
  * Decode raft state into string representation.
  */
 const char *
-raft_state_str(uint32_t state);
+raft_state_str(uint64_t state);
 
 /**
  * Basic Raft communication unit for talking to other nodes, and even to other
@@ -110,7 +110,7 @@ struct raft_msg {
 	 * State of the instance. Can be 0 if the state does not matter for the
 	 * message. For instance, when the message is sent to disk.
 	 */
-	enum raft_state state;
+	uint64_t state;
 	/**
 	 * Vclock of the instance. Can be NULL, if the node is not a candidate.
 	 * Also is omitted when does not matter (when the message is for disk).
diff --git a/test/unit/raft.c b/test/unit/raft.c
index 6369c42d3..b9bc49b78 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(9);
+	raft_start_test(11);
 	struct raft_msg msg;
 	struct raft_node node;
 	struct vclock vclock;
@@ -294,6 +294,14 @@ raft_test_bad_msg(void)
 	is(raft_node_process_msg(&node, &msg, 2), -1, "bad state");
 	is(node.raft.term, 1, "term from the bad message wasn't used");
 
+	msg = (struct raft_msg){
+		.state = -1,
+		.term = 10,
+		.vote = 2,
+	};
+	is(raft_node_process_msg(&node, &msg, 2), -1, "bad negative 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 598a7e760..f89cd1658 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..9
+    1..11
     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
@@ -55,6 +55,8 @@ ok 2 - subtests
     ok 7 - term can't be 0
     ok 8 - bad state
     ok 9 - term from the bad message wasn't used
+    ok 10 - bad negative state
+    ok 11 - term from the bad message wasn't used
 ok 3 - subtests
 	*** raft_test_bad_msg: done ***
 	*** raft_test_vote ***
-- 
2.31.1

Re: [Tarantool-patches] [PATCH v3] raft: change request state to uint64_t

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the fixes!

LGTM.

Pushed to master, 2.8, and 2.7.