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

[Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

I may miss something obvious, but prior version of a code
with pBlob and n was much shorter, compacter and more readable.
I'm curious, why do you prefer to always use argv[0]->n and
argv[0]->z instead?

Also, it seems to me we better to limit the number of bytes customer
may request to allocate from HEX()? What about to check against SQL_LIMIT_LENGTH?

Thanks,
Timur

> -----Original Message-----
> From: imeevma@tarantool.org <imeevma@tarantool.org>
> Sent: Monday, August 30, 2021 9:31 AM
> To: tsafin@tarantool.org
> Cc: tarantool-patches@dev.tarantool.org
> Subject: [PATCH v1 1/1] 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
> ---
> https://github.com/tarantool/tarantool/issues/6113
> https://github.com/tarantool/tarantool/tree/imeevma/gh-6113-fix-hex-
> segfault-2.10
> 
>  .../gh-6113-fix-segfault-in-hex-func.md       |  5 ++
>  src/box/sql/func.c                            | 75 ++++++++++-------
> --
>  test/sql-tap/engine.cfg                       |  1 +
>  ...gh-6113-assert-in-hex-on-zeroblob.test.lua | 13 ++++
>  4 files changed, 58 insertions(+), 36 deletions(-)
>  create mode 100644 changelogs/unreleased/gh-6113-fix-segfault-in-
> hex-func.md
>  create mode 100755 test/sql-tap/gh-6113-assert-in-hex-on-
> zeroblob.test.lua
> 
> 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 587adbed9..5ff0219fc 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": {},
>      "gh-6375-assert-on-unsupported-ext.test.lua": {},
>      "*": {
> 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()
> --
> 2.25.1