Re: [Tarantool-patches] [PATCH v5 01/52] sql: enhance vdbe_decode_msgpack_into_mem()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sorry, I forgot to add name of the new branch and link to issues that will be
closed by this patch. Here they are:
https://github.com/tarantool/tarantool/tree/imeevma/gh-5818-encapsulate-mem-type-checking-and-changing-reviewed
https://github.com/tarantool/tarantool/issues/5011
https://github.com/tarantool/tarantool/issues/5704
https://github.com/tarantool/tarantool/issues/5913


On Tue, Apr 13, 2021 at 03:01:18PM +0300, Mergen Imeev wrote:
> Thank you for the review! I'a sorry to ask you so late, but could you look at
> new version of the patch? It was decided that this patch should be pushed to
> 2.6 and 2.7 along with master. Only this patch of the patch-set.
> 
> Diff and new version below. Please note, that I added additional SELECT to
> the first test since currently sql-tap test system does not support binary
> result. Also, in the last two tests I created new table for each since this way
> we could see proper error. If I used space T there then in the last test there
> would be the same error as in the test 5 due to UUID being before DECIMAL in
> space format. To remove dependence of their position, I added two new spaces.
> 
> On Sun, Apr 11, 2021 at 07:42:22PM +0200, Vladislav Shpilevoy wrote:
> > Hi! Good job on the patch!
> > 
> > On 09.04.2021 18:51, Mergen Imeev via Tarantool-patches wrote:
> > > Hi! Thank you for the review! My answer and new patch below. I didn't include
> > > diffs in answers since due to merge conflicts they are partly useless.
> > > 
> > > On 30.03.2021 01:57, Vladislav Shpilevoy wrote:
> > >> Hi! I appreciate the work you did here!
> > >>
> > >> Truly huge patch. But it does the important thing which I think
> > >> should give a huge help in the task of SQL code rework.
> > >>
> > >> On 23.03.2021 10:34, Mergen Imeev via Tarantool-patches wrote:
> > >>> Currently, vdbe_decode_msgpack_into_mem() creates a MEM that is not
> > >>> properly initialized in case msgpack contains MP_EXT, MP_MAP, or
> > >>> MP_ARRAY fields. Also, it doesn't set field_type.
> > >>
> > >> AFAIR, field type wasn't set deliberately. Because we use it only for
> > >> strictly typed values obtained from formatted space fields. It wasn't
> > >> applied to plain non-formatted values on purpose.
> > >>
> > >> Why do you change that?
> > > 
> > > I didn't know about that. I thought that all MEMs that contains data should have
> > > field_type set. However, I tried to understand where did this field come from
> > > and found that it was added for two purposes: to show difference between NUMBER
> > > and INTEGER in MEM before DOUBLE was added and to show column name instead of
> > > type determined from MP-type in typeof(). I believe that both these purposes are
> > > not needed now and that this field should be removed from struct MEM. I created
> > > an issue for this: #5957. However, I was prohibited to remove this field for now
> > > by @tsafin, who believes that this field is actually important.
> > 
> > I tend to agree, that it must be done separately. Although probably could be done
> > before this patchset to make it kind of simpler. Anyway, thanks for the
> > explanation.
> 
> 
> Diff:
> 
> 
> diff --git a/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua
> new file mode 100755
> index 000000000..60978c2b5
> --- /dev/null
> +++ b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua
> @@ -0,0 +1,99 @@
> +#!/usr/bin/env tarantool
> +local test = require("sqltester")
> +test:plan(6)
> +
> +local uuid = require("uuid").fromstr("11111111-1111-1111-1111-111111111111")
> +local decimal = require("decimal").new(111.111)
> +
> +box.schema.create_space('T')
> +box.space.T:format({{name = "I", type = "integer"}, {name = "U", type = "uuid"},
> +                    {name = "D", type = "decimal"}})
> +box.space.T:create_index("primary")
> +box.space.T:insert({1, uuid, decimal})
> +
> +--
> +-- Make sure that there is no segmentation fault on select from field that
> +-- contains UUID or DECIMAL. Currently SQL does not support UUID and DECIMAL,
> +-- so they treated as VARBINARY.
> +--
> +test:do_execsql_test(
> +    "gh-5913-1",
> +    [[
> +        SELECT i, u, d FROM t;
> +        SELECT i from t;
> +    ]], {
> +        1
> +    })
> +
> +box.schema.create_space('T1')
> +box.space.T1:format({{name = "I", type = "integer"},
> +                     {name = "U", type = "uuid", is_nullable = true},
> +                     {name = "D", type = "decimal", is_nullable = true}})
> +box.space.T1:create_index("primary")
> +
> +--
> +-- Since SQL does not support UUID and DECIMAL and they treated as VARBINARY,
> +-- they cannot be inserted from SQL.
> +--
> +test:do_catchsql_test(
> +    "gh-5913-2",
> +    [[
> +        INSERT INTO t1 SELECT i, u, NULL FROM t;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to uuid"
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5913-3",
> +    [[
> +        INSERT INTO t1 SELECT i, NULL, d FROM t;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to decimal"
> +    })
> +
> +--
> +-- Still, if UUID or DECIMAL fields does not selected directly, insert is
> +-- working properly.
> +--
> +test:do_execsql_test(
> +    "gh-5913-4",
> +    [[
> +        INSERT INTO t1 SELECT * FROM t;
> +        SELECT count() FROM t1;
> +    ]], {
> +        1
> +    })
> +
> +box.schema.create_space('TU')
> +box.space.TU:format({{name = "I", type = "integer"},
> +                     {name = "U", type = "uuid"}})
> +box.space.TU:create_index("primary")
> +box.space.TU:insert({1, uuid})
> +
> +box.schema.create_space('TD')
> +box.space.TD:format({{name = "I", type = "integer"},
> +                     {name = "D", type = "decimal"}})
> +box.space.TD:create_index("primary")
> +box.space.TD:insert({1, decimal})
> +
> +--
> +-- Update of UUID or VARBINARY also does not lead to segfault, however throws an
> +-- error since after changing value cannot be inserted into the field from SQL.
> +--
> +test:do_catchsql_test(
> +    "gh-5913-5",
> +    [[
> +        UPDATE tu SET u = u;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to uuid"
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5913-6",
> +    [[
> +        UPDATE td SET d = d;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to decimal"
> +    })
> +
> +test:finish_test()
> 
> 
> 
> New patch:
> 
> 
> commit d41d8f756f8ab6bfae70ee5787bc4c509ddb844d
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Thu Mar 4 17:17:18 2021 +0300
> 
>     sql: enhance vdbe_decode_msgpack_into_mem()
>     
>     Currently, vdbe_decode_msgpack_into_mem() creates a MEM that is not
>     properly initialized in case msgpack contains MP_EXT, MP_MAP, or
>     MP_ARRAY fields. This patch makes it so that after execution of this
>     function, all created MEMs are properly initialized.
>     
>     Closes #5011
>     Closes #5704
>     Closes #5913
>     Needed for #5818
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3b3b1f01d..9a4f38bb9 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -846,16 +846,6 @@ vdbe_field_ref_fetch_data(struct vdbe_field_ref *field_ref, uint32_t fieldno)
>  	return field_begin;
>  }
>  
> -static inline enum field_type
> -vdbe_field_ref_fetch_type(struct vdbe_field_ref *field_ref, uint32_t fieldno)
> -{
> -	const struct tuple_field *tf =
> -		vdbe_field_ref_fetch_field(field_ref, fieldno);
> -	if (tf == NULL || tf->type == FIELD_TYPE_ANY)
> -		return field_type_MAX;
> -	return tf->type;
> -}
> -
>  /**
>   * Fetch field by fieldno using vdbe_field_ref and store result
>   * in dest_mem.
> @@ -879,17 +869,6 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno,
>  	if (vdbe_decode_msgpack_into_mem(data, dest_mem, &dummy) != 0)
>  		return -1;
>  
> -	/*
> -	 * MsgPack map, array or extension (unsupported in sql).
> -	 * Wrap it in a blob verbatim.
> -	 */
> -	if (dest_mem->flags == 0) {
> -		dest_mem->z = (char *) data;
> -		dest_mem->n = vdbe_field_ref_fetch_data(field_ref,
> -							fieldno + 1) - data;
> -		dest_mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype;
> -		dest_mem->subtype = SQL_SUBTYPE_MSGPACK;
> -	}
>  	/*
>  	 * Add 0 termination (at most for strings)
>  	 * Not sure why do we check MEM_Ephem
> @@ -909,7 +888,6 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno,
>  		dest_mem->flags |= MEM_Term;
>  	}
>  	UPDATE_MAX_BLOBSIZE(dest_mem);
> -	dest_mem->field_type = vdbe_field_ref_fetch_type(field_ref, fieldno);
>  	return 0;
>  }
>  
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 91b64316e..772476377 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -2793,38 +2793,62 @@ vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
>  {
>  	const char *start_buf = buf;
>  	switch (mp_typeof(*buf)) {
> -	case MP_ARRAY:
> -	case MP_MAP:
> -	case MP_EXT:
> -	default: {
> -		mem->flags = 0;
> +	case MP_ARRAY: {
> +		mem->z = (char *)buf;
> +		mp_next(&buf);
> +		mem->n = buf - mem->z;
> +		mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype;
> +		mem->subtype = SQL_SUBTYPE_MSGPACK;
> +		mem->field_type = FIELD_TYPE_ARRAY;
> +		break;
> +	}
> +	case MP_MAP: {
> +		mem->z = (char *)buf;
> +		mp_next(&buf);
> +		mem->n = buf - mem->z;
> +		mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype;
> +		mem->subtype = SQL_SUBTYPE_MSGPACK;
> +		mem->field_type = FIELD_TYPE_MAP;
> +		break;
> +	}
> +	case MP_EXT: {
> +		mem->z = (char *)buf;
> +		mp_next(&buf);
> +		mem->n = buf - mem->z;
> +		mem->flags = MEM_Blob | MEM_Ephem;
> +		mem->field_type = FIELD_TYPE_VARBINARY;
>  		break;
>  	}
>  	case MP_NIL: {
>  		mp_decode_nil(&buf);
>  		mem->flags = MEM_Null;
> +		mem->field_type = field_type_MAX;
>  		break;
>  	}
>  	case MP_BOOL: {
>  		mem->u.b = mp_decode_bool(&buf);
>  		mem->flags = MEM_Bool;
> +		mem->field_type = FIELD_TYPE_BOOLEAN;
>  		break;
>  	}
>  	case MP_UINT: {
>  		uint64_t v = mp_decode_uint(&buf);
>  		mem->u.u = v;
>  		mem->flags = MEM_UInt;
> +		mem->field_type = FIELD_TYPE_INTEGER;
>  		break;
>  	}
>  	case MP_INT: {
>  		mem->u.i = mp_decode_int(&buf);
>  		mem->flags = MEM_Int;
> +		mem->field_type = FIELD_TYPE_INTEGER;
>  		break;
>  	}
>  	case MP_STR: {
>  		/* XXX u32->int */
>  		mem->n = (int) mp_decode_strl(&buf);
>  		mem->flags = MEM_Str | MEM_Ephem;
> +		mem->field_type = FIELD_TYPE_STRING;
>  install_blob:
>  		mem->z = (char *)buf;
>  		buf += mem->n;
> @@ -2834,18 +2858,33 @@ install_blob:
>  		/* XXX u32->int */
>  		mem->n = (int) mp_decode_binl(&buf);
>  		mem->flags = MEM_Blob | MEM_Ephem;
> +		mem->field_type = FIELD_TYPE_VARBINARY;
>  		goto install_blob;
>  	}
>  	case MP_FLOAT: {
>  		mem->u.r = mp_decode_float(&buf);
> -		mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
> +		if (sqlIsNaN(mem->u.r)) {
> +			mem->flags = MEM_Null;
> +			mem->field_type = FIELD_TYPE_DOUBLE;
> +		} else {
> +			mem->flags = MEM_Real;
> +			mem->field_type = FIELD_TYPE_DOUBLE;
> +		}
>  		break;
>  	}
>  	case MP_DOUBLE: {
>  		mem->u.r = mp_decode_double(&buf);
> -		mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
> +		if (sqlIsNaN(mem->u.r)) {
> +			mem->flags = MEM_Null;
> +			mem->field_type = FIELD_TYPE_DOUBLE;
> +		} else {
> +			mem->flags = MEM_Real;
> +			mem->field_type = FIELD_TYPE_DOUBLE;
> +		}
>  		break;
>  	}
> +	default:
> +		unreachable();
>  	}
>  	*len = (uint32_t)(buf - start_buf);
>  	return 0;
> @@ -2868,15 +2907,8 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def,	/* Information about the rec
>  		pMem->z = 0;
>  		uint32_t sz = 0;
>  		vdbe_decode_msgpack_into_mem(zParse, pMem, &sz);
> -		if (sz == 0) {
> -			/* MsgPack array, map or ext. Treat as blob. */
> -			pMem->z = (char *)zParse;
> -			mp_next(&zParse);
> -			pMem->n = zParse - pMem->z;
> -			pMem->flags = MEM_Blob | MEM_Ephem;
> -		} else {
> -			zParse += sz;
> -		}
> +		assert(sz != 0);
> +		zParse += sz;
>  		pMem++;
>  	}
>  }
> diff --git a/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua
> new file mode 100755
> index 000000000..60978c2b5
> --- /dev/null
> +++ b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua
> @@ -0,0 +1,99 @@
> +#!/usr/bin/env tarantool
> +local test = require("sqltester")
> +test:plan(6)
> +
> +local uuid = require("uuid").fromstr("11111111-1111-1111-1111-111111111111")
> +local decimal = require("decimal").new(111.111)
> +
> +box.schema.create_space('T')
> +box.space.T:format({{name = "I", type = "integer"}, {name = "U", type = "uuid"},
> +                    {name = "D", type = "decimal"}})
> +box.space.T:create_index("primary")
> +box.space.T:insert({1, uuid, decimal})
> +
> +--
> +-- Make sure that there is no segmentation fault on select from field that
> +-- contains UUID or DECIMAL. Currently SQL does not support UUID and DECIMAL,
> +-- so they treated as VARBINARY.
> +--
> +test:do_execsql_test(
> +    "gh-5913-1",
> +    [[
> +        SELECT i, u, d FROM t;
> +        SELECT i from t;
> +    ]], {
> +        1
> +    })
> +
> +box.schema.create_space('T1')
> +box.space.T1:format({{name = "I", type = "integer"},
> +                     {name = "U", type = "uuid", is_nullable = true},
> +                     {name = "D", type = "decimal", is_nullable = true}})
> +box.space.T1:create_index("primary")
> +
> +--
> +-- Since SQL does not support UUID and DECIMAL and they treated as VARBINARY,
> +-- they cannot be inserted from SQL.
> +--
> +test:do_catchsql_test(
> +    "gh-5913-2",
> +    [[
> +        INSERT INTO t1 SELECT i, u, NULL FROM t;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to uuid"
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5913-3",
> +    [[
> +        INSERT INTO t1 SELECT i, NULL, d FROM t;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to decimal"
> +    })
> +
> +--
> +-- Still, if UUID or DECIMAL fields does not selected directly, insert is
> +-- working properly.
> +--
> +test:do_execsql_test(
> +    "gh-5913-4",
> +    [[
> +        INSERT INTO t1 SELECT * FROM t;
> +        SELECT count() FROM t1;
> +    ]], {
> +        1
> +    })
> +
> +box.schema.create_space('TU')
> +box.space.TU:format({{name = "I", type = "integer"},
> +                     {name = "U", type = "uuid"}})
> +box.space.TU:create_index("primary")
> +box.space.TU:insert({1, uuid})
> +
> +box.schema.create_space('TD')
> +box.space.TD:format({{name = "I", type = "integer"},
> +                     {name = "D", type = "decimal"}})
> +box.space.TD:create_index("primary")
> +box.space.TD:insert({1, decimal})
> +
> +--
> +-- Update of UUID or VARBINARY also does not lead to segfault, however throws an
> +-- error since after changing value cannot be inserted into the field from SQL.
> +--
> +test:do_catchsql_test(
> +    "gh-5913-5",
> +    [[
> +        UPDATE tu SET u = u;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to uuid"
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5913-6",
> +    [[
> +        UPDATE td SET d = d;
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to decimal"
> +    })
> +
> +test:finish_test()