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

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

This version is much simpler and is quite readable as is.

LGTM.

Though few unimportant notes...

On 30.08.2021 9:20, imeevma@tarantool.org wrote:
> 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.8
> 

> 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);

I believe this is unncessary, as those exactly checks were already done 
inside of mem_len()

> +	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);

Worth to note that here contextMalloc() used to check passed length 
against SQL_LIMIT_LENGTH, in the newer code this check disappeared.

>   	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);
>   	}
>   }

Regards,
Timur