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 19FCE6EC58; Sat, 26 Jun 2021 16:35:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 19FCE6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1624714501; bh=z56sLf6c6Qvpn5fhCXxARhJh7Dd2mstzU0pYIPaGNpg=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=GglkVrdZNy8XRAfcbJQ4HkHOHuiugIPUdNbl9pQ+sZ2g1VngSUU1wIzRj+xIx43+5 cBHjFmU0wSzLYQruLFFNJLE7RuY1geKRJKF6s/OmZ1aMKgpbnqx6yFv15ofyd4c5pj v7MdVDEj9VBp+wv0pb3+RXMWcQwu7pDKzG6a8tIY= Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) (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 AF4296EC58 for ; Sat, 26 Jun 2021 16:34:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AF4296EC58 Received: by mail-lf1-f51.google.com with SMTP id u13so21565528lfk.2 for ; Sat, 26 Jun 2021 06:34:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=4afOYZ5IdjrqoaTRVVUEnC9WGHgm12wvD4ohGVDLtKQ=; b=qHWbXdsE94ZrujszFtPyTA+GdYMs3DUaB05vcNlvI7WLbj1v5XOWYhNsMopSzemyMT kBCA9WcdONJ3hio1yMwg4DD63fvIwGU9O8eQRgN6KFwREy/h2aNLzvnl9NWXJo/5YAjp o9EjlFVrVBtFKIj5gE9qnUPnb9WbkGgk/iBwmBhGwnaM0sbVtu2LoRt89j0DezjXPFCX ePBNOIwXH8U/dAX/uUDi2NgQ0z+zueRWQLidOII+hzkmxDP1JswSPOpcMFAv5myGbQ50 rowOSP4046EA4zpV1EusXVZvYIqEUmWjUGyCzU9Lz2z3iTtSZmU03PC0d1cpED37Man9 3Qxg== X-Gm-Message-State: AOAM533OCBarWFBxyRjIBaZ0BCMwpmE5pQ8cSl4UvH/ps8ugcVyvnHv/ j+uiJFnb3ZE5cNmgBVgyUkWYYLWHf38= X-Google-Smtp-Source: ABdhPJxnNvbhDY0+BJ9IJOkREhBOG011JbXQBiEmiE8jJqKYYCDfg3PYh7ec+hsHnSfVLTFN0uoLng== X-Received: by 2002:a05:6512:21ae:: with SMTP id c14mr11793088lft.483.1624714498660; Sat, 26 Jun 2021 06:34:58 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id p24sm440018lfo.0.2021.06.26.06.34.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 26 Jun 2021 06:34:57 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 8B6765A001F; Sat, 26 Jun 2021 16:34:56 +0300 (MSK) Date: Sat, 26 Jun 2021 16:34:56 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20210625100707.87807-1-gorcunov@gmail.com> <9652278e-570e-40e5-b2d1-856fe58179fc@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9652278e-570e-40e5-b2d1-856fe58179fc@tarantool.org> User-Agent: Mutt/2.0.7 (2021-05-04) Subject: Re: [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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Fri, Jun 25, 2021 at 11:49:10PM +0200, Vladislav Shpilevoy wrote: > > 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. No, Vlad, this is not how signed\unsigned numbers are processed. The decoder doesn't care about two complements at all, we just force it to read 8 bytes as unsigned long integer and that's all. And it is up to _us_ as programmers how to treat this 8 bytes next: either we continue using it as an unsigned integer and generated instructions won't be the ones which consiged "sign flag", or we treat it as signed integer in the code so the sign flag will be either "used" in instructions results or extended up to register size. IOW, if we decode a number as unsigned integer it doesn't mean it can't be used as a signed one later. > 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(). I suppose this is done because box_raft_request_to_msg a kind of general wrapper which doesn't care much about contents validation but simply represent the data in a new structure? When stage of processing starts (ie raft_process_msg helper) we validate each member, so I think this is a good architecture to investigate contents in raft_process_msg, but if you prefer to use box_raft_request_to_msg instead I would make it so. > 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. I think I didnt put more technical details in commit message, let me try to explain in a more verbose way: the enum is stored as integer type but standart doesn't require it to be signed or unsigned, so it is implementation defined. Currently on my gcc-11 series the if statement consider @state member as _unsigned_ integer thus basically my patch does absolutely nothing, the code remains the same on asm level. Still a compiler has all rights to use a signed "state" and in this case our `if` won't hit leading to undefined behaviour in case if the "state" is in form (0x80000000 | x) and what is worse we won't even able to figure out what has happened. We can emulate this behaviour by using raft_process_msg ... if ((int)req->state < 0) { panic("RAFT: req->state > 0 -> %d", (int)req->state >= (int)raft_state_MAX); assert(req->state > 0); } in result if we pass -1u as state in the test we will have [001] +2021-06-26 15:52:58.610 [57492] main/101/main F> RAFT: req->state > 0 -> 0 thus if compiler use signed int type as internal representation then (int)req->state >= (int)raft_state_MAX won't take the branch because it is a negative value. Surely the patch won't catch integer overflows since we trim the data as you noted above because, gimme some time to think about, I really like how our box_raft_request_to_msg helpers are done and if it possible would love to not change them much: they are simple and short now. > > 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. We need an explicit cast here because as I said the internal implementation is compiler defined. On the other hands I played with a number of gcc and llvm compilers and for this code they all does correct comparision (as unsigned integers) so maybe we could just leave it as is, simply close the bug as won't implement and spend time on more important things? I don't mind actually, so up to you just say me what you think.