[tarantool-patches] Re: [PATCH v9 4/7] lua: remove exceptions from function luaL_tofield()

[Mergen Imeev <imeevma@tarantool.org>]

Thank you for fixes! I checked and added some more fixes:
1) I added new field is_value_returned for struct
lua_serialize_status.
2) I changed all "return 0" to "return 1" because we should return
exactly one result since we call this function through pcall.
3) I pushed a new value on stack when serialization is not used.
I think it should work even without this value, since we will
discard it, but I’m not 100% sure of that.

Diff between commits and diff of whole patch below. I haven't
squashed these two review fixes.

On Thu, Mar 28, 2019 at 09:40:00PM +0300, Vladislav Shpilevoy wrote:
> Thanks for the fixes! I've pushed my review fixes on the
> branch, and dropped at the end of the email. But I've not
> tested them, so I propose you to do that, if you agree
> with the concept.
> 
> =========================================================
> 

Diff between commits:

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 38245a9..e3b12d8 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -414,6 +414,10 @@ struct lua_serialize_status {
 	 * default serialization is used.
 	 */
 	bool is_serialize_used;
+	/**
+	 * True, if value should be returned after serialization.
+	 */
+	bool is_value_returned;
 	/** Parameters, passed originally to luaL_tofield. */
 	struct luaL_serializer *cfg;
 	struct luaL_field *field;
@@ -435,8 +439,11 @@ lua_field_try_serialize(struct lua_State *L)
 		(struct lua_serialize_status *) lua_touserdata(L, 2);
 	s->is_serialize_used = (luaL_getmetafield(L, 1, LUAL_SERIALIZE) != 0);
 	s->is_error = false;
-	if (! s->is_serialize_used)
-		return 0;
+	s->is_value_returned = false;
+	if (! s->is_serialize_used) {
+		lua_pushvalue(L, 1);
+		return 1;
+	}
 	struct luaL_serializer *cfg = s->cfg;
 	struct luaL_field *field = s->field;
 	if (lua_isfunction(L, -1)) {
@@ -444,12 +451,13 @@ lua_field_try_serialize(struct lua_State *L)
 		lua_pushvalue(L, 1);
 		lua_call(L, 1, 1);
 		s->is_error = (luaL_tofield(L, cfg, -1, field) != 0);
-		return s->is_error ? 0 : 1;
+		s->is_value_returned = true;
+		return 1;
 	}
 	if (!lua_isstring(L, -1)) {
 		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
 		s->is_error = true;
-		return 0;
+		return 1;
 	}
 	const char *type = lua_tostring(L, -1);
 	if (strcmp(type, "array") == 0 || strcmp(type, "seq") == 0 ||
@@ -459,21 +467,17 @@ lua_field_try_serialize(struct lua_State *L)
 		/* YAML: use flow mode if __serialize == 'seq' */
 		if (cfg->has_compact && type[3] == '\0')
 			field->compact = true;
-		lua_pop(L, 1); /* type */
-		return 1;
 	} else if (strcmp(type, "map") == 0 || strcmp(type, "mapping") == 0) {
 		field->type = MP_MAP;   /* Override type */
 		field->size = luaL_maplen(L, 1);
 		/* YAML: use flow mode if __serialize == 'map' */
 		if (cfg->has_compact && type[3] == '\0')
 			field->compact = true;
-		lua_pop(L, 1); /* type */
-		return 1;
 	} else {
 		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
 		s->is_error = true;
-		return 0;
 	}
+	return 1;
 }
 
 static int
@@ -497,10 +501,12 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
 		}
 		if (s.is_error)
 			return -1;
-		if (s.is_serialize_used) {
+		if (s.is_serialize_used && s.is_value_returned)
 			lua_replace(L, idx);
+		else
+			lua_pop(L, 1);
+		if (s.is_serialize_used)
 			return 0;
-		}
 	}
 
 	field->type = MP_ARRAY;




Diff of whole patch:

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 52939ae..2049ee5 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -164,7 +164,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
 		 */
 		for (int i = 1; i <= nrets; ++i) {
 			struct luaL_field field;
-			luaL_tofield(L, cfg, i, &field);
+			if (luaL_tofield(L, cfg, i, &field) < 0)
+				return luaT_error(L);
 			struct tuple *tuple;
 			if (field.type == MP_EXT &&
 			    (tuple = luaT_istuple(L, i)) != NULL) {
@@ -192,7 +193,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
 	 * Inspect the first result
 	 */
 	struct luaL_field root;
-	luaL_tofield(L, cfg, 1, &root);
+	if (luaL_tofield(L, cfg, 1, &root) < 0)
+		return luaT_error(L);
 	struct tuple *tuple;
 	if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
 		/* `return box.tuple()` */
@@ -221,7 +223,8 @@ 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;
-		luaL_tofield(L, cfg, -1, &field);
+		if (luaL_tofield(L, cfg, -1, &field) < 0)
+			return luaT_error(L);
 		if (field.type == MP_EXT && (tuple = luaT_istuple(L, -1))) {
 			tuple_to_mpstream(tuple, stream);
 		} else if (field.type != MP_ARRAY) {
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 60c1a39..1903ee8 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -229,7 +229,8 @@ luamp_convert_key(struct lua_State *L, struct luaL_serializer *cfg,
 		return tuple_to_mpstream(tuple, stream);
 
 	struct luaL_field field;
-	luaL_tofield(L, cfg, index, &field);
+	if (luaL_tofield(L, cfg, index, &field) < 0)
+		luaT_error(L);
 	if (field.type == MP_ARRAY) {
 		lua_pushvalue(L, index);
 		luamp_encode_r(L, cfg, stream, &field, 0);
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index b470060..1b1874e 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -149,10 +149,12 @@ restart: /* used by MP_EXT */
 		lua_pushnil(L);  /* first key */
 		while (lua_next(L, top) != 0) {
 			lua_pushvalue(L, -2); /* push a copy of key to top */
-			luaL_tofield(L, cfg, lua_gettop(L), field);
+			if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+				return luaT_error(L);
 			luamp_encode_r(L, cfg, stream, field, level + 1);
 			lua_pop(L, 1); /* pop a copy of key */
-			luaL_tofield(L, cfg, lua_gettop(L), field);
+			if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+				return luaT_error(L);
 			luamp_encode_r(L, cfg, stream, field, level + 1);
 			lua_pop(L, 1); /* pop value */
 		}
@@ -168,7 +170,8 @@ restart: /* used by MP_EXT */
 		mpstream_encode_array(stream, size);
 		for (uint32_t i = 0; i < size; i++) {
 			lua_rawgeti(L, top, i + 1);
-			luaL_tofield(L, cfg, top + 1, field);
+			if (luaL_tofield(L, cfg, top + 1, field) < 0)
+				return luaT_error(L);
 			luamp_encode_r(L, cfg, stream, field, level + 1);
 			lua_pop(L, 1);
 		}
@@ -203,7 +206,8 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
 	}
 
 	struct luaL_field field;
-	luaL_tofield(L, cfg, lua_gettop(L), &field);
+	if (luaL_tofield(L, cfg, lua_gettop(L), &field) < 0)
+		return luaT_error(L);
 	enum mp_type top_type = luamp_encode_r(L, cfg, stream, &field, 0);
 
 	if (!on_top) {
diff --git a/src/lua/utils.c b/src/lua/utils.c
index a418b95..e3b12d8 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -392,61 +392,123 @@ 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);
-		luaL_tofield(L, cfg, idx, field);
+		if (luaL_tofield(L, cfg, idx, field) < 0)
+			luaT_error(L);
 	} /* else ignore lua_gettable exceptions */
 	lua_settop(L, top); /* remove temporary objects */
 }
 
-static void
-lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
-			int idx, struct luaL_field *field)
-{
-	assert(lua_type(L, idx) == LUA_TTABLE);
-	const char *type;
-	uint32_t size = 0;
-	uint32_t max = 0;
-
-	/* Try to get field LUAL_SERIALIZER_TYPE from metatable */
-	if (!cfg->encode_load_metatables ||
-	    !luaL_getmetafield(L, idx, LUAL_SERIALIZE))
-		goto skip;
+/**
+ * A helper structure to simplify safe call of __serialize method.
+ * It passes some arguments into the serializer called via pcall,
+ * and carries out some results.
+ */
+struct lua_serialize_status {
+	/**
+	 * True if an attempt to call __serialize has failed. A
+	 * diag message is set.
+	 */
+	bool is_error;
+	/**
+	 * True, if __serialize exists. Otherwise an ordinary
+	 * default serialization is used.
+	 */
+	bool is_serialize_used;
+	/**
+	 * True, if value should be returned after serialization.
+	 */
+	bool is_value_returned;
+	/** Parameters, passed originally to luaL_tofield. */
+	struct luaL_serializer *cfg;
+	struct luaL_field *field;
+};
 
+/**
+ * Call __serialize method of a table object if the former exists.
+ * The function expects 2 values pushed onto the Lua stack: a
+ * value to serialize, and a pointer at a struct
+ * lua_serialize_status object. If __serialize exists and is
+ * correct, then one value is pushed as a result of serialization.
+ * Otherwise is_error and is_serialize_used attributes of status
+ * should be checked.
+ */
+static int
+lua_field_try_serialize(struct lua_State *L)
+{
+	struct lua_serialize_status *s =
+		(struct lua_serialize_status *) lua_touserdata(L, 2);
+	s->is_serialize_used = (luaL_getmetafield(L, 1, LUAL_SERIALIZE) != 0);
+	s->is_error = false;
+	s->is_value_returned = false;
+	if (! s->is_serialize_used) {
+		lua_pushvalue(L, 1);
+		return 1;
+	}
+	struct luaL_serializer *cfg = s->cfg;
+	struct luaL_field *field = s->field;
 	if (lua_isfunction(L, -1)) {
 		/* copy object itself */
-		lua_pushvalue(L, idx);
+		lua_pushvalue(L, 1);
 		lua_call(L, 1, 1);
-		/* replace obj with the unpacked value */
-		lua_replace(L, idx);
-		luaL_tofield(L, cfg, idx, field);
-		return;
-	} else if (!lua_isstring(L, -1)) {
-		luaL_error(L, "invalid " LUAL_SERIALIZE  " value");
+		s->is_error = (luaL_tofield(L, cfg, -1, field) != 0);
+		s->is_value_returned = true;
+		return 1;
 	}
-
-	type = lua_tostring(L, -1);
+	if (!lua_isstring(L, -1)) {
+		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
+		s->is_error = true;
+		return 1;
+	}
+	const char *type = lua_tostring(L, -1);
 	if (strcmp(type, "array") == 0 || strcmp(type, "seq") == 0 ||
 	    strcmp(type, "sequence") == 0) {
 		field->type = MP_ARRAY; /* Override type */
-		field->size = luaL_arrlen(L, idx);
+		field->size = luaL_arrlen(L, 1);
 		/* YAML: use flow mode if __serialize == 'seq' */
 		if (cfg->has_compact && type[3] == '\0')
 			field->compact = true;
-		lua_pop(L, 1); /* type */
-
-		return;
 	} else if (strcmp(type, "map") == 0 || strcmp(type, "mapping") == 0) {
 		field->type = MP_MAP;   /* Override type */
-		field->size = luaL_maplen(L, idx);
+		field->size = luaL_maplen(L, 1);
 		/* YAML: use flow mode if __serialize == 'map' */
 		if (cfg->has_compact && type[3] == '\0')
 			field->compact = true;
-		lua_pop(L, 1); /* type */
-		return;
 	} else {
-		luaL_error(L, "invalid " LUAL_SERIALIZE "  value");
+		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
+		s->is_error = true;
+	}
+	return 1;
+}
+
+static int
+lua_field_inspect_table(struct lua_State *L, 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) {
+		struct lua_serialize_status s;
+		s.cfg = cfg;
+		s.field = field;
+		lua_pushcfunction(L, lua_field_try_serialize);
+		lua_pushvalue(L, idx);
+		lua_pushlightuserdata(L, &s);
+		if (lua_pcall(L, 2, 1, 0) != 0) {
+			diag_set(LuajitError, lua_tostring(L, -1));
+			return -1;
+		}
+		if (s.is_error)
+			return -1;
+		if (s.is_serialize_used && s.is_value_returned)
+			lua_replace(L, idx);
+		else
+			lua_pop(L, 1);
+		if (s.is_serialize_used)
+			return 0;
 	}
 
-skip:
 	field->type = MP_ARRAY;
 
 	/* Calculate size and check that table can represent an array */
@@ -465,7 +527,7 @@ skip:
 			}
 			field->type = MP_MAP;
 			field->size = size;
-			return;
+			return 0;
 		}
 		if (k > max)
 			max = k;
@@ -475,15 +537,18 @@ skip:
 	if (cfg->encode_sparse_ratio > 0 &&
 	    max > size * (uint32_t)cfg->encode_sparse_ratio &&
 	    max > (uint32_t)cfg->encode_sparse_safe) {
-		if (!cfg->encode_sparse_convert)
-			luaL_error(L, "excessively sparse array");
+		if (!cfg->encode_sparse_convert) {
+			diag_set(LuajitError, "excessively sparse array");
+			return -1;
+		}
 		field->type = MP_MAP;
 		field->size = size;
-		return;
+		return 0;
 	}
 
 	assert(field->type == MP_ARRAY);
 	field->size = max;
+	return 0;
 }
 
 static void
@@ -496,12 +561,13 @@ 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);
-	luaL_tofield(L, cfg, idx, field);
+	if (luaL_tofield(L, cfg, idx, field) < 0)
+		luaT_error(L);
 }
 
-void
+int
 luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
-		 struct luaL_field *field)
+	     struct luaL_field *field)
 {
 	if (index < 0)
 		index = lua_gettop(L) + index + 1;
@@ -510,10 +576,12 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
 	double intpart;
 	size_t size;
 
-#define CHECK_NUMBER(x) ({\
-	if (!isfinite(x) && !cfg->encode_invalid_numbers) {		\
-		if (!cfg->encode_invalid_as_nil)				\
-			luaL_error(L, "number must not be NaN or Inf");		\
+#define CHECK_NUMBER(x) ({							\
+	if (!isfinite(x) && !cfg->encode_invalid_numbers) {			\
+		if (!cfg->encode_invalid_as_nil) {				\
+			diag_set(LuajitError, "number must not be NaN or Inf");	\
+			return -1;						\
+		}								\
 		field->type = MP_NIL;						\
 	}})
 
@@ -534,94 +602,93 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
 			field->dval = num;
 			CHECK_NUMBER(num);
 		}
-		return;
+		return 0;
 	case LUA_TCDATA:
 	{
-		uint32_t ctypeid = 0;
-		void *cdata = luaL_checkcdata(L, index, &ctypeid);
+		GCcdata *cd = cdataV(L->base + index - 1);
+		void *cdata = (void *)cdataptr(cd);
+
 		int64_t ival;
-		switch (ctypeid) {
+		switch (cd->ctypeid) {
 		case CTID_BOOL:
 			field->type = MP_BOOL;
 			field->bval = *(bool*) cdata;
-			return;
+			return 0;
 		case CTID_CCHAR:
 		case CTID_INT8:
 			ival = *(int8_t *) cdata;
 			field->type = (ival >= 0) ? MP_UINT : MP_INT;
 			field->ival = ival;
-			return;
+			return 0;
 		case CTID_INT16:
 			ival = *(int16_t *) cdata;
 			field->type = (ival >= 0) ? MP_UINT : MP_INT;
 			field->ival = ival;
-			return;
+			return 0;
 		case CTID_INT32:
 			ival = *(int32_t *) cdata;
 			field->type = (ival >= 0) ? MP_UINT : MP_INT;
 			field->ival = ival;
-			return;
+			return 0;
 		case CTID_INT64:
 			ival = *(int64_t *) cdata;
 			field->type = (ival >= 0) ? MP_UINT : MP_INT;
 			field->ival = ival;
-			return;
+			return 0;
 		case CTID_UINT8:
 			field->type = MP_UINT;
 			field->ival = *(uint8_t *) cdata;
-			return;
+			return 0;
 		case CTID_UINT16:
 			field->type = MP_UINT;
 			field->ival = *(uint16_t *) cdata;
-			return;
+			return 0;
 		case CTID_UINT32:
 			field->type = MP_UINT;
 			field->ival = *(uint32_t *) cdata;
-			return;
+			return 0;
 		case CTID_UINT64:
 			field->type = MP_UINT;
 			field->ival = *(uint64_t *) cdata;
-			return;
+			return 0;
 		case CTID_FLOAT:
 			field->type = MP_FLOAT;
 			field->fval = *(float *) cdata;
 			CHECK_NUMBER(field->fval);
-			return;
+			return 0;
 		case CTID_DOUBLE:
 			field->type = MP_DOUBLE;
 			field->dval = *(double *) cdata;
 			CHECK_NUMBER(field->dval);
-			return;
+			return 0;
 		case CTID_P_CVOID:
 		case CTID_P_VOID:
 			if (*(void **) cdata == NULL) {
 				field->type = MP_NIL;
-				return;
+				return 0;
 			}
 			/* Fall through */
 		default:
 			field->type = MP_EXT;
-			return;
 		}
-		return;
+		return 0;
 	}
 	case LUA_TBOOLEAN:
 		field->type = MP_BOOL;
 		field->bval = lua_toboolean(L, index);
-		return;
+		return 0;
 	case LUA_TNIL:
 		field->type = MP_NIL;
-		return;
+		return 0;
 	case LUA_TSTRING:
 		field->sval.data = lua_tolstring(L, index, &size);
 		field->sval.len = (uint32_t) size;
 		field->type = MP_STR;
-		return;
+		return 0;
 	case LUA_TTABLE:
 	{
 		field->compact = false;
-		lua_field_inspect_table(L, cfg, index, field);
-		return;
+		return lua_field_inspect_table(L, cfg, index, field);
 	}
 	case LUA_TLIGHTUSERDATA:
 	case LUA_TUSERDATA:
@@ -629,14 +696,14 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
 		field->sval.len = 0;
 		if (lua_touserdata(L, index) == NULL) {
 			field->type = MP_NIL;
-			return;
+			return 0;
 		}
 		/* Fall through */
 	default:
 		field->type = MP_EXT;
-		return;
 	}
 #undef CHECK_NUMBER
+	return 0;
 }
 
 void
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 96311b7..df4d50e 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -311,8 +311,11 @@ struct luaL_field {
  * @param cfg configuration
  * @param index stack index
  * @param field conversion result
+ *
+ * @retval  0 Success.
+ * @retval -1 Error.
  */
-void
+int
 luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
 	     struct luaL_field *field);
 
@@ -352,7 +355,8 @@ static inline void
 luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
 		struct luaL_field *field)
 {
-	luaL_tofield(L, cfg, idx, field);
+	if (luaL_tofield(L, cfg, idx, field) < 0)
+		luaT_error(L);
 	if (field->type != MP_EXT)
 		return;
 	luaL_convertfield(L, cfg, idx, field);