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

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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