Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org,
	roman Khabibov <roman.habibov@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] serializer: serialize recursive structures
Date: Wed, 21 Apr 2021 15:34:36 +0300	[thread overview]
Message-ID: <67243305-a17e-0923-bbb0-322c3506841c@tarantool.org> (raw)
In-Reply-To: <20210311054949.91263-1-roman.habibov@tarantool.org>

Hello,

seems patch series below is a bit outdated, so I'll review and quote 
actual version in a branch.

 >    @TarantoolBot
 >    __serialize parameter
 >    If __serialize parameter is initialized as a function then this
 >    function should be pure. A pure function is a function which
 >    returns identical values for identical arguments (no variation
 >    with local static variables, non-local variables, mutable
 >    reference arguments or input streams). Otherwise, the behavior
 >    is undefined.

1. __serialize is a method, not parameter

--- /dev/null
+++ b/changelogs/unreleased/serializer-bug.md
@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Recursive structures appeared after __serialize call don't lead
+* to segfault anymore in lua and yaml serializers (gh-5392).

2. seems issue number is wrong, proper one is #3228

3. I have reviewed a code coverage and propose to cover uncovered 
branches in lua_field_try_serialize(), see [1]

I believe it would be enough to add more examples to proposed tests to 
enhance coverage.


1. https://coveralls.io/builds/38864750/source?filename=src%2Flua%2Futils.c


On 11.03.2021 08:49, Roman Khabibov via Tarantool-patches wrote:
> Fix bug with bus error during serializing of recursive
> structures.
>
> Closes #3228
> ---
>
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3228-visited-set
> Issue: https://github.com/tarantool/tarantool/issues/3228
>
>   src/box/lua/call.c                            |   6 +-
>   src/box/lua/execute.c                         |   2 +-
>   src/box/lua/serialize_lua.c                   |  96 ++------
>   src/box/lua/tuple.c                           |   2 +-
>   src/box/sql/func.c                            |   2 +-
>   src/lua/msgpack.c                             |  10 +-
>   src/lua/pickle.c                              |   2 +-
>   src/lua/utils.c                               | 226 ++++++++++++++++--
>   src/lua/utils.h                               |  43 +++-
>   ...-3228-serializer-look-for-recursion.result |  67 ++++++
>   ...228-serializer-look-for-recursion.test.lua |  17 ++
>   test/swim/swim.result                         |  18 +-
>   third_party/lua-cjson/lua_cjson.c             |   4 +-
>   third_party/lua-yaml/lyaml.cc                 |  88 +++----
>   14 files changed, 390 insertions(+), 193 deletions(-)
>   create mode 100644 test/app/gh-3228-serializer-look-for-recursion.result
>   create mode 100644 test/app/gh-3228-serializer-look-for-recursion.test.lua
>
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 0315e720c..e4907a3f7 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -193,7 +193,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
>   		 */
>   		for (int i = 1; i <= nrets; ++i) {
>   			struct luaL_field field;
> -			if (luaL_tofield(L, cfg, NULL, i, &field) < 0)
> +			if (luaL_tofield(L, cfg, 0, NULL, i, &field) < 0)
>   				return luaT_error(L);
>   			struct tuple *tuple;
>   			if (field.type == MP_EXT &&
> @@ -222,7 +222,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
>   	 * Inspect the first result
>   	 */
>   	struct luaL_field root;
> -	if (luaL_tofield(L, cfg, NULL, 1, &root) < 0)
> +	if (luaL_tofield(L, cfg, 0, NULL, 1, &root) < 0)
>   		return luaT_error(L);
>   	struct tuple *tuple;
>   	if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
> @@ -252,7 +252,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
>   	for (uint32_t t = 1; t <= root.size; t++) {
>   		lua_rawgeti(L, 1, t);
>   		struct luaL_field field;
> -		if (luaL_tofield(L, cfg, NULL, -1, &field) < 0)
> +		if (luaL_tofield(L, cfg, 0, NULL, -1, &field) < 0)
>   			return luaT_error(L);
>   		if (field.type == MP_EXT && (tuple = luaT_istuple(L, -1))) {
>   			tuple_to_mpstream(tuple, stream);
> diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
> index 926a0a61c..76d271f5c 100644
> --- a/src/box/lua/execute.c
> +++ b/src/box/lua/execute.c
> @@ -328,7 +328,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
>   		bind->name = NULL;
>   		bind->name_len = 0;
>   	}
> -	if (luaL_tofield(L, luaL_msgpack_default, NULL, -1, &field) < 0)
> +	if (luaL_tofield(L, luaL_msgpack_default, 0, NULL, -1, &field) < 0)
>   		return -1;
>   	switch (field.type) {
>   	case MP_UINT:
> diff --git a/src/box/lua/serialize_lua.c b/src/box/lua/serialize_lua.c
> index caa08a60f..ab612dfc2 100644
> --- a/src/box/lua/serialize_lua.c
> +++ b/src/box/lua/serialize_lua.c
> @@ -215,7 +215,7 @@ trace_node(struct lua_dumper *d)
>   	struct luaL_field field;
>   
>   	memset(&field, 0, sizeof(field));
> -	luaL_checkfield(d->L, d->cfg, lua_gettop(d->L), &field);
> +	luaL_checkfield(d->L, d->cfg, 0, lua_gettop(d->L), &field);
>   
>   	if (field.type < lengthof(mp_type_names)) {
>   		if (field.type == MP_EXT) {
> @@ -234,7 +234,7 @@ trace_node(struct lua_dumper *d)
>   
>   	memset(&field, 0, sizeof(field));
>   
> -	luaL_checkfield(d->L, d->cfg, top, &field);
> +	luaL_checkfield(d->L, d->cfg, 0, top, &field);
>   	say_info("serializer-trace: node    :\tfield type %s (%d)",
>   		 type_str, field.type);
>   }
> @@ -339,41 +339,16 @@ emit_node(struct lua_dumper *d, struct node *nd, int indent,
>   static const char *
>   get_lua_anchor(struct lua_dumper *d)
>   {
> -	const char *s = "";
> -
> -	lua_pushvalue(d->L, -1);
> -	lua_rawget(d->L, d->anchortable_index);
> -	if (!lua_toboolean(d->L, -1)) {
> -		lua_pop(d->L, 1);
> -		return NULL;
> -	}
> -
> -	if (lua_isboolean(d->L, -1)) {
> -		/*
> -		 * This element is referenced more
> -		 * than once but has not been named.
> -		 */
> -		char buf[32];
> -		snprintf(buf, sizeof(buf), "%u", d->anchor_number++);
> -		lua_pop(d->L, 1);
> -		lua_pushvalue(d->L, -1);
> -		lua_pushstring(d->L, buf);
> -		s = lua_tostring(d->L, -1);
> -		lua_rawset(d->L, d->anchortable_index);
> -		trace_anchor(s, false);
> -	} else {
> -		/*
> -		 * An aliased element.
> -		 *
> -		 * FIXME: Need an example to use.
> -		 *
> -		 * const char *str = lua_tostring(d->L, -1);
> -		 */
> -		const char *str = lua_tostring(d->L, -1);
> -		trace_anchor(str, true);
> -		lua_pop(d->L, 1);
> +	const char *anchor = NULL;
> +	int code = get_anchor(d->L, d->anchortable_index, &d->anchor_number,
> +			      &anchor);
> +	if (code == 1) {
> +		trace_anchor(anchor, false);
> +	} else if (code == 2) {
> +		trace_anchor(anchor, true);
> +		return "";
>   	}
> -	return s;
> +	return anchor;
>   }
>   
>   static void
> @@ -783,7 +758,7 @@ dump_node(struct lua_dumper *d, struct node *nd, int indent)
>   		return -1;
>   
>   	memset(field, 0, sizeof(*field));
> -	luaL_checkfield(d->L, d->cfg, lua_gettop(d->L), field);
> +	luaL_checkfield(d->L, d->cfg, 0, lua_gettop(d->L), field);
>   
>   	switch (field->type) {
>   	case MP_NIL:
> @@ -881,49 +856,6 @@ dump_node(struct lua_dumper *d, struct node *nd, int indent)
>   	return emit_node(d, nd, indent, str, len);
>   }
>   
> -/**
> - * Find references to tables, we use it
> - * to find self references in tables.
> - */
> -static void
> -find_references(struct lua_dumper *d)
> -{
> -	int newval;
> -
> -	if (lua_type(d->L, -1) != LUA_TTABLE)
> -		return;
> -
> -	/* Copy of a table for self refs */
> -	lua_pushvalue(d->L, -1);
> -	lua_rawget(d->L, d->anchortable_index);
> -	if (lua_isnil(d->L, -1))
> -		newval = 0;
> -	else if (!lua_toboolean(d->L, -1))
> -		newval = 1;
> -	else
> -		newval = -1;
> -	lua_pop(d->L, 1);
> -
> -	if (newval != -1) {
> -		lua_pushvalue(d->L, -1);
> -		lua_pushboolean(d->L, newval);
> -		lua_rawset(d->L, d->anchortable_index);
> -	}
> -
> -	if (newval != 0)
> -		return;
> -
> -	/*
> -	 * Other values and keys in the table
> -	 */
> -	lua_pushnil(d->L);
> -	while (lua_next(d->L, -2) != 0) {
> -		find_references(d);
> -		lua_pop(d->L, 1);
> -		find_references(d);
> -	}
> -}
> -
>   /**
>    * Dump recursively from the root node.
>    */
> @@ -935,7 +867,7 @@ dump_root(struct lua_dumper *d)
>   	};
>   	int ret;
>   
> -	luaL_checkfield(d->L, d->cfg, lua_gettop(d->L), &nd.field);
> +	luaL_checkfield(d->L, d->cfg, 0, lua_gettop(d->L), &nd.field);
>   
>   	if (nd.field.type != MP_ARRAY || nd.field.size != 1) {
>   		d->err = EINVAL;
> @@ -984,7 +916,7 @@ lua_encode(lua_State *L, struct luaL_serializer *serializer,
>   
>   	/* Push copy of arg we're processing */
>   	lua_pushvalue(L, 1);
> -	find_references(&dumper);
> +	find_references(dumper.L, dumper.anchortable_index, "before");
>   
>   	if (dump_root(&dumper) != 0)
>   		goto out;
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 3e6f043b4..4d417ec34 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -411,7 +411,7 @@ luamp_convert_key(struct lua_State *L, struct luaL_serializer *cfg,
>   		return tuple_to_mpstream(tuple, stream);
>   
>   	struct luaL_field field;
> -	if (luaL_tofield(L, cfg, NULL, index, &field) < 0)
> +	if (luaL_tofield(L, cfg, 0, NULL, index, &field) < 0)
>   		luaT_error(L);
>   	if (field.type == MP_ARRAY) {
>   		lua_pushvalue(L, index);
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index f15d27051..d4a7064f0 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -262,7 +262,7 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
>   		return NULL;
>   	for (int i = 0; i < argc; i++) {
>   		struct luaL_field field;
> -		if (luaL_tofield(L, luaL_msgpack_default,
> +		if (luaL_tofield(L, luaL_msgpack_default, 0,
>   				 NULL, -1 - i, &field) < 0) {
>   			goto error;
>   		}
> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
> index 24b0d2ccd..412736572 100644
> --- a/src/lua/msgpack.c
> +++ b/src/lua/msgpack.c
> @@ -156,11 +156,11 @@ restart: /* used by MP_EXT of unidentified subtype */
>   		lua_pushnil(L);  /* first key */
>   		while (lua_next(L, top) != 0) {
>   			lua_pushvalue(L, -2); /* push a copy of key to top */
> -			if (luaL_tofield(L, cfg, opts, lua_gettop(L), field) < 0)
> +			if (luaL_tofield(L, cfg, 0, opts, lua_gettop(L), field) < 0)
>   				return luaT_error(L);
>   			luamp_encode_r(L, cfg, opts, stream, field, level + 1);
>   			lua_pop(L, 1); /* pop a copy of key */
> -			if (luaL_tofield(L, cfg, opts, lua_gettop(L), field) < 0)
> +			if (luaL_tofield(L, cfg, 0, opts, lua_gettop(L), field) < 0)
>   				return luaT_error(L);
>   			luamp_encode_r(L, cfg, opts, stream, field, level + 1);
>   			lua_pop(L, 1); /* pop value */
> @@ -181,7 +181,7 @@ restart: /* used by MP_EXT of unidentified subtype */
>   		mpstream_encode_array(stream, size);
>   		for (uint32_t i = 0; i < size; i++) {
>   			lua_rawgeti(L, top, i + 1);
> -			if (luaL_tofield(L, cfg, opts, top + 1, field) < 0)
> +			if (luaL_tofield(L, cfg, 0, opts, top + 1, field) < 0)
>   				return luaT_error(L);
>   			luamp_encode_r(L, cfg, opts, stream, field, level + 1);
>   			lua_pop(L, 1);
> @@ -204,7 +204,7 @@ restart: /* used by MP_EXT of unidentified subtype */
>   			if (type != MP_EXT)
>   				return type; /* Value has been packed by the trigger */
>   			/* Try to convert value to serializable type */
> -			luaL_convertfield(L, cfg, top, field);
> +			luaL_convertfield(L, cfg, 0, top, field);
>   			/* handled by luaL_convertfield */
>   			assert(field->type != MP_EXT);
>   			assert(lua_gettop(L) == top);
> @@ -229,7 +229,7 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
>   	}
>   
>   	struct luaL_field field;
> -	if (luaL_tofield(L, cfg, opts, lua_gettop(L), &field) < 0)
> +	if (luaL_tofield(L, cfg, 0, opts, lua_gettop(L), &field) < 0)
>   		return luaT_error(L);
>   	enum mp_type top_type = luamp_encode_r(L, cfg, opts, stream, &field, 0);
>   
> diff --git a/src/lua/pickle.c b/src/lua/pickle.c
> index 65208b5b3..86f0f77e8 100644
> --- a/src/lua/pickle.c
> +++ b/src/lua/pickle.c
> @@ -80,7 +80,7 @@ lbox_pack(struct lua_State *L)
>   		if (i > nargs)
>   			luaL_error(L, "pickle.pack: argument count does not match "
>   				   "the format");
> -		luaL_checkfield(L, luaL_msgpack_default, i, &field);
> +		luaL_checkfield(L, luaL_msgpack_default, 0, i, &field);
>   		switch (*format) {
>   		case 'B':
>   		case 'b':
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index b5a6ca5b7..64404e3fc 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -439,9 +439,54 @@ lua_gettable_wrapper(lua_State *L)
>   	return 1;
>   }
>   
> +/**
> + * Check if node with @a idx serialized.
> + */
> +static int
> +is_serialized(struct lua_State *L, int anchortable_index, int idx)
> +{
> +	if (anchortable_index == 0)
> +		return 0;
> +	lua_pushvalue(L, idx);
> +	lua_rawget(L, anchortable_index);
> +	int is_serialized = 0;
> +	if (lua_isnil(L, -1) == 0) {
> +		lua_pushstring(L, "serialized");
> +		lua_rawget(L, -2);
> +		is_serialized = lua_toboolean(L, -1);
> +		lua_pop(L, 1);
> +	}
> +	lua_pop(L, 1);
> +	return is_serialized;
> +}
> +
> +/**
> + * Mark node with @a idx as serialized.
> + */
> +static void
> +mark_as_serialized(struct lua_State *L, int anchortable_index, int idx)
> +{
> +	if (anchortable_index == 0)
> +		return;
> +	/* Push copy of table. */
> +	lua_pushvalue(L, idx);
> +	lua_pushvalue(L, idx);
> +	lua_rawget(L, anchortable_index);
> +	if (lua_isnil(L, -1) == 1) {
> +		lua_pop(L, 1);
> +		lua_newtable(L);
> +	}
> +	int flags_index = lua_gettop(L);
> +	lua_pushstring(L, "serialized");
> +	lua_pushboolean(L, true);
> +	lua_rawset(L, flags_index);
> +	lua_rawset(L, anchortable_index);
> +}
> +
>   static void
>   lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
> -			int idx, struct luaL_field *field)
> +			 int anchortable_index, int idx,
> +			 struct luaL_field *field)
>   {
>   	if (!cfg->encode_load_metatables)
>   		return;
> @@ -463,12 +508,148 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
>   		lua_pcall(L, 1, 1, 0);
>   		/* replace obj with the unpacked value */
>   		lua_replace(L, idx);
> -		if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
> +		if (luaL_tofield(L, cfg, anchortable_index, NULL, idx,
> +			         field) < 0)
>   			luaT_error(L);
>   	} /* else ignore lua_gettable exceptions */
>   	lua_settop(L, top); /* remove temporary objects */
>   }
>   
> +void
> +find_references(struct lua_State *L, int anchortable_index, const char *mode)
> +{
> +	int type = lua_type(L, -1);
> +	if (type != LUA_TTABLE || anchortable_index == 0)
> +		return;
> +
> +	int node_index = lua_gettop(L);
> +	lua_pushvalue(L, node_index);
> +
> +	/* Get value with flags from anchor table. */
> +	lua_pushvalue(L, node_index);
> +	lua_rawget(L, anchortable_index);
> +	if (lua_isnil(L, -1) == 1) {
> +		lua_pop(L, 1);
> +		lua_newtable(L);
> +	}
> +	int flags_index = lua_gettop(L);
> +
> +	/* Get and increment counter. */
> +	lua_pushstring(L, mode);
> +	lua_rawget(L, flags_index);
> +
> +	int new_count = 0;
> +	if (lua_isnil(L, -1) == 1) {
> +		new_count = 1;
> +		lua_pop(L, 1);
> +	} else {
> +		lua_Integer number = lua_tointeger(L, -1);
> +		lua_pop(L, 1);
> +		if (number < 2)
> +			new_count = ++number;
> +		else
> +			new_count = -1;
> +	}
> +
> +	/* Push new count to value with flags. */
> +	if (new_count == -1) {
> +		lua_pop(L, 2);
> +		assert(node_index == lua_gettop(L));
> +		return;
> +	}
> +	lua_pushstring(L, mode);
> +	lua_pushinteger(L, new_count);
> +	lua_rawset(L, flags_index);
> +
> +	/* Check if node is already serialized. */
> +	bool already_serialized = false;
> +	if (strcmp(mode, "after") == 0) {
> +		lua_pushstring(L, "serialized");
> +		lua_rawget(L, flags_index);
> +		already_serialized = lua_toboolean(L, -1);
> +		lua_pop(L, 1);
> +	}
> +	lua_rawset(L, anchortable_index);
> +	assert(node_index == lua_gettop(L));
> +	if (already_serialized == true)
> +		return;
> +
> +	/* Recursively process other table values. */
> +	lua_pushnil(L);
> +	while (lua_next(L, -2) != 0) {
> +		/* Find references on value. */
> +		find_references(L, anchortable_index, mode);
> +		lua_pop(L, 1);
> +		/* Find references on key. */
> +		find_references(L, anchortable_index, mode);
> +	}
> +}
> +
> +int
> +get_anchor(struct lua_State *L, int anchortable_index,
> +	   unsigned int *anchor_number, const char **anchor)
> +{
> +	lua_pushvalue(L, -1);
> +	lua_rawget(L, anchortable_index);
> +	if (lua_isnil(L, -1) == 1) {
> +		lua_pop(L, 1);
> +		*anchor = NULL;
> +		return 0;
> +	}
> +
> +	lua_pushstring(L, "before");
> +	lua_rawget(L, -2);
> +	const char *str = NULL;
> +	if (lua_type(L, -1) == LUA_TSTRING)
> +		str = lua_tostring(L, -1);
> +	lua_Integer number_before = lua_tointeger(L, -1);
> +	lua_pop(L, 1);
> +	lua_Integer number_after = 0;
> +	if (str == NULL) {
> +		lua_pushstring(L, "after");
> +		lua_rawget(L, -2);
> +		if (lua_type(L, -1) == LUA_TSTRING)
> +			str = lua_tostring(L, -1);
> +		number_after = lua_tointeger(L, -1);
> +		lua_pop(L, 1);
> +	}
> +
> +	*anchor = "";
> +	int ret = 0;
> +	if ((number_before > 1 || number_after > 1) && str == NULL) {
> +		/*
> +		 * This element is referenced more than once but
> +		 * has not been named.
> +		 */
> +		char buf[32];
> +		snprintf(buf, sizeof(buf), "%u", (*anchor_number)++);
> +		lua_pop(L, 1);
> +		lua_pushvalue(L, -1);
> +
> +		lua_pushvalue(L, -1);
> +		lua_rawget(L, anchortable_index);
> +		if (number_before > 1)
> +			lua_pushstring(L, "before");
> +		else
> +			lua_pushstring(L, "after");
> +		lua_pushstring(L, buf);
> +		*anchor = lua_tostring(L, -1);
> +		lua_rawset(L, -3);
> +		lua_rawset(L, anchortable_index);
> +		ret = 1;
> +	} else if (str != NULL) {
> +		/* This is an aliased element. */
> +		lua_pop(L, 1);
> +		*anchor = str;
> +		ret = 2;
> +	} else {
> +		lua_pop(L, 1);
> +		*anchor = NULL;
> +		ret = 0;
> +	}
> +	return ret;
> +}
> +
>   /**
>    * Call __serialize method of a table object by index
>    * if the former exists.
> @@ -496,11 +677,13 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
>    *      proceed with default table encoding.
>    */
>   static int
> -lua_field_try_serialize(struct lua_State *L, struct luaL_serializer *cfg,
> -			int idx, struct luaL_field *field)
> +lua_field_try_serialize(struct lua_State *L, int anchortable_index,
> +			struct luaL_serializer *cfg, int idx,
> +			struct luaL_field *field)
>   {
>   	if (luaL_getmetafield(L, idx, LUAL_SERIALIZE) == 0)
>   		return 1;
> +	mark_as_serialized(L, anchortable_index, idx);
>   	if (lua_isfunction(L, -1)) {
>   		/* copy object itself */
>   		lua_pushvalue(L, idx);
> @@ -508,7 +691,9 @@ lua_field_try_serialize(struct lua_State *L, struct luaL_serializer *cfg,
>   			diag_set(LuajitError, lua_tostring(L, -1));
>   			return -1;
>   		}
> -		if (luaL_tofield(L, cfg, NULL, -1, field) != 0)
> +		find_references(L, anchortable_index, "after");
> +		if (luaL_tofield(L, cfg, anchortable_index, NULL, -1,
> +				 field) != 0)
>   			return -1;
>   		lua_replace(L, idx);
>   		return 0;
> @@ -541,16 +726,19 @@ lua_field_try_serialize(struct lua_State *L, struct luaL_serializer *cfg,
>   }
>   
>   static int
> -lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
> -			int idx, struct luaL_field *field)
> +lua_field_inspect_table(struct lua_State *L, int anchortable_index,
> +			struct luaL_serializer *cfg, int idx,
> +			struct luaL_field *field)
>   {
>   	assert(lua_type(L, idx) == LUA_TTABLE);
>   	uint32_t size = 0;
>   	uint32_t max = 0;
>   
> -	if (cfg->encode_load_metatables) {
> +	if (cfg->encode_load_metatables &&
> +	    is_serialized(L, anchortable_index, idx) != 1) {
>   		int top = lua_gettop(L);
> -		int res = lua_field_try_serialize(L, cfg, idx, field);
> +		int res = lua_field_try_serialize(L, anchortable_index, cfg,
> +						  idx, field);
>   		if (res == -1)
>   			return -1;
>   		assert(lua_gettop(L) == top);
> @@ -612,14 +800,14 @@ lua_field_tostring(struct lua_State *L, struct luaL_serializer *cfg, int idx,
>   	lua_call(L, 1, 1);
>   	lua_replace(L, idx);
>   	lua_settop(L, top);
> -	if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
> +	if (luaL_tofield(L, cfg, 0, NULL, idx, field) < 0)
>   		luaT_error(L);
>   }
>   
>   int
>   luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
> -	     const struct serializer_opts *opts, int index,
> -	     struct luaL_field *field)
> +	     int anchortable_index, const struct serializer_opts *opts,
> +	     int index, struct luaL_field *field)
>   {
>   	if (index < 0)
>   		index = lua_gettop(L) + index + 1;
> @@ -753,7 +941,8 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
>   	case LUA_TTABLE:
>   	{
>   		field->compact = false;
> -		return lua_field_inspect_table(L, cfg, index, field);
> +		return lua_field_inspect_table(L, anchortable_index, cfg,
> +					       index, field);
>   	}
>   	case LUA_TLIGHTUSERDATA:
>   	case LUA_TUSERDATA:
> @@ -773,8 +962,8 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
>   }
>   
>   void
> -luaL_convertfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
> -		  struct luaL_field *field)
> +luaL_convertfield(struct lua_State *L, struct luaL_serializer *cfg,
> +		  int anchortable_index, int idx, struct luaL_field *field)
>   {
>   	if (idx < 0)
>   		idx = lua_gettop(L) + idx + 1;
> @@ -789,9 +978,12 @@ luaL_convertfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
>   			 */
>   			GCcdata *cd = cdataV(L->base + idx - 1);
>   			if (cd->ctypeid > CTID_CTYPEID)
> -				lua_field_inspect_ucdata(L, cfg, idx, field);
> +				lua_field_inspect_ucdata(L, cfg,
> +							 anchortable_index, idx,
> +							 field);
>   		} else if (type == LUA_TUSERDATA) {
> -			lua_field_inspect_ucdata(L, cfg, idx, field);
> +			lua_field_inspect_ucdata(L, cfg, anchortable_index, idx,
> +						 field);
>   		}
>   	}
>   
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 37531676d..ef9c70dfb 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -354,6 +354,29 @@ struct luaL_field {
>   	bool compact;                /* a flag used by YAML serializer */
>   };
>   
> +/**
> + * Find references to tables to look up self references in tables.
> + *
> + * @param L Lua stack.
> + * @param anchortable_index Index of anchor table on stack.
> + * @param mode When the function is called before or after dump
> +               function.
> + */
> +void
> +find_references(struct lua_State *L, int anchortable_index, const char *mode);
> +
> +/**
> + * Generate aliases and anchor numbers for self references.
> + *
> + * @param L Lua stack.
> + * @param anchortable_index Index of anchor table on stack.
> + * @param[out] anchor_number Ptr to the number anchors.
> + * @param[out] anchor Ptr to anchor string.
> + */
> +int
> +get_anchor(struct lua_State *L, int anchortable_index,
> +	   unsigned int *anchor_number, const char **anchor);
> +
>   /**
>    * @brief Convert a value from the Lua stack to a lua_field structure.
>    * This function is designed for use with Lua bindings and data
> @@ -388,6 +411,7 @@ struct luaL_field {
>    *
>    * @param L stack
>    * @param cfg configuration
> + * @param anchortable_index Index of anchor table on stack.
>    * @param opts the Lua serializer additional options.
>    * @param index stack index
>    * @param field conversion result
> @@ -397,8 +421,8 @@ struct luaL_field {
>    */
>   int
>   luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
> -	     const struct serializer_opts *opts, int index,
> -	     struct luaL_field *field);
> +	     int anchortable_index, const struct serializer_opts *opts,
> +	     int index, struct luaL_field *field);
>   
>   /**
>    * @brief Try to convert userdata/cdata values using defined conversion logic.
> @@ -406,18 +430,21 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
>    *
>    * @param L stack
>    * @param cfg configuration
> + * @param anchortable_index Index of anchor table on stack.
>    * @param idx stack index
>    * @param field conversion result
>    */
>   void
> -luaL_convertfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
> -		  struct luaL_field *field);
> +luaL_convertfield(struct lua_State *L, struct luaL_serializer *cfg,
> +		  int anchortable_index, int idx, struct luaL_field *field);
>   
>   /**
>    * @brief A wrapper for luaL_tofield() and luaL_convertfield() that
>    * tries to convert value or raise an error.
>    * @param L stack
>    * @param cfg configuration
> + * @param anchortable_index Stack index of table with node visit
> +                            and serialization info.
>    * @param idx stack index
>    * @param field conversion result
>    * @sa lua_tofield()
> @@ -433,14 +460,14 @@ luaL_convertfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
>    * (tostring) -> (nil) -> exception
>    */
>   static inline void
> -luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
> -		struct luaL_field *field)
> +luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg,
> +		int anchortable_index, int idx, struct luaL_field *field)
>   {
> -	if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
> +	if (luaL_tofield(L, cfg, anchortable_index, NULL, idx, field) < 0)
>   		luaT_error(L);
>   	if (field->type != MP_EXT || field->ext_type != MP_UNKNOWN_EXTENSION)
>   		return;
> -	luaL_convertfield(L, cfg, idx, field);
> +	luaL_convertfield(L, cfg, anchortable_index, idx, field);
>   }
>   
>   void
> diff --git a/test/app/gh-3228-serializer-look-for-recursion.result b/test/app/gh-3228-serializer-look-for-recursion.result
> new file mode 100644
> index 000000000..773470c8e
> --- /dev/null
> +++ b/test/app/gh-3228-serializer-look-for-recursion.result
> @@ -0,0 +1,67 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +
> +--
> +-- gh-3228: Check the error message in the case of a __serialize
> +-- function generating infinite recursion.
> +--
> +setmetatable({}, {__serialize = function(a) return a end})
> + | ---
> + | - []
> + | ...
> +setmetatable({}, {__serialize = function(a) return {a} end})
> + | ---
> + | - - []
> + | ...
> +setmetatable({}, {__serialize = function(a) return {a, a} end})
> + | ---
> + | - - &0 []
> + |   - *0
> + | ...
> +setmetatable({}, {__serialize = function(a) return {a, a, a} end})
> + | ---
> + | - - &0 []
> + |   - *0
> + |   - *0
> + | ...
> +setmetatable({}, {__serialize = function(a) return {{a, a}, a} end})
> + | ---
> + | - - - &0 []
> + |     - *0
> + |   - *0
> + | ...
> +setmetatable({}, {__serialize = function(a) return {a, 1} end})
> + | ---
> + | - - []
> + |   - 1
> + | ...
> +setmetatable({}, {__serialize = function(a) return {{{{a}}}} end})
> + | ---
> + | - - - - - []
> + | ...
> +setmetatable({}, {__serialize = function(a) return {{{{1}}}} end})
> + | ---
> + | - - - - - 1
> + | ...
> +b = {}
> + | ---
> + | ...
> +setmetatable({b}, {__serialize = function(a) return {a_1 = a, a_2 = a, b_1 = b, b_2 = b} end})
> + | ---
> + | - b_2: &0 []
> + |   a_2: &1
> + |   - *0
> + |   a_1: *1
> + |   b_1: *0
> + | ...
> +setmetatable({b}, {__serialize = function(a) return {a_1 = a, a_2 = {a, b}, b = b} end})
> + | ---
> + | - b: &0 []
> + |   a_2:
> + |   - &1
> + |     - *0
> + |   - *0
> + |   a_1: *1
> + | ...
> diff --git a/test/app/gh-3228-serializer-look-for-recursion.test.lua b/test/app/gh-3228-serializer-look-for-recursion.test.lua
> new file mode 100644
> index 000000000..1c9b0375f
> --- /dev/null
> +++ b/test/app/gh-3228-serializer-look-for-recursion.test.lua
> @@ -0,0 +1,17 @@
> +test_run = require('test_run').new()
> +
> +--
> +-- gh-3228: Check the error message in the case of a __serialize
> +-- function generating infinite recursion.
> +--
> +setmetatable({}, {__serialize = function(a) return a end})
> +setmetatable({}, {__serialize = function(a) return {a} end})
> +setmetatable({}, {__serialize = function(a) return {a, a} end})
> +setmetatable({}, {__serialize = function(a) return {a, a, a} end})
> +setmetatable({}, {__serialize = function(a) return {{a, a}, a} end})
> +setmetatable({}, {__serialize = function(a) return {a, 1} end})
> +setmetatable({}, {__serialize = function(a) return {{{{a}}}} end})
> +setmetatable({}, {__serialize = function(a) return {{{{1}}}} end})
> +b = {}
> +setmetatable({b}, {__serialize = function(a) return {a_1 = a, a_2 = a, b_1 = b, b_2 = b} end})
> +setmetatable({b}, {__serialize = function(a) return {a_1 = a, a_2 = {a, b}, b = b} end})
> diff --git a/test/swim/swim.result b/test/swim/swim.result
> index bfc83c143..539131677 100644
> --- a/test/swim/swim.result
> +++ b/test/swim/swim.result
> @@ -1322,16 +1322,13 @@ m_list
>       incarnation: cdata {generation = 0ULL, version = 1ULL}
>       uuid: 00000000-0000-1000-8000-000000000002
>       payload_size: 0
> -  - uri: 127.0.0.1:<port>
> -    status: alive
> -    incarnation: cdata {generation = 0ULL, version = 2ULL}
> -    uuid: 00000000-0000-1000-8000-000000000001
> -    payload_size: 8
> -  - uri: 127.0.0.1:<port>
> +  - &0
> +    uri: 127.0.0.1:<port>
>       status: alive
>       incarnation: cdata {generation = 0ULL, version = 2ULL}
>       uuid: 00000000-0000-1000-8000-000000000001
>       payload_size: 8
> +  - *0
>   ...
>   e_list
>   ---
> @@ -1374,16 +1371,13 @@ fiber.sleep(0)
>   -- Two events - status update to 'left', and 'drop'.
>   m_list
>   ---
> -- - uri: 127.0.0.1:<port>
> -    status: left
> -    incarnation: cdata {generation = 0ULL, version = 1ULL}
> -    uuid: 00000000-0000-1000-8000-000000000002
> -    payload_size: 0
> -  - uri: 127.0.0.1:<port>
> +- - &0
> +    uri: 127.0.0.1:<port>
>       status: left
>       incarnation: cdata {generation = 0ULL, version = 1ULL}
>       uuid: 00000000-0000-1000-8000-000000000002
>       payload_size: 0
> +  - *0
>   ...
>   e_list
>   ---
> diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
> index 38e999870..67a9762bc 100644
> --- a/third_party/lua-cjson/lua_cjson.c
> +++ b/third_party/lua-cjson/lua_cjson.c
> @@ -354,7 +354,7 @@ static void json_append_object(lua_State *l, struct luaL_serializer *cfg,
>               comma = 1;
>   
>       struct luaL_field field;
> -    luaL_checkfield(l, cfg, -2, &field);
> +    luaL_checkfield(l, cfg, 0, -2, &field);
>       if (field.type == MP_UINT) {
>           strbuf_append_char(json, '"');
>           json_append_uint(cfg, json, field.ival);
> @@ -384,7 +384,7 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg,
>                                int current_depth, strbuf_t *json)
>   {
>       struct luaL_field field;
> -    luaL_checkfield(l, cfg, -1, &field);
> +    luaL_checkfield(l, cfg, 0, -1, &field);
>       switch (field.type) {
>       case MP_UINT:
>           return json_append_uint(cfg, json, field.ival);
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 9c3a4a646..72f269e6c 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -500,38 +500,23 @@ usage_error:
>   static int dump_node(struct lua_yaml_dumper *dumper);
>   
>   static yaml_char_t *get_yaml_anchor(struct lua_yaml_dumper *dumper) {
> -   const char *s = "";
> -   lua_pushvalue(dumper->L, -1);
> -   lua_rawget(dumper->L, dumper->anchortable_index);
> -   if (!lua_toboolean(dumper->L, -1)) {
> -      lua_pop(dumper->L, 1);
> -      return NULL;
> -   }
> -
> -   if (lua_isboolean(dumper->L, -1)) {
> -      /* this element is referenced more than once but has not been named */
> -      char buf[32];
> -      snprintf(buf, sizeof(buf), "%u", dumper->anchor_number++);
> -      lua_pop(dumper->L, 1);
> -      lua_pushvalue(dumper->L, -1);
> -      lua_pushstring(dumper->L, buf);
> -      s = lua_tostring(dumper->L, -1);
> -      lua_rawset(dumper->L, dumper->anchortable_index);
> -   } else {
> -      /* this is an aliased element */
> +   const char *anchor = NULL;
> +   if (get_anchor(dumper->L, dumper->anchortable_index, &dumper->anchor_number,
> +                  &anchor) == 2) {
>         yaml_event_t ev;
> -      const char *str = lua_tostring(dumper->L, -1);
> -      if (!yaml_alias_event_initialize(&ev, (yaml_char_t *) str) ||
> +      if (!yaml_alias_event_initialize(&ev, (yaml_char_t *) anchor) ||
>             !yaml_emitter_emit(&dumper->emitter, &ev))
>            luaL_error(dumper->L, OOM_ERRMSG);
> -      lua_pop(dumper->L, 1);
> +      return (yaml_char_t *)"";
>      }
> -   return (yaml_char_t *)s;
> +   return (yaml_char_t *) anchor;
>   }
>   
> -static int dump_table(struct lua_yaml_dumper *dumper, struct luaL_field *field){
> +static int dump_table(struct lua_yaml_dumper *dumper, struct luaL_field *field,
> +                      yaml_char_t *anchor) {
>      yaml_event_t ev;
> -   yaml_char_t *anchor = get_yaml_anchor(dumper);
> +   if (anchor == NULL)
> +      anchor = get_yaml_anchor(dumper);
>   
>      if (anchor && !*anchor) return 1;
>   
> @@ -556,10 +541,12 @@ static int dump_table(struct lua_yaml_dumper *dumper, struct luaL_field *field){
>             yaml_emitter_emit(&dumper->emitter, &ev) != 0 ? 1 : 0;
>   }
>   
> -static int dump_array(struct lua_yaml_dumper *dumper, struct luaL_field *field){
> +static int dump_array(struct lua_yaml_dumper *dumper, struct luaL_field *field,
> +                      yaml_char_t *anchor) {
>      unsigned i;
>      yaml_event_t ev;
> -   yaml_char_t *anchor = get_yaml_anchor(dumper);
> +   if (anchor == NULL)
> +      anchor = get_yaml_anchor(dumper);
>   
>      if (anchor && !*anchor)
>         return 1;
> @@ -621,8 +608,18 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>      bool unused;
>      (void) unused;
>   
> +   yaml_char_t *anchor = NULL;
> +
> +   /*
> +    * Keep anchor to print it if luaL_checkfield() push new value
> +    * on stack after serialization.
> +    */
> +   if (lua_istable(dumper->L, -1) == 1)
> +      anchor = get_yaml_anchor(dumper);
> +
>      int top = lua_gettop(dumper->L);
> -   luaL_checkfield(dumper->L, dumper->cfg, top, &field);
> +   luaL_checkfield(dumper->L, dumper->cfg, dumper->anchortable_index, top,
> +                   &field);
>      switch(field.type) {
>      case MP_UINT:
>         snprintf(buf, sizeof(buf) - 1, "%" PRIu64, field.ival);
> @@ -647,9 +644,9 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>         len = strlen(buf);
>         break;
>      case MP_ARRAY:
> -      return dump_array(dumper, &field);
> +      return dump_array(dumper, &field, anchor);
>      case MP_MAP:
> -      return dump_table(dumper, &field);
> +      return dump_table(dumper, &field, anchor);
>      case MP_STR:
>         str = lua_tolstring(dumper->L, -1, &len);
>         if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) ||
> @@ -745,35 +742,6 @@ static int append_output(void *arg, unsigned char *buf, size_t len) {
>      return 1;
>   }
>   
> -static void find_references(struct lua_yaml_dumper *dumper) {
> -   int newval = -1, type = lua_type(dumper->L, -1);
> -   if (type != LUA_TTABLE)
> -      return;
> -
> -   lua_pushvalue(dumper->L, -1); /* push copy of table */
> -   lua_rawget(dumper->L, dumper->anchortable_index);
> -   if (lua_isnil(dumper->L, -1))
> -      newval = 0;
> -   else if (!lua_toboolean(dumper->L, -1))
> -      newval = 1;
> -   lua_pop(dumper->L, 1);
> -   if (newval != -1) {
> -      lua_pushvalue(dumper->L, -1);
> -      lua_pushboolean(dumper->L, newval);
> -      lua_rawset(dumper->L, dumper->anchortable_index);
> -   }
> -   if (newval)
> -      return;
> -
> -   /* recursively process other table values */
> -   lua_pushnil(dumper->L);
> -   while (lua_next(dumper->L, -2) != 0) {
> -      find_references(dumper); /* find references on value */
> -      lua_pop(dumper->L, 1);
> -      find_references(dumper); /* find references on key */
> -   }
> -}
> -
>   int
>   lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer,
>                   const char *tag_handle, const char *tag_prefix)
> @@ -819,7 +787,7 @@ lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer,
>      dumper.anchortable_index = lua_gettop(L);
>      dumper.anchor_number = 0;
>      lua_pushvalue(L, 1); /* push copy of arg we're processing */
> -   find_references(&dumper);
> +   find_references(L, dumper.anchortable_index, "before");
>      dump_document(&dumper);
>      if (dumper.error)
>         goto error;

  parent reply	other threads:[~2021-04-21 12:34 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-11  5:49 Roman Khabibov via Tarantool-patches
2021-03-14 12:42 ` Sergey Kaplun via Tarantool-patches
2021-04-05 22:05   ` Roman Khabibov via Tarantool-patches
2021-04-21  9:27     ` Sergey Kaplun via Tarantool-patches
2021-04-21 13:12       ` Sergey Bronnikov via Tarantool-patches
2021-04-21 18:20         ` Sergey Kaplun via Tarantool-patches
2021-04-13 15:54 ` Sergey Bronnikov via Tarantool-patches
2021-04-15 20:39   ` Roman Khabibov via Tarantool-patches
2021-04-21 12:34 ` Sergey Bronnikov via Tarantool-patches [this message]
2021-07-07 22:42 ` Alexander Turenko 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=67243305-a17e-0923-bbb0-322c3506841c@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=roman.habibov@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] serializer: serialize recursive structures' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox