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