From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 0B1E76EC58; Fri, 25 Jun 2021 13:07:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0B1E76EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1624615634; bh=iUI8yi7MwCPbK7ZoOoWijVwFFLGr+KwLBGaQfVbuLz4=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=M4UvHJpLuu5EsNj/Y+VYWlEKn7r8I9Vqo9/9aM4VfqfbDqClQVSJFCNi0lGrjovtg k+LO7xzDuHGllU5sq8dUjCCLLstnrmdzID8MVdnYXfZPluLZcIKJVHnQ6c0whRT6rH kbC5U2vIlKpzVrglNTrHc0cgn5nvutNHcjFcq6hQ= Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C03E36EC58 for ; Fri, 25 Jun 2021 13:07:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C03E36EC58 Received: by mail-lf1-f49.google.com with SMTP id a15so7592730lfr.6 for ; Fri, 25 Jun 2021 03:07:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=cmOioiOPcNBRycbW/154mY3XQhbnYlVmpZ0R0kYY+tE=; b=gcDRSoAbCha7nzj/Q0yoHxiOOnGift/KagODqyo32EhMp8674u+j3cw3OH/YGFoBtp UL98y9EO7sxZOPZ7P6ZYap9hA3MiP1bFzXIb9Ljf/4JU3c1t+T8tuHObEUAvwq9g9NTm b0NTFLcN9h7a9KRCJkGYXN5afHoKBGbe3SjVse3W1TWVS3aOGa8/rx8u0qYTJkwWKoI1 HmO8oYRSjIwIyaImpEsbku9qEGnELjtnxjuKVaWJNm+MlI9yWIdbWFoZDE0BWOQftQ/0 JpW9W9pFN+xZ5G4lSCMjrMTbSC4LwlSvuTa1tXS9/1+L6hL0NJvZBzaAqWshc0VWiAHA J/tg== X-Gm-Message-State: AOAM531v7P+HxDb7nXQWIyS5NZOxkpEkSB1Gd43uJ9L1I+uKgNazcFGl PF3ZM8HKl+DApTx6l8ZOAtpQ5/JQpZo= X-Google-Smtp-Source: ABdhPJwCDSBgIAWBcRLMGFET1kdMVZG+TVMA+XiF36qUke3yy3/N/drTvo6MdyJYtwinrI+CDk/KUg== X-Received: by 2002:a05:6512:3d8a:: with SMTP id k10mr7830161lfv.109.1624615632535; Fri, 25 Jun 2021 03:07:12 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id u23sm471993lfq.78.2021.06.25.03.07.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Jun 2021 03:07:11 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id B9D235A0020; Fri, 25 Jun 2021 13:07:10 +0300 (MSK) To: tml Date: Fri, 25 Jun 2021 13:07:07 +0300 Message-Id: <20210625100707.87807-1-gorcunov@gmail.com> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Cc: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 --- 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