Tarantool development patches archive
 help / color / mirror / Atom feed
From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key.
Date: Wed, 15 Sep 2021 18:31:12 +0300
Message-ID: <31B92E03-B5C4-4719-9191-41D8581C930E@tarantool.org> (raw)
In-Reply-To: <45fce284fce8df636a18e7303114f82d5bbce2f0.1631170629.git.skaplun@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
> 


  reply	other threads:[~2021-09-15 15:31 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-09  7:03 [Tarantool-patches] [PATCH luajit 0/3] Follow-up fixes for full 64-bit lightuserdata interning Sergey Kaplun via Tarantool-patches
2021-09-09  7:03 ` [Tarantool-patches] [PATCH luajit 1/3] test: fix path storage for non-concatable objects Sergey Kaplun via Tarantool-patches
2021-09-15 15:30   ` sergos via Tarantool-patches
2021-09-20  8:28     ` Sergey Kaplun via Tarantool-patches
2021-09-20  9:37       ` sergos via Tarantool-patches
2021-09-09  7:03 ` [Tarantool-patches] [PATCH luajit 2/3] Reorganize lightuserdata interning code Sergey Kaplun via Tarantool-patches
2021-09-15 15:30   ` sergos via Tarantool-patches
2021-09-20  8:32     ` Sergey Kaplun via Tarantool-patches
2021-09-20  9:37       ` sergos via Tarantool-patches
2021-09-09  7:03 ` [Tarantool-patches] [PATCH luajit 3/3] Avoid conflict between 64 bit lightuserdata and ITERN key Sergey Kaplun via Tarantool-patches
2021-09-15 15:31   ` sergos via Tarantool-patches [this message]
2021-09-20  8:38     ` Sergey Kaplun via Tarantool-patches
2021-09-20  9:37       ` sergos via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=31B92E03-B5C4-4719-9191-41D8581C930E@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git