[tarantool-patches] [PATCH v9 4/7] lua: remove exceptions from function luaL_tofield()
- From: imeevma@xxxxxxxxxxxxx
- To: v.shpilevoy@xxxxxxxxxxxxx
- Date: Fri, 22 Mar 2019 13:50:38 +0300
Hi! Thank you for review. My answers and new version below.
On 1/29/19 11:42 PM, Vladislav Shpilevoy wrote:
Hi! Thanks for the patch! See 2 comments below.
@@ -525,94 +533,95 @@ luaL_tofield(struct lua_State *L, struct
luaL_serializer *cfg, int index,
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;
+ if (lua_field_inspect_table(L, cfg, index, field) < 0)
+ return -1;
+ return 0;
1. Why not simply 'return lua_field_inspect_table' ?
Fixed.
}
case LUA_TLIGHTUSERDATA:
case LUA_TUSERDATA:
diff --git a/src/lua/utils.h b/src/lua/utils.h
index a47e3d2..fde3514 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -345,7 +348,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);
--
2.7.4
2. Looking at lua_field_inspect_table I found that if a table has __serialize
metamethod, it is called without a protection (utils.c:409). __serialize is an
arbitrary unprotected user code, that can throw an error deliberately. What
are
we gonna do with that? Personally I've already faced with some user code,
throwing
an error from __serialize, deliberately. On not critical errors not meaning
panic.
As a solution we could 1) do not care, again; 2) finally accept the fact that
wrap into a pcall was not so bad and use it; 3) use lua_pcall in that
particular place. Please, consult Kostja, what does he choose.
I checked it this way:
diff --git a/src/lua/utils.c b/src/lua/utils.c
index bbfc549..eae3b5f 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -398,6 +398,20 @@ lua_field_inspect_ucdata(struct lua_State *L, struct
luaL_serializer *cfg,
lua_settop(L, top); /* remove temporary objects */
}
+/**
+ * Check that the field LUAL_SERIALIZE of the metatable is
+ * available.
+ *
+ * @param L Lua State.
+ */
+static int
+get_metafield_serialize(struct lua_State *L)
+{
+ int idx = *(int *)lua_topointer(L, -1);
+ luaL_getmetafield(L, idx, LUAL_SERIALIZE);
+ return 0;
+}
+
static int
lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
int idx, struct luaL_field *field)
@@ -408,6 +422,9 @@ lua_field_inspect_table(struct lua_State *L, struct
luaL_serializer *cfg,
uint32_t max = 0;
/* Try to get field LUAL_SERIALIZER_TYPE from metatable */
+ if (!cfg->encode_load_metatables &&
+ luaT_cpcall(L, get_metafield_serialize, &idx) != 0)
+ return -1;
if (!cfg->encode_load_metatables ||
!luaL_getmetafield(L, idx, LUAL_SERIALIZE))
goto skip;
New version:
commit e3395eb44aee7ce47c0191291708c585de596c6a
Author: Mergen Imeev <imeevma@xxxxxxxxx>
Date: Fri Jan 11 21:16:12 2019 +0300
lua: remove exceptions from function luaL_tofield()
Before this commit, the luaL_tofield() function threw a Lua
exception when an error occurred. This behavior has been changed
in this patch. Now it sets diag_set() and returns -1 in case of
an error.
Needed for #3505
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..eae3b5f 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -392,12 +392,27 @@ 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
+/**
+ * Check that the field LUAL_SERIALIZE of the metatable is
+ * available.
+ *
+ * @param L Lua State.
+ */
+static int
+get_metafield_serialize(struct lua_State *L)
+{
+ int idx = *(int *)lua_topointer(L, -1);
+ luaL_getmetafield(L, idx, LUAL_SERIALIZE);
+ return 0;
+}
+
+static int
lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
int idx, struct luaL_field *field)
{
@@ -407,6 +422,9 @@ lua_field_inspect_table(struct lua_State *L, struct
luaL_serializer *cfg,
uint32_t max = 0;
/* Try to get field LUAL_SERIALIZER_TYPE from metatable */
+ if (!cfg->encode_load_metatables &&
+ luaT_cpcall(L, get_metafield_serialize, &idx) != 0)
+ return -1;
if (!cfg->encode_load_metatables ||
!luaL_getmetafield(L, idx, LUAL_SERIALIZE))
goto skip;
@@ -417,10 +435,10 @@ lua_field_inspect_table(struct lua_State *L, struct
luaL_serializer *cfg,
lua_call(L, 1, 1);
/* replace obj with the unpacked value */
lua_replace(L, idx);
- luaL_tofield(L, cfg, idx, field);
- return;
+ return luaL_tofield(L, cfg, idx, field);
} else if (!lua_isstring(L, -1)) {
- luaL_error(L, "invalid " LUAL_SERIALIZE " value");
+ diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
+ return -1;
}
type = lua_tostring(L, -1);
@@ -433,7 +451,7 @@ lua_field_inspect_table(struct lua_State *L, struct
luaL_serializer *cfg,
field->compact = true;
lua_pop(L, 1); /* type */
- return;
+ return 0;
} else if (strcmp(type, "map") == 0 || strcmp(type, "mapping") == 0) {
field->type = MP_MAP; /* Override type */
field->size = luaL_maplen(L, idx);
@@ -441,9 +459,10 @@ lua_field_inspect_table(struct lua_State *L, struct
luaL_serializer *cfg,
if (cfg->has_compact && type[3] == '\0')
field->compact = true;
lua_pop(L, 1); /* type */
- return;
+ return 0;
} else {
- luaL_error(L, "invalid " LUAL_SERIALIZE " value");
+ diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
+ return -1;
}
skip:
@@ -465,7 +484,7 @@ skip:
}
field->type = MP_MAP;
field->size = size;
- return;
+ return 0;
}
if (k > max)
max = k;
@@ -475,15 +494,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 +518,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 +533,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 +559,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 +653,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);
--
2.7.4
Other related posts:
