[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for review! My answers and new patch below.

On Wed, Aug 07, 2019 at 09:25:59PM +0300, n.pettik wrote:
> 
> > 
> > New patch:
> > 
> > From 5dde6f6398ca9845c3a17173b9e9625bc1b20d32 Mon Sep 17 00:00:00 2001
> > From: Mergen Imeev <imeevma@gmail.com>
> > Date: Mon, 22 Jul 2019 12:54:34 +0300
> > Subject: [PATCH] sql: fix error in case ARRAY/MAP converted to SCALAR
> > 
> > Since ARRAY and MAP cannot be converted to a scalar, this
> > operation should throw an error. But when the error was throws
> > from SQL, the error was unreadable. The reason for this is that
> > the given array or map was not correctly converted to a string.
> > This patch fixes the problem by converting ARRAY or MAP to their
> > string representation.
> > For example:
> > 
> > box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
> > format = {}
> > format[1] = {type = 'integer', name = 'I'}
> > format[2] = {type = 'array', name = 'A'}
> > s = box.schema.space.create('T2', {format=format})
> > i = s:create_index('ii')
> > s:insert({1, {1,2,3}})
> > box.execute('INSERT INTO t1 SELECT * FROM t2;')
> > 
> > Should return:
> > - error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
> > 
> > Follow-up #4189
> 
> Fixed a bit commit message:
> 
>     sql: make valueToText() operate on MAP/ARRAY values
>     
>     Since ARRAY and MAP cannot be converted to SCALAR type, this operation
>     should throw an error. But when the error is raised in SQL, it is
>     displayed in unreadable form. The reason for this is that the given
>     array or map is not correctly converted to a string. This patch fixes
>     the problem by converting ARRAY or MAP to their string representation.
>     For example:
>     
>     box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
>     format = {}
>     format[1] = {type = 'integer', name = 'I'}
>     format[2] = {type = 'array', name = 'A'}
>     s = box.schema.space.create('T2', {format=format})
>     i = s:create_index('ii')
>     s:insert({1, {1,2,3}})
>     box.execute('INSERT INTO t1 SELECT * FROM t2;')
>     
>     Should return:
>     - error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
>     
>     Follow-up #4189
> 
Thank you, fixed.

> > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > index 847a6b0..8bea46b 100644
> > --- a/src/box/sql/vdbemem.c
> > +++ b/src/box/sql/vdbemem.c
> > @@ -1135,6 +1135,19 @@ valueToText(sql_value * pVal)
> > {
> > 	assert(pVal != 0);
> > 	assert((pVal->flags & (MEM_Null)) == 0);
> > +	if ((pVal->flags & MEM_Subtype) != 0 &&
> > +	    pVal->subtype == SQL_SUBTYPE_MSGPACK) {
> > +		const char *value = mp_str(pVal->z);
> > +		size_t len = strlen(value) + 1;
> > +		char *result = region_alloc(&fiber()->gc, len);
> > +		if (result == NULL) {
> > +			diag_set(OutOfMemory, len, "region_alloc", "result");
> > +			sqlOomFault(sql_get());
> > +			return NULL;
> > +		}
> > +		memcpy(result, value, len);
> > +		return result;
> 
> That’s not what we need IMHO. Firstly, for all other memory types                  
> valueToText() function returns pVal->z, i.e. this function firstly                 
> converts value to string and then returns it.  Secondly, we don’t track            
> value allocated on region: obviously if it is used after transaction               
> commitment, it will lead to use-after-free bug. I can’t say whether this           
> scenario is possible looking only on code.  You should rather patch                
> sqlVdbeMemStringify: reserve memory using malloc, memcpy string to mem,            
> change type of memory to MEM_Str. So that make it work with ARRAY and              
> MAP types in the same way as with other types.                                     
> 
Fixed.

New patch:

From d0ffad654fe4ddf366bd4cfd4e1c9340a8044244 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 22 Jul 2019 12:54:34 +0300
Subject: [PATCH] sql: make valueToText() operate on MAP/ARRAY values

Since ARRAY and MAP cannot be converted to SCALAR type, this
operation should throw an error. But when the error is raised in
SQL, it is displayed in unreadable form. The reason for this is
that the given array or map is not correctly converted to a
string. This patch fixes the problem by converting ARRAY or MAP to
their string representation.
For example:

box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
format = {}
format[1] = {type = 'integer', name = 'I'}
format[2] = {type = 'array', name = 'A'}
s = box.schema.space.create('T2', {format=format})
i = s:create_index('ii')
s:insert({1, {1,2,3}})
box.execute('INSERT INTO t1 SELECT * FROM t2;')

Should return:
- error: 'Type mismatch: can not convert [1, 2, 3] to scalar'

Follow-up #4189

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 5516d7f..f94f118 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -281,15 +281,30 @@ int
 sqlVdbeMemStringify(Mem * pMem)
 {
 	int fg = pMem->flags;
-	const int nByte = 32;
+	int nByte = 32;
 
-	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
+	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
+	    ((pMem->flags & MEM_Subtype) == 0 ||
+	     pMem->subtype != SQL_SUBTYPE_MSGPACK))
 		return 0;
 
 	assert(!(fg & MEM_Zero));
-	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool)) != 0);
+	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool |
+		      MEM_Subtype)) != 0);
 	assert(EIGHT_BYTE_ALIGNMENT(pMem));
 
+	/*
+	 * In case we have ARRAY/MAP we should save decoded value
+	 * before clearing pMem->z.
+	 */
+	char *value = NULL;
+	if ((fg & MEM_Subtype) != 0 && pMem->subtype == SQL_SUBTYPE_MSGPACK) {
+		const char *value_str = mp_str(pMem->z);
+		nByte = strlen(value_str) + 1;
+		value = region_alloc(&fiber()->gc, nByte);
+		memcpy(value, value_str, nByte);
+	}
+
 	if (sqlVdbeMemClearAndResize(pMem, nByte)) {
 		return -1;
 	}
@@ -302,6 +317,11 @@ sqlVdbeMemStringify(Mem * pMem)
 	} else if ((fg & MEM_Bool) != 0) {
 		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
 		pMem->flags &= ~MEM_Bool;
+	} else if ((fg & MEM_Subtype) != 0 &&
+		   pMem->subtype == SQL_SUBTYPE_MSGPACK) {
+		sql_snprintf(nByte, pMem->z, "%s", value);
+		pMem->flags &= ~MEM_Subtype;
+		pMem->subtype = SQL_SUBTYPE_NO;
 	} else {
 		assert(fg & MEM_Real);
 		sql_snprintf(nByte, pMem->z, "%!.15g", pMem->u.r);
@@ -1148,7 +1168,9 @@ valueToText(sql_value * pVal)
 {
 	assert(pVal != 0);
 	assert((pVal->flags & (MEM_Null)) == 0);
-	if (pVal->flags & (MEM_Blob | MEM_Str)) {
+	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
+	    ((pVal->flags & MEM_Subtype) == 0 ||
+	     pVal->subtype != SQL_SUBTYPE_MSGPACK)) {
 		if (ExpandBlob(pVal))
 			return 0;
 		pVal->flags |= MEM_Str;
diff --git a/test/sql/types.result b/test/sql/types.result
index 1241ae4..6740c15 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -2266,3 +2266,74 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+---
+- row_count: 1
+...
+format = {}
+---
+...
+format[1] = {type = 'integer', name = 'I'}
+---
+...
+format[2] = {type = 'array', name = 'A'}
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert [1, 2, 3] to scalar'
+...
+s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
+---
+- [1, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,
+    22, 23, 24, 25, 26, 27, 28, 29, 30]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
+  15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30] to scalar'
+...
+s:drop()
+---
+...
+format[2].type = 'map'
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {b = 1}})
+---
+- [1, {'b': 1}]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert {"b": 1} to scalar'
+...
+s:drop()
+---
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index f6a2dfd..c9f0952 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -512,3 +512,26 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
 box.execute('UPDATE t SET b = false WHERE i = 1;')
 s:select()
 s:drop()
+
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+format = {}
+format[1] = {type = 'integer', name = 'I'}
+format[2] = {type = 'array', name = 'A'}
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {1,2,3}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+format[2].type = 'map'
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {b = 1}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+box.execute('DROP TABLE t1;')