[Tarantool-patches] [PATCH v1 1/1] sql: fix a segfault in hex() on receiving zeroblob
Mergen Imeev
imeevma at tarantool.org
Fri Aug 27 11:26:34 MSK 2021
Thank you for the review! My answers, diff and new patch below.
On Thu, Aug 26, 2021 at 10:42:00PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index c063552d6..2ff368dc7 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -53,6 +53,49 @@
> > +/** Implementation of the HEX() SQL built-in function. */
> > +static void
> > +func_hex(struct sql_context *ctx, int argc, struct Mem **argv)
> > +{
> > + assert(argc == 1);
> > + (void)argc;
> > + if (argv[0]->type == MEM_TYPE_NULL)
> > + return mem_set_null(ctx->pOut);
> > +
> > + assert(argv[0]->type == MEM_TYPE_BIN && argv[0]->n >= 0);
> > + assert((argv[0]->flags & MEM_Zero) == 0 || argv[0]->u.nZero >= 0);
> > + uint32_t size = 2 * argv[0]->n;
> > + if ((argv[0]->flags & MEM_Zero) != 0)
> > + size += 2 * argv[0]->u.nZero;
> > + if (size == 0)
> > + return mem_set_str0_static(ctx->pOut, "");
> > +
> > + char *str = sqlDbMallocRawNN(sql_get(), size);
> > + if (str == NULL) {
> > + ctx->is_aborted = true;
> > + return;
> > + }
> > + for (int i = 0; i < argv[0]->n; ++i) {
> > + char c = argv[0]->z[i];
> > + str[2 * i] = hexdigits[(c >> 4) & 0xf];
> > + str[2 * i + 1] = hexdigits[c & 0xf];
> > + }
> > + if ((argv[0]->flags & MEM_Zero) != 0) {
> > + for (int i = 0; i < argv[0]->u.nZero; ++i) {
> > + int j = argv[0]->n + i;
> > + str[2 * j] = '0';
> > + str[2 * j + 1] = '0';
>
> 1. The same as for the patch for 2.8 branch.
>
Fixed.
> > + }
> > + }
> > + mem_set_str_allocated(ctx->pOut, str, size);
> > +}
> > @@ -2034,7 +2042,7 @@ static struct sql_func_definition definitions[] = {
> > {"GROUP_CONCAT", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
> > FIELD_TYPE_VARBINARY, groupConcatStep, groupConcatFinalize},
> >
> > - {"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, hexFunc, NULL},
> > + {"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, func_hex, NULL},
>
> 2. What is the final name pattern? I see among new function names
>
> - trim_func - 'func' suffix
>
> - sql_func_uuid, sql_func_version - 'sql_func' prefix
>
> - sql_builtin_stub - 'sql' prefix
>
> - sum_step - no prefixes or suffixes
>
> now you add a fifth way:
>
> - func_hex - 'func' prefix.
>
> I suggest to choose one way to use for all new function names.
I see no need for 'sql_' prefix since these functions will be static. I plan to
use 'func_' prefix for usual functions, 'step_' for aggregate step-functions and
'fin_' for finalize functions. Most of functions I plan to rewrite during few
weeks.
Diff:
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 2ff368dc7..fa2a2c245 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -86,13 +86,8 @@ func_hex(struct sql_context *ctx, int argc, struct Mem **argv)
str[2 * i] = hexdigits[(c >> 4) & 0xf];
str[2 * i + 1] = hexdigits[c & 0xf];
}
- if ((argv[0]->flags & MEM_Zero) != 0) {
- for (int i = 0; i < argv[0]->u.nZero; ++i) {
- int j = argv[0]->n + i;
- str[2 * j] = '0';
- str[2 * j + 1] = '0';
- }
- }
+ if ((argv[0]->flags & MEM_Zero) != 0)
+ memset(&str[2 * argv[0]->n], '0', 2 * argv[0]->u.nZero);
mem_set_str_allocated(ctx->pOut, str, size);
}
New patch:
commit cded1126f703416c526bc7e9a6992dde8f52e58e
Author: Mergen Imeev <imeevma at 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 c063552d6..fa2a2c245 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -53,6 +53,44 @@
static struct mh_strnptr_t *built_in_functions = NULL;
static struct func_sql_builtin **functions;
+/** Array for converting from half-bytes into ASCII hex digits. */
+static const char hexdigits[] = {
+ '0', '1', '2', '3', '4', '5', '6', '7',
+ '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
+};
+
+/** Implementation of the HEX() SQL built-in function. */
+static void
+func_hex(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+ assert(argc == 1);
+ (void)argc;
+ if (argv[0]->type == MEM_TYPE_NULL)
+ return mem_set_null(ctx->pOut);
+
+ assert(argv[0]->type == MEM_TYPE_BIN && argv[0]->n >= 0);
+ assert((argv[0]->flags & MEM_Zero) == 0 || argv[0]->u.nZero >= 0);
+ uint32_t size = 2 * argv[0]->n;
+ if ((argv[0]->flags & MEM_Zero) != 0)
+ size += 2 * argv[0]->u.nZero;
+ if (size == 0)
+ return mem_set_str0_static(ctx->pOut, "");
+
+ char *str = sqlDbMallocRawNN(sql_get(), size);
+ if (str == NULL) {
+ ctx->is_aborted = true;
+ return;
+ }
+ for (int i = 0; i < argv[0]->n; ++i) {
+ char c = argv[0]->z[i];
+ str[2 * i] = hexdigits[(c >> 4) & 0xf];
+ str[2 * i + 1] = hexdigits[c & 0xf];
+ }
+ if ((argv[0]->flags & MEM_Zero) != 0)
+ memset(&str[2 * argv[0]->n], '0', 2 * argv[0]->u.nZero);
+ mem_set_str_allocated(ctx->pOut, str, size);
+}
+
static const unsigned char *
mem_as_ustr(struct Mem *mem)
{
@@ -1072,14 +1110,6 @@ sql_func_version(struct sql_context *context,
sql_result_text(context, tarantool_version(), -1, SQL_STATIC);
}
-/* Array for converting from half-bytes (nybbles) into ASCII hex
- * digits.
- */
-static const char hexdigits[] = {
- '0', '1', '2', '3', '4', '5', '6', '7',
- '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
-};
-
/*
* Implementation of the QUOTE() function. This function takes a single
* argument. If the argument is numeric, the return value is the same as
@@ -1233,33 +1263,6 @@ charFunc(sql_context * context, int argc, sql_value ** argv)
sql_result_text64(context, (char *)z, zOut - z, sql_free);
}
-/*
- * The hex() function. Interpret the argument as a blob. Return
- * a hexadecimal rendering as text.
- */
-static void
-hexFunc(sql_context * context, int argc, sql_value ** argv)
-{
- int i, n;
- const unsigned char *pBlob;
- char *zHex, *z;
- assert(argc == 1);
- UNUSED_PARAMETER(argc);
- pBlob = mem_as_bin(argv[0]);
- n = mem_len_unsafe(argv[0]);
- 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++) {
- unsigned char c = *pBlob;
- *(z++) = hexdigits[(c >> 4) & 0xf];
- *(z++) = hexdigits[c & 0xf];
- }
- *z = 0;
- sql_result_text(context, zHex, n * 2, sql_free);
- }
-}
-
/*
* The zeroblob(N) function returns a zero-filled blob of size N bytes.
*/
@@ -2034,7 +2037,7 @@ static struct sql_func_definition definitions[] = {
{"GROUP_CONCAT", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY},
FIELD_TYPE_VARBINARY, groupConcatStep, groupConcatFinalize},
- {"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, hexFunc, NULL},
+ {"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, func_hex, NULL},
{"IFNULL", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
sql_builtin_stub, NULL},
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 35754f769..664cfdd77 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -35,6 +35,7 @@
"built-in-functions.test.lua": {
"memtx": {"engine": "memtx"}
},
+ "gh-6113-assert-in-hex-on-zeroblob.test.lua": {},
"gh-4077-iproto-execute-no-bind.test.lua": {},
"*": {
"memtx": {"engine": "memtx"},
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()
More information about the Tarantool-patches
mailing list