Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix a segfault in hex() on receiving zeroblob

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

Thank you for the review! My answers, diff and new patch below.

On Thu, Aug 26, 2021 at 10:31:53PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index b137c6125..d182bb313 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -1221,14 +1221,22 @@ hexFunc(sql_context * context, int argc, sql_value ** argv)
> >  	UNUSED_PARAMETER(argc);
> >  	pBlob = mem_as_bin(argv[0]);
> >  	n = mem_len_unsafe(argv[0]);
> > +	assert((argv[0]->flags & MEM_Zero) == 0 ||
> > +	       argv[0]->type == MEM_TYPE_BIN);
> > +	int zero_len = (argv[0]->flags & MEM_Zero) == 0 ? 0 : argv[0]->u.nZero;
> >  	assert(pBlob == mem_as_bin(argv[0]));	/* No encoding change */
> >  	z = zHex = contextMalloc(context, ((i64) n) * 2 + 1);
> >  	if (zHex) {
> > -		for (i = 0; i < n; i++, pBlob++) {
> > +		for (i = 0; i < n - zero_len; i++, pBlob++) {
> >  			unsigned char c = *pBlob;
> >  			*(z++) = hexdigits[(c >> 4) & 0xf];
> >  			*(z++) = hexdigits[c & 0xf];
> >  		}
> > +		for (; i < n; ++i) {
> > +			assert((argv[0]->flags & MEM_Zero) != 0);
> 
> 1. This assert can be out of the loop. It does not depend on z or i.
> 
Actually, it does, since MEM_Zero flag is set only when i < n. Fixed.

> 2. The loop could be replaced with memset().
> 
Thanks, fixed.

> > +			*(z++) = '0';
> > +			*(z++) = '0';
> > +		}

Diff:


diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index d182bb313..3ef31705e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1232,12 +1232,10 @@ hexFunc(sql_context * context, int argc, sql_value ** argv)
 			*(z++) = hexdigits[(c >> 4) & 0xf];
 			*(z++) = hexdigits[c & 0xf];
 		}
-		for (; i < n; ++i) {
-			assert((argv[0]->flags & MEM_Zero) != 0);
-			*(z++) = '0';
-			*(z++) = '0';
-		}
-		*z = 0;
+		assert(i == n || (argv[0]->flags & MEM_Zero) != 0);
+		assert(n == zero_len + i);
+		memset(z, '0', 2 * zero_len);
+		z[2 * zero_len] = '\0';
 		sql_result_text(context, zHex, n * 2, sql_free);
 	}
 }


New patch:


commit 3fddf927be4ef819b63e172f29af58ac352da640
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sun Aug 22 08:05:45 2021 +0300

    sql: fix a segfault in hex() on receiving zeroblob
    
    This patch fixes a segmentation fault when zeroblob is received by the
    SQL built-in HEX() function.
    
    Closes #6113

diff --git a/changelogs/unreleased/gh-6113-fix-segfault-in-hex-func.md b/changelogs/unreleased/gh-6113-fix-segfault-in-hex-func.md
new file mode 100644
index 000000000..c59be4d96
--- /dev/null
+++ b/changelogs/unreleased/gh-6113-fix-segfault-in-hex-func.md
@@ -0,0 +1,5 @@
+## bugfix/sql
+
+* The HEX() SQL built-in function now does not throw an assert on receiving
+  varbinary values that consist of zero-bytes (gh-6113).
+
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b137c6125..3ef31705e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1221,15 +1221,21 @@ hexFunc(sql_context * context, int argc, sql_value ** argv)
 	UNUSED_PARAMETER(argc);
 	pBlob = mem_as_bin(argv[0]);
 	n = mem_len_unsafe(argv[0]);
+	assert((argv[0]->flags & MEM_Zero) == 0 ||
+	       argv[0]->type == MEM_TYPE_BIN);
+	int zero_len = (argv[0]->flags & MEM_Zero) == 0 ? 0 : argv[0]->u.nZero;
 	assert(pBlob == mem_as_bin(argv[0]));	/* No encoding change */
 	z = zHex = contextMalloc(context, ((i64) n) * 2 + 1);
 	if (zHex) {
-		for (i = 0; i < n; i++, pBlob++) {
+		for (i = 0; i < n - zero_len; i++, pBlob++) {
 			unsigned char c = *pBlob;
 			*(z++) = hexdigits[(c >> 4) & 0xf];
 			*(z++) = hexdigits[c & 0xf];
 		}
-		*z = 0;
+		assert(i == n || (argv[0]->flags & MEM_Zero) != 0);
+		assert(n == zero_len + i);
+		memset(z, '0', 2 * zero_len);
+		z[2 * zero_len] = '\0';
 		sql_result_text(context, zHex, n * 2, sql_free);
 	}
 }
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 693a477b7..ddee8c328 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -21,6 +21,7 @@
         "memtx": {"engine": "memtx"}
     },
     "gh-4077-iproto-execute-no-bind.test.lua": {},
+    "gh-6113-assert-in-hex-on-zeroblob.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
         "vinyl": {"engine": "vinyl"}
diff --git a/test/sql-tap/gh-6113-assert-in-hex-on-zeroblob.test.lua b/test/sql-tap/gh-6113-assert-in-hex-on-zeroblob.test.lua
new file mode 100755
index 000000000..91a29a5b4
--- /dev/null
+++ b/test/sql-tap/gh-6113-assert-in-hex-on-zeroblob.test.lua
@@ -0,0 +1,13 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(1)
+
+test:do_execsql_test(
+    "gh-6113",
+    [[
+        SELECT hex(zeroblob(0)), hex(zeroblob(10));
+    ]], {
+        '', '00000000000000000000'
+    })
+
+test:finish_test()