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 C70C46DB02; Tue, 5 Oct 2021 12:00:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C70C46DB02 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1633424443; bh=OEjncI24i8I3dNkb8NpZqMooZs7/CIsvic5dGmiP1x8=; 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=XpnLnDpEqlWQ7i1z5+i20XRmIhOcGIO0mPLoSYozfFdw72yhLHB5gfzveAbGpM87+ 6nVkN4NocXzjcEluSa5Q7hMnzwpwHlIfQhTOzSkxHg47ZbhLDRPgDX1jTuaO71tUnE zVfAW/kvc1eIoPTbT48apCQAK19G2xrxKhUalo2s= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 795D86DB02 for ; Tue, 5 Oct 2021 12:00:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 795D86DB02 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mXgJV-0006Wc-Pf; Tue, 05 Oct 2021 12:00:42 +0300 Date: Tue, 5 Oct 2021 12:00:40 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20211005090040.GB55311@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9064ADF4728AA0EE9F29F6F937CFD73092774A1760F25EB43182A05F538085040827B06B73F91B52726BE20DB24DA952A565E0434BD20ACC81EDF6E4FE575E568 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE721D130CF676D2164EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063726CA83C7ABDB938E8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D832F30C781BE8CC20D071C28084AC7E82117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC3A703B70628EAD7BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18F04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B66F6A3E018CF4DC80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5EBA6967AE182C3D0DDF3C518A36D01C13A8723C60A9E2BFED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75C4D20244F7083972410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A246DFF68F0EE19ECD511C11216EDD24DF88AE54C403092F3283240E82DFBCD196D2B33C8BB498061D7E09C32AA3244CF82EAC322F8758DF1BDD9850AE1B98F01DD47778AE04E04D729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojhAh8SZXECpAWXNEbKMDSrg== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DDE4628FB262A03EE7362FD8B4B6C885483D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v4 02/16] sql: fix possible undefined behavior during cast 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: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answers below. On Mon, Oct 04, 2021 at 11:52:18PM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > On 01.10.2021 14:48, imeevma@tarantool.org wrote: > > This patch fixes possible undefined behavior during the implicit cast of > > INTEGER to DOUBLE. The problem is, if the INTEGER is close enough to > > 2^64, it will be cast to 2^64 when it is cast to DOUBLE. Since we have a > > check for loss of precision, this will cause this DOUBLE to be cast to > > an INTEGER, which will result in undefined behavior since this DOUBLE is > > outside the range of INTEGER. > > --- > > src/box/sql/mem.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index 24d6d7dbf..079083fa1 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -682,7 +682,7 @@ uint_to_double_precise(struct Mem *mem) > > assert(mem->type == MEM_TYPE_UINT); > > double d; > > d = (double)mem->u.u; > > - if (mem->u.u != (uint64_t)d) > > + if (d == (double)UINT64_MAX || mem->u.u != (uint64_t)d) > > What if uint really was UINT64_MAX? Then you treat its conversion > into UINT64_MAX double as an error? > The UINT64_MAX value is 2^64-1, which will change to 2^64 when INTEGER is cast to DOUBLE. Since the cast is not exact, an error will be thrown anyway. The question is what we will get when DOUBLE 2^64 is cast to INTEGER? Since it is not defined, I decided to avoid this by adding a new restriction. > Maybe we could simply refuse to convert all ints > 2^53 and <-2^53 > (or whatever is the precise int limit in double). All ints above > these values will convert 'from time to time' depending on exact > values, which does not look reliable anyway. I don't think we should be doing this. One reason is that in this case we must decide what to do with casts from DOUBLE to INTEGER, DOUBLE to DECIMAL, DECIMAL to DOUBLE, and DECIMAL to INTEGER. I don't see simple enough rules to give us an easy way to determine when we should allow these casts and when we shouldn't. The current rule of "allow when cast is exact" seems easier to understand and implement.