From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [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 735B446971A for ; Wed, 11 Dec 2019 13:49:05 +0300 (MSK) From: imeevma@tarantool.org Date: Wed, 11 Dec 2019 13:49:02 +0300 Message-Id: <0e3de775c20f0e5776301a5b39fa1e22e0e8bdb7.1576060409.git.imeevma@gmail.com> Subject: [Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: alexander.turenko@tarantool.org Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for review! My answers and new patch below. https://github.com/tarantool/tarantool/issues/4672 https://github.com/tarantool/tarantool/tree/imeevma/gh-4672-fix-min-integer-in-lua On 12/6/19 3:41 AM, Alexander Turenko wrote: > Nice catch! > > I think it worth to file an issue. The problem looks small, but it would > be better to let users aware of it. > > Let's describe where the problem can appear: msgpack module (and so box > functions, binary protocol responses -- everything that encodes lua > values to msgpack except msgpackffi and so except tuple.update), json > and yaml modules. Describe buggy and desired behaviour for -2^63: > > (I use type names from [msgpack spec][1] rather then our sizeless MP_*.) > > * msgpack module (the same for iproto responses I guess): expected > int64, got float64. > * box (insert into an integer column): expected success, got an error. > * json / yaml modules (and so console output): expected precise value > -9223372036854775808, got approximated one in scientific notation > -9.2233720368548e+18. > > If you know other cases, let's add them too. Maybe something in SQL? > Done. SQL do not use this serializer, so it doesn't have this problem. > I would add all those cases to corresponding tests: re msgpack, re > msgpackffi, re json, re yaml, re iproto, re console, re box's insert, re > tuple.update, (something else?). Yep, don't miss msgpackffi and > tuple.update -- its behaviour is right, but a case is worthful anyway to > catch possible future regressions. > I added a new test in serializer_test. Since this module is used in the json, yaml, msgpack and msgpackffi tests, I think we do not need any other tests for these modules. > Let's consult with Kirill Yu. whether we should add those cases to > existing files, add all them into one file or create a file for each > one. Personally I vote for the first way. > Kirill said to create one test for each test-suite. Since we no longer need tests in app-tap, we only need one test in box-tap. However, I created a test in box instead of box-tap, as it was a little easier. > See other comments below. > > [1]: https://github.com/msgpack/msgpack/blob/master/spec.md > > WBR, Alexander Turenko. > >> lua: allow to push -2^63 in INTEGER field > > Nit: This header is a bit misleading: prefix states that it is about > lua, but 'integer field' is more about box. > > I would give a general description in the header, like so: > >> lua/serializer: encode Lua number -2^63 as integer > Fixed. > And would give all details about particular cases in the commit message. > > On Thu, Nov 28, 2019 at 05:11:49PM +0300, imeevma@tarantool.org wrote: >> This patch fixes a bug that appeared due to commit 3a13be1d. > > Nit: We usually use form 3a13be1de38e79b6b7e0e7a6fa81f4c59c379f4e ('lua: > fix lua_tofield() for 2**64 value') for a reference to a commit. > Done. However, in the log and on github this looks different since github automatically shortens the commit id. >> --- >> https://github.com/tarantool/tarantool/tree/imeevma/small_fixes >> >> src/lua/utils.c | 2 +- >> test/engine/insert.result | 28 ++++++++++++++++++++++++++++ >> test/engine/insert.test.lua | 8 ++++++++ >> 3 files changed, 37 insertions(+), 1 deletion(-) >> >> diff --git a/src/lua/utils.c b/src/lua/utils.c >> index 7f7bf47..bad5f7b 100644 >> --- a/src/lua/utils.c >> +++ b/src/lua/utils.c >> @@ -667,7 +667,7 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index, >> } else if (num >= 0 && num < exp2(64)) { >> field->type = MP_UINT; >> field->ival = (uint64_t) num; >> - } else if (num > -exp2(63) && num < exp2(63)) { >> + } else if (num >= -exp2(63) && num < exp2(63)) { >> field->type = MP_INT; >> field->ival = (int64_t) num; >> } else { > >> diff --git a/test/engine/insert.test.lua b/test/engine/insert.test.lua >> index 39b1501..d6df60e 100644 >> --- a/test/engine/insert.test.lua >> +++ b/test/engine/insert.test.lua >> @@ -123,3 +123,11 @@ s:select{} >> s:drop() >> fiber = nil >> >> +format = {{name = 'i', type = 'integer'}} >> +s = box.schema.space.create('s', {format = format, engine = engine}) >> +_ = s:create_index('ii') >> +s:insert({-2^63}) >> +s:insert({-2^64}) >> +s:insert({2^63}) >> +s:insert({2^64}) >> +s:drop() > > Nit: Let's add 'gh-xxxx: short description' above the test case. Done. New patch: >From 0e3de775c20f0e5776301a5b39fa1e22e0e8bdb7 Mon Sep 17 00:00:00 2001 From: Mergen Imeev Date: Fri, 6 Dec 2019 19:22:40 +0300 Subject: [PATCH] lua/serializer: encode Lua number -2^63 as integer This patch fixes a bug that appeared after commit 3a13be1de38e79b6b7e0e7a6fa81f4c59c379f4e ('lua: fix lua_tofield() for 2**64 value') . Due to this bug, -2^63 was serialized as double, although it should be serialized as integer. Closes #4672 diff --git a/src/lua/utils.c b/src/lua/utils.c index d5122ee..208b64e 100644 --- a/src/lua/utils.c +++ b/src/lua/utils.c @@ -667,7 +667,7 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index, } else if (num >= 0 && num < exp2(64)) { field->type = MP_UINT; field->ival = (uint64_t) num; - } else if (num > -exp2(63) && num < exp2(63)) { + } else if (num >= -exp2(63) && num < exp2(63)) { field->type = MP_INT; field->ival = (int64_t) num; } else { diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua index 1609f76..0385597 100644 --- a/test/app-tap/lua/serializer_test.lua +++ b/test/app-tap/lua/serializer_test.lua @@ -101,7 +101,7 @@ local function test_unsigned(test, s) end local function test_signed(test, s) - test:plan(52) + test:plan(53) rt(test, s, -1, 'number') rt(test, s, -1LL, 'number') @@ -143,6 +143,10 @@ local function test_signed(test, s) rt(test, s, ffi.new('short', -128), 'number') rt(test, s, ffi.new('int', -128), 'number') + + -- gh-4672: Make sure that -2^63 encoded as INTEGER. + test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63), + '-2^63 encoded as INTEGER') end local function test_double(test, s) diff --git a/test/box/gh-4672-min-integer-value-in-serializer.result b/test/box/gh-4672-min-integer-value-in-serializer.result new file mode 100644 index 0000000..f4d7adb --- /dev/null +++ b/test/box/gh-4672-min-integer-value-in-serializer.result @@ -0,0 +1,97 @@ +-- test-run result file version 2 +remote = require('net.box') + | --- + | ... +env = require('test_run') + | --- + | ... +test_run = env.new() + | --- + | ... + +-- +-- gh-4672: Due to an error in the serializer, -2^63 was +-- serialized as double, although in accordance with the rules of +-- serialization it should be serialized as an integer. +-- +format={{name='u', type='unsigned'}, {name='i', type='integer'}} + | --- + | ... +s = box.schema.space.create('serializer_test_space', {format=format}) + | --- + | ... +_ = s:create_index('ii') + | --- + | ... + +s:insert({1, -2^63}) + | --- + | - [1, -9223372036854775808] + | ... +s:insert({2, -9223372036854775808LL}) + | --- + | - [2, -9223372036854775808] + | ... +s:insert({3, 0}) + | --- + | - [3, 0] + | ... +s:update(3, {{'=', 'i', -2^63}}) + | --- + | - [3, -9223372036854775808] + | ... +s:insert({4, 0}) + | --- + | - [4, 0] + | ... +s:update(4, {{'=', 'i', -9223372036854775808LL}}) + | --- + | - [4, -9223372036854775808] + | ... + +box.schema.user.grant('guest', 'read, write', 'space', 'serializer_test_space') + | --- + | ... + +cn = remote.connect(box.cfg.listen) + | --- + | ... +s = cn.space.serializer_test_space + | --- + | ... + +s:insert({11, -2^63}) + | --- + | - [11, -9223372036854775808] + | ... +s:insert({12, -9223372036854775808LL}) + | --- + | - [12, -9223372036854775808] + | ... +s:insert({13, 0}) + | --- + | - [13, 0] + | ... +s:update(13, {{'=', 'i', -2^63}}) + | --- + | - [13, -9223372036854775808] + | ... +s:insert({14, 0}) + | --- + | - [14, 0] + | ... +s:update(14, {{'=', 'i', -9223372036854775808LL}}) + | --- + | - [14, -9223372036854775808] + | ... + +cn:close() + | --- + | ... +box.schema.user.revoke('guest', 'read, write', 'space', 'serializer_test_space') + | --- + | ... + +box.space.serializer_test_space:drop() + | --- + | ... diff --git a/test/box/gh-4672-min-integer-value-in-serializer.test.lua b/test/box/gh-4672-min-integer-value-in-serializer.test.lua new file mode 100644 index 0000000..f718a56 --- /dev/null +++ b/test/box/gh-4672-min-integer-value-in-serializer.test.lua @@ -0,0 +1,36 @@ +remote = require('net.box') +env = require('test_run') +test_run = env.new() + +-- +-- gh-4672: Due to an error in the serializer, -2^63 was +-- serialized as double, although in accordance with the rules of +-- serialization it should be serialized as an integer. +-- +format={{name='u', type='unsigned'}, {name='i', type='integer'}} +s = box.schema.space.create('serializer_test_space', {format=format}) +_ = s:create_index('ii') + +s:insert({1, -2^63}) +s:insert({2, -9223372036854775808LL}) +s:insert({3, 0}) +s:update(3, {{'=', 'i', -2^63}}) +s:insert({4, 0}) +s:update(4, {{'=', 'i', -9223372036854775808LL}}) + +box.schema.user.grant('guest', 'read, write', 'space', 'serializer_test_space') + +cn = remote.connect(box.cfg.listen) +s = cn.space.serializer_test_space + +s:insert({11, -2^63}) +s:insert({12, -9223372036854775808LL}) +s:insert({13, 0}) +s:update(13, {{'=', 'i', -2^63}}) +s:insert({14, 0}) +s:update(14, {{'=', 'i', -9223372036854775808LL}}) + +cn:close() +box.schema.user.revoke('guest', 'read, write', 'space', 'serializer_test_space') + +box.space.serializer_test_space:drop()