Tarantool development patches archive
 help / color / mirror / Atom feed
From: Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state
Date: Fri, 2 Jul 2021 16:43:10 +0300	[thread overview]
Message-ID: <YN8X7nDf2O/7mRb+@grain> (raw)
In-Reply-To: <4b0f4d8f-4e0c-00d0-38ad-0b6abad3aafe@tarantool.org>

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


  parent reply	other threads:[~2021-07-02 13:43 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-25 10:07 Cyrill Gorcunov via Tarantool-patches
2021-06-25 21:49 ` Vladislav Shpilevoy via Tarantool-patches
2021-06-25 21:49   ` Vladislav Shpilevoy via Tarantool-patches
2021-06-26 13:34   ` Cyrill Gorcunov via Tarantool-patches
2021-06-27 14:12     ` Vladislav Shpilevoy via Tarantool-patches
2021-06-27 19:42       ` Cyrill Gorcunov via Tarantool-patches
2021-07-02 13:43       ` Cyrill Gorcunov via Tarantool-patches [this message]
2021-07-07 21:25         ` Vladislav Shpilevoy via Tarantool-patches
2021-07-07 21:59           ` Cyrill Gorcunov via Tarantool-patches
2021-07-07 22:05             ` Cyrill Gorcunov via Tarantool-patches
2021-07-08  9:28               ` [Tarantool-patches] [PATCH v3] raft: change request state to uint64_t Cyrill Gorcunov via Tarantool-patches
2021-07-08 21:17                 ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YN8X7nDf2O/7mRb+@grain \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox