Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org,
	Alexander Turenko <alexander.turenko@tarantool.org>
Subject: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()
Date: Sun, 11 Oct 2020 15:57:38 +0300	[thread overview]
Message-ID: <e8afad701131cb592e113abf83ca83f5bd12cfc6.1602420460.git.alexander.turenko@tarantool.org> (raw)
In-Reply-To: <cover.1602420460.git.alexander.turenko@tarantool.org>

This change fixes incorrect behaviour at tuple serialization error in
several places: <space_object>:frommap(), <key_def_object>:compare(),
<merge_source>:select(). See more in #5382.

Disallow creating a tuple from objects on the Lua stack (idx == 0) in
luaT_tuple_new() for simplicity. There are no such usages in tarantool.
The function is not exposed yet to the module API. This is only
necessary in box.tuple.new(), which anyway raises Lua errors by its
contract.

The better way to implement it would be rewritting of serialization from
Lua to msgpack without raising Lua errors, but it is labirous work. Some
day, I hope, I'll return here.

Part of #5273
Fixes #5382
---
 src/box/lua/tuple.c             | 118 ++++++++++++++++++++++++--------
 src/box/lua/tuple.h             |   7 +-
 test/unit/luaT_tuple_new.c      |  42 +++++++++---
 test/unit/luaT_tuple_new.result |   6 +-
 4 files changed, 127 insertions(+), 46 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index ed97c85e4..18cfef979 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
 
 uint32_t CTID_STRUCT_TUPLE_REF;
 
+int luaT_tuple_encode_table_ref = LUA_NOREF;
+
 box_tuple_t *
 luaT_checktuple(struct lua_State *L, int idx)
 {
@@ -98,39 +100,79 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
+/**
+ * Encode a Lua values on a Lua stack as an MsgPack array.
+ *
+ * Raise a Lua error when encoding fails.
+ */
+static int
+luaT_tuple_encode_values(struct lua_State *L)
+{
+	struct ibuf *buf = tarantool_lua_ibuf;
+	ibuf_reset(buf);
+	struct mpstream stream;
+	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
+		      L);
+	int argc = lua_gettop(L);
+	mpstream_encode_array(&stream, argc);
+	for (int k = 1; k <= argc; ++k) {
+		luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
+	}
+	mpstream_flush(&stream);
+	return 0;
+}
+
+/**
+ * Encode a Lua table or a tuple as MsgPack.
+ *
+ * Raise a Lua error when encoding fails.
+ *
+ * It is a kind of critical section to be run under luaT_call().
+ */
+static int
+luaT_tuple_encode_table(struct lua_State *L)
+{
+	struct ibuf *buf = tarantool_lua_ibuf;
+	ibuf_reset(buf);
+	struct mpstream stream;
+	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
+		      L);
+	luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
+	mpstream_flush(&stream);
+	return 0;
+}
+
 static char *
 luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
 			      size_t *tuple_len_ptr)
 {
-	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
+	assert(idx != 0);
+	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
 		diag_set(IllegalParams, "A tuple or a table expected, got %s",
 			 lua_typename(L, lua_type(L, idx)));
 		return NULL;
 	}
 
-	struct ibuf *buf = tarantool_lua_ibuf;
-	ibuf_reset(buf);
-	struct mpstream stream;
-	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
-		      luamp_error, L);
-	if (idx == 0) {
-		/*
-		 * Create the tuple from lua stack
-		 * objects.
-		 */
-		int argc = lua_gettop(L);
-		mpstream_encode_array(&stream, argc);
-		for (int k = 1; k <= argc; ++k) {
-			luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
-		}
-	} else {
-		/* Create the tuple from a Lua table. */
-		luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
-	}
-	mpstream_flush(&stream);
+	int top = lua_gettop(L);
+
+	/* Calculate absolute value in the stack. */
+	if (idx < 0)
+		idx = top + idx + 1;
+
+	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
+	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
+	assert(lua_isfunction(L, -1));
+
+	lua_pushvalue(L, idx);
+
+	int rc = luaT_call(L, 1, 0);
+	lua_settop(L, top);
+	if (rc != 0)
+		return NULL;
+
 	if (tuple_len_ptr != NULL)
-		*tuple_len_ptr = ibuf_used(buf);
-	return buf->buf;
+		*tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);
+	return tarantool_lua_ibuf->buf;
 }
 
 struct tuple *
@@ -156,15 +198,30 @@ lbox_tuple_new(lua_State *L)
 		lua_newtable(L); /* create an empty tuple */
 		++argc;
 	}
+
+	box_tuple_format_t *fmt = box_tuple_format_default();
+
 	/*
 	 * Use backward-compatible parameters format:
-	 * box.tuple.new(1, 2, 3) (idx == 0), or the new one:
-	 * box.tuple.new({1, 2, 3}) (idx == 1).
+	 * box.tuple.new(1, 2, 3).
 	 */
-	int idx = argc == 1 && (lua_istable(L, 1) ||
-		luaT_istuple(L, 1));
-	box_tuple_format_t *fmt = box_tuple_format_default();
-	struct tuple *tuple = luaT_tuple_new(L, idx, fmt);
+	if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
+		struct ibuf *buf = tarantool_lua_ibuf;
+		luaT_tuple_encode_values(L); /* may raise */
+		struct tuple *tuple = box_tuple_new(fmt, buf->buf,
+						    buf->buf + ibuf_used(buf));
+		ibuf_reinit(buf);
+		if (tuple == NULL)
+			return luaT_error(L);
+		luaT_pushtuple(L, tuple);
+		return 1;
+	}
+
+	/*
+	 * Use the new parameters format:
+	 * box.tuple.new({1, 2, 3}).
+	 */
+	struct tuple *tuple = luaT_tuple_new(L, 1, fmt);
 	if (tuple == NULL)
 		return luaT_error(L);
 	/* box_tuple_new() doesn't leak on exception, see public API doc */
@@ -593,4 +650,7 @@ box_lua_tuple_init(struct lua_State *L)
 	(void) rc;
 	CTID_STRUCT_TUPLE_REF = luaL_ctypeid(L, "struct tuple &");
 	assert(CTID_STRUCT_TUPLE_REF != 0);
+
+	lua_pushcfunction(L, luaT_tuple_encode_table);
+	luaT_tuple_encode_table_ref = luaL_ref(L, LUA_REGISTRYINDEX);
 }
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index fd280f565..6787d1afe 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -82,11 +82,8 @@ luaT_istuple(struct lua_State *L, int idx);
 /** \endcond public */
 
 /**
- * Create a new tuple with specific format from a Lua table, a
- * tuple, or objects on the lua stack.
- *
- * Set idx to zero to create the new tuple from objects on the lua
- * stack.
+ * Create a new tuple with specific format from a Lua table or a
+ * tuple.
  *
  * In case of an error set a diag and return NULL.
  */
diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c
index 8f25c8e07..09a81cabe 100644
--- a/test/unit/luaT_tuple_new.c
+++ b/test/unit/luaT_tuple_new.c
@@ -51,20 +51,20 @@ check_tuple(struct tuple *tuple, box_tuple_format_t *format,
 
 void
 check_error(struct lua_State *L, struct tuple *tuple, int retvals,
+	    const struct type_info *error_type, const char *exp_err,
 	    const char *case_name)
 {
-	const char *exp_err = "A tuple or a table expected, got number";
 	is(tuple, NULL, "%s: tuple == NULL", case_name);
 	is(retvals, 0, "%s: check retvals count", case_name);
 	struct error *e = diag_last_error(diag_get());
-	is(e->type, &type_IllegalParams, "%s: check error type", case_name);
+	is(e->type, error_type, "%s: check error type", case_name);
 	ok(!strcmp(e->errmsg, exp_err), "%s: check error message", case_name);
 }
 
 int
 test_basic(struct lua_State *L)
 {
-	plan(19);
+	plan(23);
 	header();
 
 	int top;
@@ -106,14 +106,12 @@ test_basic(struct lua_State *L)
 	assert(lua_gettop(L) == 0);
 
 	/*
-	 * Case: elements on the stack (idx == 0) as an input and
-	 * a non-default format.
+	 * Case: a non-default format (a Lua table on idx == -1).
 	 */
 
 	/* Prepare the Lua stack. */
-	lua_pushinteger(L, 1);
-	lua_pushinteger(L, 2);
-	lua_pushinteger(L, 3);
+	luaL_loadstring(L, "return {1, 2, 3}");
+	lua_call(L, 0, 1);
 
 	/* Create a new format. */
 	struct key_part_def part;
@@ -130,12 +128,12 @@ test_basic(struct lua_State *L)
 
 	/* Create and check a tuple. */
 	top = lua_gettop(L);
-	tuple = luaT_tuple_new(L, 0, another_format);
+	tuple = luaT_tuple_new(L, -1, another_format);
 	check_tuple(tuple, another_format, lua_gettop(L) - top, "objects");
 
 	/* Clean up. */
 	tuple_format_delete(another_format);
-	lua_pop(L, 3);
+	lua_pop(L, 1);
 	assert(lua_gettop(L) == 0);
 
 	/*
@@ -148,7 +146,28 @@ test_basic(struct lua_State *L)
 	/* Try to create and check for the error. */
 	top = lua_gettop(L);
 	tuple = luaT_tuple_new(L, -1, default_format);
-	check_error(L, tuple, lua_gettop(L) - top, "unexpected type");
+	check_error(L, tuple, lua_gettop(L) - top, &type_IllegalParams,
+		    "A tuple or a table expected, got number",
+		    "unexpected type");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: unserializable item within a Lua table.
+	 *
+	 * The function should not raise a Lua error.
+	 */
+	luaL_loadstring(L, "return {function() end}");
+	lua_call(L, 0, 1);
+
+	/* Try to create and check for the error. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, -1, default_format);
+	check_error(L, tuple, lua_gettop(L) - top, &type_LuajitError,
+		    "unsupported Lua type 'function'",
+		    "unserializable element");
 
 	/* Clean up. */
 	lua_pop(L, 1);
@@ -170,6 +189,7 @@ main()
 	luaL_openlibs(L);
 
 	box_init();
+	tarantool_lua_error_init(L);
 	luaopen_msgpack(L);
 	box_lua_tuple_init(L);
 	lua_pop(L, 1);
diff --git a/test/unit/luaT_tuple_new.result b/test/unit/luaT_tuple_new.result
index 110aa68c2..8f3407130 100644
--- a/test/unit/luaT_tuple_new.result
+++ b/test/unit/luaT_tuple_new.result
@@ -1,4 +1,4 @@
-1..19
+1..23
 	*** test_basic ***
 ok 1 - table: tuple != NULL
 ok 2 - table: check tuple format id
@@ -19,4 +19,8 @@ ok 16 - unexpected type: tuple == NULL
 ok 17 - unexpected type: check retvals count
 ok 18 - unexpected type: check error type
 ok 19 - unexpected type: check error message
+ok 20 - unserializable element: tuple == NULL
+ok 21 - unserializable element: check retvals count
+ok 22 - unserializable element: check error type
+ok 23 - unserializable element: check error message
 	*** test_basic: done ***
-- 
2.25.0

  parent reply	other threads:[~2020-10-11 12:57 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-11 12:57 [Tarantool-patches] [PATCH v2 00/15] RFC: module api: extend for external key_def Lua module Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 01/15] module api: get rid of typedef redefinitions Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 02/15] module api: expose box region Alexander Turenko
2020-10-11 15:26   ` Vladislav Shpilevoy
2020-10-12  6:07     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 03/15] module api/lua: add luaL_iscdata() function Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 04/15] lua: factor out tuple encoding from luaT_tuple_new Alexander Turenko
2020-10-11 12:57 ` Alexander Turenko [this message]
2020-10-11 15:25   ` [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new() Vladislav Shpilevoy
2020-10-12 10:37     ` Alexander Turenko
2020-10-12 13:34       ` Timur Safin
2020-10-14 23:41       ` Vladislav Shpilevoy
2020-10-15 19:43         ` Alexander Turenko
2020-10-15 22:10           ` Vladislav Shpilevoy
2020-10-11 17:47   ` Igor Munkin
2020-10-11 18:08     ` Igor Munkin
2020-10-12 10:37     ` Alexander Turenko
2020-10-12 10:51       ` Igor Munkin
2020-10-12 18:41         ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 06/15] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12 10:35     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 07/15] module api/lua: expose luaT_tuple_new() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12  6:11     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 08/15] module api/lua: add API_EXPORT to tuple functions Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 09/15] module api: add API_EXPORT to key_def functions Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 10/15] module api: add box_key_def_new_v2() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12  7:21     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 11/15] module api: add box_key_def_dump_parts() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12  6:50     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 12/15] module api: expose box_key_def_validate_tuple() Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 13/15] WIP: module api: expose box_key_def_merge() Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 14/15] WIP: module api: expose box_key_def_extract_key() Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 15/15] WIP: module api: add box_key_def_validate_key() Alexander Turenko

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=e8afad701131cb592e113abf83ca83f5bd12cfc6.1602420460.git.alexander.turenko@tarantool.org \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 05/15] lua: don'\''t raise a Lua error from luaT_tuple_new()' \
    /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