Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.

[sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the patch!

Just some test comment.

Regards,
Sergos

> On 9 Sep 2021, at 10:03, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> 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?


> +
> +-- 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 <lua.h>
> +#include <lauxlib.h>
> +
> +#undef NDEBUG
> +#include <assert.h>
> +
> +/* 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
>