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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.