From 401f72de818367d380978d7afb3093ff71420e1c Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Fri, 1 Oct 2021 14:05:23 +0300
Subject: [PATCH] sql: remove MEM_Zero flag from struct MEM
This patch removes zeroblob optimization from SQL code. This
optimization complicates the code, and there is almost no profit from
it.
Closes #6113
Needed for #4145
---
.../gh-6113-fix-segfault-in-hex-func.md | 4 +
src/box/sql/func.c | 9 +-
src/box/sql/mem.c | 119 ++----------------
src/box/sql/mem.h | 22 +---
src/box/sql/sqlInt.h | 14 ---
src/box/sql/vdbe.c | 10 +-
src/box/sql/vdbeapi.c | 42 -------
test/sql-tap/engine.cfg | 1 +
...gh-6113-assert-in-hex-on-zeroblob.test.lua | 13 ++
9 files changed, 38 insertions(+), 196 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 0000000000..d9bd9e279c
--- /dev/null
+++ b/changelogs/unreleased/gh-6113-fix-segfault-in-hex-func.md
@@ -0,0 +1,4 @@
+## bugfix/sql
+
+* The HEX() SQL built-in function no longer throw an assert when its argument
+ consist of zero-bytes (gh-6113).
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 29d713fd08..187456d2c0 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1272,11 +1272,18 @@ zeroblobFunc(sql_context * context, int argc, sql_value ** argv)
n = mem_get_int_unsafe(argv[0]);
if (n < 0)
n = 0;
- if (sql_result_zeroblob64(context, n) != 0) {
+ if (n > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
"is too big");
context->is_aborted = true;
+ return;
+ }
+ char *str = sqlDbMallocZero(sql_get(), n);
+ if (str == NULL) {
+ context->is_aborted = true;
+ return;
}
+ mem_set_bin_allocated(context->pOut, str, n);
}
/*
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 7e94dc8bbb..89b99a183e 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -503,19 +503,6 @@ mem_copy_bin(struct Mem *mem, const char *value, uint32_t size)
return 0;
}
-void
-mem_set_zerobin(struct Mem *mem, int n)
-{
- mem_destroy(mem);
- if (n < 0)
- n = 0;
- mem->u.nZero = n;
- mem->z = NULL;
- mem->n = 0;
- mem->type = MEM_TYPE_BIN;
- mem->flags = MEM_Zero;
-}
-
static inline void
set_msgpack_value(struct Mem *mem, char *value, uint32_t size, int alloc_type,
enum mem_type type)
@@ -806,8 +793,6 @@ static inline int
bin_to_str(struct Mem *mem)
{
assert(mem->type == MEM_TYPE_BIN);
- if (ExpandBlob(mem) != 0)
- return -1;
mem->type = MEM_TYPE_STR;
mem->flags &= ~MEM_Scalar;
return 0;
@@ -817,8 +802,6 @@ static inline int
bin_to_str0(struct Mem *mem)
{
assert(mem->type == MEM_TYPE_BIN);
- if (ExpandBlob(mem) != 0)
- return -1;
if (sqlVdbeMemGrow(mem, mem->n + 1, 1) != 0)
return -1;
mem->z[mem->n] = '\0';
@@ -831,8 +814,6 @@ static inline int
bin_to_uuid(struct Mem *mem)
{
assert(mem->type == MEM_TYPE_BIN);
- if (ExpandBlob(mem) != 0)
- return -1;
if (mem->n != UUID_LEN ||
tt_uuid_validate((struct tt_uuid *)mem->z) != 0)
return -1;
@@ -1871,7 +1852,7 @@ mem_get_bin(const struct Mem *mem, const char **s)
*s = mem->n > 0 ? mem->z : NULL;
return 0;
}
- if (mem->type != MEM_TYPE_BIN || (mem->flags & MEM_Zero) != 0)
+ if (mem->type != MEM_TYPE_BIN)
return -1;
*s = mem->z;
return 0;
@@ -1882,11 +1863,7 @@ mem_len(const struct Mem *mem, uint32_t *len)
{
if (!mem_is_bytes(mem))
return -1;
- assert((mem->flags & MEM_Zero) == 0 || mem->type == MEM_TYPE_BIN);
- if ((mem->flags & MEM_Zero) != 0)
- *len = mem->n + mem->u.nZero;
- else
- *len = mem->n;
+ *len = mem->n;
return 0;
}
@@ -1912,9 +1889,6 @@ mem_copy(struct Mem *to, const struct Mem *from)
return 0;
if ((to->flags & MEM_Static) != 0)
return 0;
- assert((to->flags & MEM_Zero) == 0 || to->type == MEM_TYPE_BIN);
- if ((to->flags & MEM_Zero) != 0)
- return sqlVdbeMemExpandBlob(to);
to->zMalloc = sqlDbRealloc(to->db, to->zMalloc, MAX(32, to->n));
assert(to->zMalloc != NULL || sql_get()->mallocFailed != 0);
if (to->zMalloc == NULL)
@@ -1939,7 +1913,7 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from)
return;
if ((to->flags & (MEM_Static | MEM_Ephem)) != 0)
return;
- to->flags &= MEM_Term | MEM_Zero;
+ to->flags &= MEM_Term;
to->flags |= MEM_Ephem;
return;
}
@@ -1956,7 +1930,7 @@ mem_move(struct Mem *to, struct Mem *from)
}
int
-mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
+mem_concat(const struct Mem *a, const struct Mem *b, struct Mem *result)
{
if (mem_is_any_null(a, b)) {
mem_set_null(result);
@@ -1983,9 +1957,6 @@ mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
return -1;
}
- if (ExpandBlob(a) != 0 || ExpandBlob(b) != 0)
- return -1;
-
uint32_t size = a->n + b->n;
if ((int)size > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big");
@@ -2332,37 +2303,8 @@ static int
mem_cmp_bin(const struct Mem *a, const struct Mem *b)
{
assert((a->type & b->type & MEM_TYPE_BIN) != 0);
- int an = a->n;
- int bn = b->n;
- int minlen = MIN(an, bn);
-
- /*
- * It is possible to have a Blob value that has some non-zero content
- * followed by zero content. But that only comes up for Blobs formed
- * by the OP_MakeRecord opcode, and such Blobs never get passed into
- * mem_compare().
- */
- assert((a->flags & MEM_Zero) == 0 || an == 0);
- assert((b->flags & MEM_Zero) == 0 || bn == 0);
-
- if ((a->flags & b->flags & MEM_Zero) != 0)
- return a->u.nZero - b->u.nZero;
- if ((a->flags & MEM_Zero) != 0) {
- for (int i = 0; i < minlen; ++i) {
- if (b->z[i] != 0)
- return -1;
- }
- return a->u.nZero - bn;
- }
- if ((b->flags & MEM_Zero) != 0) {
- for (int i = 0; i < minlen; ++i) {
- if (a->z[i] != 0)
- return 1;
- }
- return b->u.nZero - an;
- }
- int res = memcmp(a->z, b->z, minlen);
- return res != 0 ? res : an - bn;
+ int res = memcmp(a->z, b->z, MIN(a->n, b->n));
+ return res != 0 ? res : a->n - b->n;
}
static int
@@ -2799,10 +2741,6 @@ sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
}
sql_snprintf(100, zCsr, "]%s", "(8)");
zCsr += sqlStrlen30(zCsr);
- if (f & MEM_Zero) {
- sql_snprintf(100, zCsr,"+%dz",pMem->u.nZero);
- zCsr += sqlStrlen30(zCsr);
- }
*zCsr = '\0';
} else if (pMem->type == MEM_TYPE_STR) {
int j, k;
@@ -2888,32 +2826,6 @@ registerTrace(int iReg, Mem *p) {
}
#endif
-/*
- * If the given Mem* has a zero-filled tail, turn it into an ordinary
- * blob stored in dynamically allocated space.
- */
-int
-sqlVdbeMemExpandBlob(Mem * pMem)
-{
- int nByte;
- assert(pMem->flags & MEM_Zero);
- assert(pMem->type == MEM_TYPE_BIN);
-
- /* Set nByte to the number of bytes required to store the expanded blob. */
- nByte = pMem->n + pMem->u.nZero;
- if (nByte <= 0) {
- nByte = 1;
- }
- if (sqlVdbeMemGrow(pMem, nByte, 1)) {
- return -1;
- }
-
- memset(&pMem->z[pMem->n], 0, pMem->u.nZero);
- pMem->n += pMem->u.nZero;
- pMem->flags &= ~(MEM_Zero | MEM_Term);
- return 0;
-}
-
static int
sqlVdbeMemGrow(struct Mem *pMem, int n, int bPreserve)
{
@@ -3038,13 +2950,8 @@ int
sqlVdbeMemTooBig(Mem * p)
{
assert(p->db != 0);
- if (mem_is_bytes(p)) {
- int n = p->n;
- if (p->flags & MEM_Zero) {
- n += p->u.nZero;
- }
- return n > p->db->aLimit[SQL_LIMIT_LENGTH];
- }
+ if (mem_is_bytes(p))
+ return p->n > p->db->aLimit[SQL_LIMIT_LENGTH];
return 0;
}
@@ -3253,14 +3160,8 @@ mpstream_encode_vdbe_mem(struct mpstream *stream, struct Mem *var)
mpstream_encode_double(stream, var->u.r);
return;
case MEM_TYPE_BIN:
- if ((var->flags & MEM_Zero) != 0) {
- mpstream_encode_binl(stream, var->n + var->u.nZero);
- mpstream_memcpy(stream, var->z, var->n);
- mpstream_memset(stream, 0, var->u.nZero);
- } else {
- mpstream_encode_binl(stream, var->n);
- mpstream_memcpy(stream, var->z, var->n);
- }
+ mpstream_encode_binl(stream, var->n);
+ mpstream_memcpy(stream, var->z, var->n);
return;
case MEM_TYPE_ARRAY:
case MEM_TYPE_MAP:
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 0da45b8af5..1574da22d9 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -68,7 +68,6 @@ struct Mem {
i64 i; /* Integer value used when MEM_Int is set in flags */
uint64_t u; /* Unsigned integer used when MEM_UInt is set. */
bool b; /* Boolean value used when MEM_Bool is set in flags */
- int nZero; /* Used when bit MEM_Zero is set in flags */
void *p; /* Generic pointer */
/**
* A pointer to function implementation.
@@ -111,7 +110,6 @@ struct Mem {
#define MEM_Dyn 0x0800 /* Need to call Mem.xDel() on Mem.z */
#define MEM_Static 0x1000 /* Mem.z points to a static string */
#define MEM_Ephem 0x2000 /* Mem.z points to an ephemeral string */
-#define MEM_Zero 0x8000 /* Mem.i contains count of 0s appended to blob */
static inline bool
mem_is_null(const struct Mem *mem)
@@ -246,13 +244,6 @@ mem_is_cleared(const struct Mem *mem)
return (mem->flags & MEM_Cleared) != 0;
}
-static inline bool
-mem_is_zerobin(const struct Mem *mem)
-{
- assert((mem->flags & MEM_Zero) == 0 || mem->type == MEM_TYPE_BIN);
- return (mem->flags & MEM_Zero) != 0;
-}
-
static inline bool
mem_is_same_type(const struct Mem *mem1, const struct Mem *mem2)
{
@@ -474,12 +465,6 @@ mem_set_binl(struct Mem *mem, char *value, uint32_t size,
return mem_set_bin_dynamic(mem, value, size);
}
-/**
- * Clear MEM and set it to VARBINARY. The binary value consist of n zero bytes.
- */
-void
-mem_set_zerobin(struct Mem *mem, int n);
-
/**
* Copy binary value to a newly allocated memory. The MEM type becomes
* VARBINARY.
@@ -603,7 +588,7 @@ mem_move(struct Mem *to, struct Mem *from);
* result MEM is set to NULL even if the result MEM is actually the first MEM.
*/
int
-mem_concat(struct Mem *left, struct Mem *right, struct Mem *result);
+mem_concat(const struct Mem *a, const struct Mem *b, struct Mem *result);
/**
* Add the first MEM to the second MEM and write the result to the third MEM.
@@ -935,11 +920,6 @@ registerTrace(int iReg, Mem *p);
#define memIsValid(M) ((M)->type != MEM_TYPE_INVALID)
#endif
-int sqlVdbeMemExpandBlob(struct Mem *);
-#define ExpandBlob(P) (((P)->flags&MEM_Zero)?sqlVdbeMemExpandBlob(P):0)
-
-/** Setters = Change MEM value. */
-
int sqlVdbeMemClearAndResize(struct Mem * pMem, int n);
void sqlValueFree(struct Mem *);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2e250dc291..eac6bcec80 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -394,13 +394,6 @@ void
sql_result_value(sql_context *,
sql_value *);
-void
-sql_result_zeroblob(sql_context *, int n);
-
-int
-sql_result_zeroblob64(sql_context *,
- sql_uint64 n);
-
char *
sql_mprintf(const char *, ...);
char *
@@ -631,13 +624,6 @@ int
sql_bind_text64(sql_stmt *, int, const char *,
sql_uint64, void (*)(void *));
-int
-sql_bind_zeroblob(sql_stmt *, int, int n);
-
-int
-sql_bind_zeroblob64(sql_stmt *, int,
- sql_uint64);
-
int
sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 44533fb3eb..86550541bf 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1462,8 +1462,6 @@ case OP_MustBeInt: { /* jump, in1 */
*/
case OP_Cast: { /* in1 */
pIn1 = &aMem[pOp->p1];
- if (ExpandBlob(pIn1) != 0)
- goto abort_due_to_error;
rc = mem_cast_explicit(pIn1, pOp->p2);
UPDATE_MAX_BLOBSIZE(pIn1);
if (rc == 0)
@@ -2772,8 +2770,6 @@ case OP_Found: { /* jump, in3 */
#ifdef SQL_DEBUG
for(ii=0; ii<r.nField; ii++) {
assert(memIsValid(&r.aMem[ii]));
- assert(!mem_is_zerobin(&r.aMem[ii]) ||
- r.aMem[ii].n == 0);
if (ii != 0)
REGISTER_TRACE(p, pOp->p3+ii, &r.aMem[ii]);
}
@@ -2784,7 +2780,6 @@ case OP_Found: { /* jump, in3 */
pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def);
if (pIdxKey==0) goto no_mem;
assert(mem_is_bin(pIn3));
- (void)ExpandBlob(pIn3);
sqlVdbeRecordUnpackMsgpack(pC->key_def,
pIn3->z, pIdxKey);
}
@@ -3411,8 +3406,7 @@ case OP_SorterInsert: { /* in2 */
assert(isSorter(cursor));
pIn2 = &aMem[pOp->p2];
assert(mem_is_bin(pIn2));
- if (ExpandBlob(pIn2) != 0 ||
- sqlVdbeSorterWrite(cursor, pIn2) != 0)
+ if (sqlVdbeSorterWrite(cursor, pIn2) != 0)
goto abort_due_to_error;
break;
}
@@ -3445,8 +3439,6 @@ case OP_IdxReplace:
case OP_IdxInsert: {
pIn2 = &aMem[pOp->p1];
assert(mem_is_bin(pIn2));
- if (ExpandBlob(pIn2) != 0)
- goto abort_due_to_error;
struct space *space;
if (pOp->p4type == P4_SPACEPTR)
space = pOp->p4.space;
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 115940227a..97bd19863a 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -236,25 +236,6 @@ sql_result_value(sql_context * pCtx, sql_value * pValue)
pCtx->is_aborted = true;
}
-void
-sql_result_zeroblob(sql_context * pCtx, int n)
-{
- mem_set_zerobin(pCtx->pOut, n);
-}
-
-int
-sql_result_zeroblob64(sql_context * pCtx, u64 n)
-{
- Mem *pOut = pCtx->pOut;
- if (n > (u64) pOut->db->aLimit[SQL_LIMIT_LENGTH]) {
- diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
- "is too big");
- return -1;
- }
- mem_set_zerobin(pCtx->pOut, (int)n);
- return 0;
-}
-
/*
* Execute the statement pStmt, either until a row of data is ready, the
* statement is completely executed or an error occurs.
@@ -818,29 +799,6 @@ sql_bind_text64(sql_stmt * pStmt,
}
}
-int
-sql_bind_zeroblob(sql_stmt * pStmt, int i, int n)
-{
- Vdbe *p = (Vdbe *) pStmt;
- if (vdbeUnbind(p, i) != 0)
- return -1;
- mem_set_zerobin(&p->aVar[i - 1], n);
- return 0;
-}
-
-int
-sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64 n)
-{
- Vdbe *p = (Vdbe *) pStmt;
- if (n > (u64) p->db->aLimit[SQL_LIMIT_LENGTH]) {
- diag_set(ClientError, ER_SQL_EXECUTE, "string or binary string"\
- "is too big");
- return -1;
- }
- assert((n & 0x7FFFFFFF) == n);
- return sql_bind_zeroblob(pStmt, i, n);
-}
-
int
sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
{
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 9d318a4569..528212ab6e 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -40,6 +40,7 @@
"gh-4077-iproto-execute-no-bind.test.lua": {},
"gh-6375-assert-on-unsupported-ext.test.lua": {},
"gh-6485-bugs-in-decimal.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 0000000000..91a29a5b40
--- /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()
--
GitLab