From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 520C822BBE for ; Mon, 1 Jul 2019 17:52:03 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id wFzkVT_F1ZzM for ; Mon, 1 Jul 2019 17:52:03 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1088D22B80 for ; Mon, 1 Jul 2019 17:52:03 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type References: From: Vladislav Shpilevoy Message-ID: Date: Mon, 1 Jul 2019 23:53:02 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Nikita Pettik , tarantool-patches@freelists.org There still are places, which access Mem.u.i/u without checking flags properly. Please, for each place either explain why it is correct, or fix it and add a test. -------------------------------------------------------------- vdbe.c:2659: if ((pDest->flags & (MEM_Int | MEM_UInt)) != 0) { if (field_type == FIELD_TYPE_NUMBER) sqlVdbeMemSetDouble(pDest, pDest->u.i); } You can't access pDest->u.i - it can be a big unsigned. -------------------------------------------------------------- vdbe.c:2955: pIn1 = &aMem[pOp->p1]; uint32_t space_id = pIn1->u.i; Here you touch u.i, but it is always unsigned. You should use u.u, I think. -------------------------------------------------------------- vdbeaux.c:2428: if (flags & MEM_Int) { ... i64 i = pMem->u.i; u64 u; if (i < 0) { u = ~i; } else { u = i; } Why do you check 'i < 0', if you already see its flag is 'Int'. It should be < 0 by definition. -------------------------------------------------------------- vdbeaux.c:2589: u64 v; u32 i; if (serial_type == 7) { assert(sizeof(v) == sizeof(pMem->u.r)); memcpy(&v, &pMem->u.r, sizeof(v)); swapMixedEndianFloat(v); } else { v = pMem->u.i; } Why are you sure it is safe to store u.i into uint64_t variable? -------------------------------------------------------------- vdbeaux.c:2644: u64 x = FOUR_BYTE_UINT(buf); u32 y = FOUR_BYTE_UINT(buf + 4); x = (x << 32) + y; if (serial_type == 6) { /* EVIDENCE-OF: R-29851-52272 Value is a big-endian 64-bit * twos-complement integer. */ pMem->u.i = *(i64 *) & x; pMem->flags = MEM_Int; Vice versa - why is it safe to store uint64_t into u.i, and set its flag to 'Int' (== value is negative). Actually, all this 'serial' shit looks broken. Please, verify that code. -------------------------------------------------------------- vdbeaux.c:2998: if ((f1 & MEM_UInt) != 0) { if ((f2 & MEM_Real) != 0) { return sqlIntFloatCompare(pMem1->u.i, pMem1 is unsigned, according to the first check, but you use u.i. Why? -------------------------------------------------------------- vdbeaux.c:3184: do_int: if ((pKey2->flags & (MEM_Int | MEM_UInt)) != 0) { if (mem1.u.i < pKey2->u.i) { rc = -1; pKey2 is an integer, but you don't know the sign. And use u.i, assuming it is negative, or at least small enough. Why? Line 3207 is the same. -------------------------------------------------------------- vdbemem.c:1431: } else if (pVal->u.i == SMALLEST_INT64) { pVal->u.r = -(double)SMALLEST_INT64; MemSetTypeFlag(pVal, MEM_Real); } else { pVal->u.i = -pVal->u.i; } You compare u.i and SMALLEST_INT64, but you can't be sure, that u.i is not a big unsigned, can you? -------------------------------------------------------------- I will verify u.u, MEM_Int, and MEM_UInt usages, when we finish with u.i.