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 0DBEF6EC40; Mon, 20 Sep 2021 11:40:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0DBEF6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632127211; bh=1pihLfRkTvTX00r0N8Ws2Jg2nvTwMqHQBV8YYGzYNDA=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=lfbVEKeVpCHx5u0oKD9qNMo5OZ3mYjCyQPUCKo703HHT6CyDReZGNHsOknwPNrY4y cfxyEwHS1pUPelkgtsRIRFnw/lNsQmwb0uVHYDRLHoc5vkeg1kJR03kryVFWKJ3TlH t8oQKI81TdO0ywQT+Qv4TnQPgFO2kMD0pIELF+18= Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 960DA6EC40 for ; Mon, 20 Sep 2021 11:40:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 960DA6EC40 Received: by smtp48.i.mail.ru with esmtpa (envelope-from ) id 1mSEqO-0001tc-JK; Mon, 20 Sep 2021 11:40:09 +0300 Date: Mon, 20 Sep 2021 11:38:43 +0300 To: sergos Message-ID: References: <45fce284fce8df636a18e7303114f82d5bbce2f0.1631170629.git.skaplun@tarantool.org> <31B92E03-B5C4-4719-9191-41D8581C930E@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <31B92E03-B5C4-4719-9191-41D8581C930E@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91AE02D33A9C88A2F5B11AABA1A86F90E3128B236C8D9319300894C459B0CD1B90D50D3AB0464853141B68B7DA86A1212C10F4D8BEBE1110E1313137BD0912F42 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BC08626EA5717D14EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CAA352D56883AEE98638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8677166A9B5E2989BECBB8D121F3FCE95117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B3A703B70628EAD7BA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CAA44A86D94E7BBB043847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A571776A8EE637913591219D12A29D7E380AB04DB8A7F4C61FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75DD5744CF7ED0C6846D6546786ADF492D5A0AA20F8A0307212E763F503762DF50CC9F233799F3C5CF8E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34E3E1DC0F9BD553095F8460D274ED711906C304273CB4C97291CFD15C9E119FF2BA09D72A0800753C1D7E09C32AA3244C5A241636EE4AA7FDE55155F98396335955E75C8D0ED9F6EEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojsFc9NKou+3jEF2TgvO8mzA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB40D141EB68966CDF409EA4A94F2858ABF9D8EA9046FAED6CAF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key. 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergos! Thanks for the review! On 15.09.21, sergos wrote: > Hi! Thanks for the patch! > > Just some test comment. > > Regards, > Sergos > > > On 9 Sep 2021, at 10:03, Sergey Kaplun wrote: > > > > From: Mike Pall > > > > Reported by XmiliaH. > > > > (cherry picked from commit 16d38a4b214e8b8a20be554be77bd20286072365) > > > > This patch fixes the regression introduced in scope of > > fa8e7ffefb715abf55dc5b0c708c63251868 ('Add support for full-range > > 64 bit lightuserdata.'). > > > > The maximum available number of lightuserdata segment is 255. So the > > high bits of this lightuserdata TValue are 0xfffe7fff. The same high > > bits are set for special control variable on the stack for ITERN/ITERC > > bytecodes via ISNEXT bytecode. When ITERN bytecode is despecialize to > > ITERC bytecode and a table has the lightuserdata with the maximum > > available segment number as a key, the special control variable is > > considered as this key and iteration is broken. > > > > This patch forbids to use more than 254 lightuserdata segments to > > avoid clashing with the aforementioned control variable. In case when > > user tries to create lightuserdata with 255th segment number an error > > "bad light userdata pointer" is raised. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#5629 > > --- > > src/lj_udata.c | 3 +- > > test/tarantool-tests/CMakeLists.txt | 1 + > > .../lj-727-lightuserdata-itern.test.lua | 48 ++++++++++++++ > > .../lj-727-lightuserdata-itern/CMakeLists.txt | 1 + > > .../lightuserdata.c | 63 +++++++++++++++++++ > > 5 files changed, 115 insertions(+), 1 deletion(-) > > create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern.test.lua > > create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt > > create mode 100644 test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c > > > > diff --git a/src/lj_udata.c b/src/lj_udata.c > > index 6808b1bc..1b7841fa 100644 > > --- a/src/lj_udata.c > > +++ b/src/lj_udata.c > > @@ -49,9 +49,10 @@ void *lj_lightud_intern(lua_State *L, void *p) > > if (segmap[seg] == up) /* Fast path. */ > > return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u)); > > segnum++; > > + /* Leave last segment unused to avoid clash with ITERN key. */ > > + if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)-1) lj_err_msg(L, LJ_ERR_BADLU); > > } > > if (!((segnum-1) & segnum) && segnum != 1) { > > - if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU); > > lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t); > > setmref(g->gc.lightudseg, segmap); > > } > > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > > index a872fa5e..c933cc3f 100644 > > --- a/test/tarantool-tests/CMakeLists.txt > > +++ b/test/tarantool-tests/CMakeLists.txt > > @@ -60,6 +60,7 @@ add_subdirectory(gh-4427-ffi-sandwich) > > add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64) > > add_subdirectory(gh-6189-cur_L) > > add_subdirectory(lj-49-bad-lightuserdata) > > +add_subdirectory(lj-727-lightuserdata-itern) > > add_subdirectory(lj-flush-on-trace) > > add_subdirectory(misclib-getmetrics-capi) > > > > diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua > > new file mode 100644 > > index 00000000..ebe885bf > > --- /dev/null > > +++ b/test/tarantool-tests/lj-727-lightuserdata-itern.test.lua > > @@ -0,0 +1,48 @@ > > +local tap = require('tap') > > + > > +-- Test file to demonstrate next FF incorrect behaviour on LJ_64. > > +-- See also, https://github.com/LuaJIT/LuaJIT/issues/727. > > + > > +local test = tap.test('lj-727-lightuserdata-itern') > > +test:plan(1) > > + > > +local ud = require('lightuserdata').craft_ptr_wp() > > + > > +-- We now have the tagged lightuuserdata pointer > > +-- 0xFFFE7FFF00000002 in the up before this patch (after the patch > > +-- the maximum available lightuserdata segment is 0xffe). > > Shall we end the test here with just an expectation of an error? > I believe you can make a way simpler test: pcall(craft_ptr()) should work > successfully 254 times and error on an 255th one, isn’t it? Not exactly, I think. The main idea of the test -- generate as much lightuserdata objects as we can, and save them in the same table. After that we check that iteration by them is correct. Test you suggested doesn't show up the possible issue with ITERN, does it? > > > > + > > +-- These pointers are special in for loop keys as they are used in > > +-- the INTERN control variable to denote the current index in the > > +-- array. > > +-- If the ITERN is then patched to ITERC because of > > +-- despecialization via the ISNEXT bytecode, the control variable > > +-- is considered as the existing key in the table and some > > +-- elements are skipped during iteration. > > + > > +local real_next = next > > +local next = next > > + > > +local function itern_test(t, f) > > + for k, v in next, t do > > + f(k, v) > > + end > > +end > > + > > +local visited = {} > > +local t = {1, [ud] = 2345} > > +itern_test(t, function(k, v) > > + visited[k] = v > > + if next == real_next then > > + next = function(tab, key) > > + return real_next(tab, key) > > + end > > + -- Despecialize bytecode. > > + itern_test({}, function() end) > > + end > > +end) > > + > > +test.strict = true > > +test:is_deeply(visited, t, 'userdata node is visited') > > + > > +os.exit(test:check() and 0 or 1) > > diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt > > new file mode 100644 > > index 00000000..564b688b > > --- /dev/null > > +++ b/test/tarantool-tests/lj-727-lightuserdata-itern/CMakeLists.txt > > @@ -0,0 +1 @@ > > +BuildTestCLib(lightuserdata lightuserdata.c) > > diff --git a/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c > > new file mode 100644 > > index 00000000..0fe6441c > > --- /dev/null > > +++ b/test/tarantool-tests/lj-727-lightuserdata-itern/lightuserdata.c > > @@ -0,0 +1,63 @@ > > +#include > > +#include > > + > > +#undef NDEBUG > > +#include > > + > > +/* To stay within 47 bits, lightuserdata is segmented. */ > > +#define LJ_LIGHTUD_BITS_SEG 8 > > +#define NSEGMENTS ((1 << LJ_LIGHTUD_BITS_SEG)-1) > > + > > +/* > > + * The function to wrap: get a number to form lightuserdata to > > + * return with the 0xXXXXXfff00000002 format. > > + * It may raise an error, when the available lightuserdata > > + * segments are run out. > > + */ > > +static int craft_ptr(lua_State *L) > > +{ > > + const unsigned long long i = lua_tonumber(L, 1); > > + lua_pushlightuserdata(L, (void *)((i << 44) + 0xfff00000002)); > > + return 1; > > +} > > + > > +/* > > + * The function to generate bunch of lightuserdata of the > > + * 0xXXXXXfff00000002 format and push the last one on the stack. > > + */ > > +static int craft_ptr_wp(lua_State *L) > > +{ > > + void *ptr = NULL; > > + /* > > + * There are only 255 available lightuserdata segments. > > + * Generate a bunch of pointers to take them all. > > + * XXX: After this patch the last userdata segment is > > + * reserved for ISNEXT/ITERC/ITERN control variable, so > > + * `craft_ptr()` function will raise an error at the last > > + * iteration. > > + */ > > + unsigned long long i = 0; > > + for (; i < NSEGMENTS; i++) { > > + lua_pushcfunction(L, craft_ptr); > > + lua_pushnumber(L, i); > > + if (lua_pcall(L, 1, 1, 0) == LUA_OK) > > + ptr = (void *)lua_topointer(L, -1); > > + else > > + break; > > + } > > + assert(ptr != NULL); > > + /* Overwrite possible error message. */ > > + lua_pushlightuserdata(L, ptr); > > + return 1; > > +} > > + > > +static const struct luaL_Reg lightuserdata[] = { > > + {"craft_ptr_wp", craft_ptr_wp}, > > + {NULL, NULL} > > +}; > > + > > +LUA_API int luaopen_lightuserdata(lua_State *L) > > +{ > > + luaL_register(L, "lightuserdata", lightuserdata); > > + return 1; > > +} > > -- > > 2.31.0 > > > -- Best regards, Sergey Kaplun