* [tarantool-patches] [PATCH] sql: terminate with \0 encoded msgpack
@ 2018-12-11 16:21 Nikita Pettik
2018-12-12 11:40 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 4+ messages in thread
From: Nikita Pettik @ 2018-12-11 16:21 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
During table, index, constraint or trigger creation it is required to
encode entry's parts/opts to msgpack for the next insertion into system
space. Encoded msgpack is known to lack null termination at the end of
binary string. Hence, while such string is passed to printf, strlen or
other function which excpects null termination, buffer overflow may
occur.
This bug was hard to detect due to several reasons.
Firstly, SQLite memory allocators align memory to 8-byte border. Hence,
as a rule even ASAN doesn't detect overflow, since there is additional
alignment which can be addressable.
Secondly, msgpack can contain nulls through the encoded string. As a
result, strlen returns not the real size of binary string, but something
less.
Note that null termination is required only encoded string is to be
displayed (i.e. EXPLAIN statement is executed or vdbe_debug mode is
enabled).
Bug was discovered with help of AddressSanitizer.
Closes #3868
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-3868-buffer-overflow
Issue: https://github.com/tarantool/tarantool/issues/3868
* Notes to the one who will review *
To enable ASAN on macOS:
cmake -DENABLE_ASAN=ON .
Also it may be required to apply this diff:
diff --git a/src/fiber.c b/src/fiber.c
index 7658a2294..743ceff7a 100644
--- a/src/fiber.c
+++ b/src/fiber.c
@@ -54,7 +54,7 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
__sanitizer_start_switch_fiber((will_switch_back) ? &var_name : NULL, \
(bottom), (size))
#define ASAN_FINISH_SWITCH_FIBER(var_name) \
- __sanitizer_finish_switch_fiber(var_name);
+ __sanitizer_finish_switch_fiber(var_name, 0, 0);
To turn off SQLite 8-byte allocation alignment apply this diff:
diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c
index 9526cce5e..fd206632b 100644
--- a/src/box/sql/malloc.c
+++ b/src/box/sql/malloc.c
@@ -49,7 +49,7 @@ sql_sized_malloc(int nByte)
{
sqlite3_int64 *p;
assert(nByte > 0);
- nByte = ROUND8(nByte);
+ //nByte = ROUND8(nByte);
p = malloc(nByte + 8);
if (p) {
p[0] = nByte;
@@ -312,9 +312,9 @@ sqlite3MallocAlarm(int nByte)
static int
mallocWithAlarm(int n, void **pp)
{
- int nFull;
+ int nFull = n;
void *p;
- nFull = ROUND8(n);
+ //nFull = ROUND8(n);
sqlite3StatusHighwater(SQLITE_STATUS_MALLOC_SIZE, n);
if (mem0.alarmThreshold > 0) {
sqlite3_int64 nUsed =
To observe buffer-overflow run this query
(it is taken from sql-tap/table.test.lua):
EXPLAIN CREATE TABLE test1(f1 int primary key);
src/box/sql/build.c | 19 ++++++++++++++++---
src/box/sql/trigger.c | 4 ++--
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 52f0bde15..be51028e3 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1140,7 +1140,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
if (index_parts == NULL)
goto error;
char *raw = sqlite3DbMallocRaw(parse->db,
- index_opts_sz +index_parts_sz);
+ index_opts_sz + index_parts_sz + 1);
if (raw == NULL)
return;
memcpy(raw, index_opts, index_opts_sz);
@@ -1148,6 +1148,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
raw += index_opts_sz;
memcpy(raw, index_parts, index_parts_sz);
index_parts = raw;
+ index_parts[index_parts_sz] = '\0';
if (parse->pNewTable != NULL) {
sqlite3VdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg);
@@ -1209,8 +1210,18 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
char *table_stmt = sql_encode_table(region, table, &table_stmt_sz);
if (table_stmt == NULL)
goto error;
+ /*
+ * If we are executing EXPLAIN query or vdbe_debug mode
+ * is enabled, we should terminate string with \0:
+ * in internals it is copied to be displayed.
+ * In turn, to copy string strlen() is called:
+ * see handling of P4 in sqlite3VdbeList().
+ * Without null termination call of strlen() may lead to
+ * buffer-overflow. This bug was detected by ASAN.
+ */
+
char *raw = sqlite3DbMallocRaw(pParse->db,
- table_stmt_sz + table_opts_stmt_sz);
+ table_stmt_sz + table_opts_stmt_sz + 1);
if (raw == NULL)
return;
@@ -1219,6 +1230,7 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
raw += table_opts_stmt_sz;
memcpy(raw, table_stmt, table_stmt_sz);
table_stmt = raw;
+ table_stmt[table_stmt_sz] = '\0';
sqlite3VdbeAddOp2(v, OP_SCopy, iSpaceId, iFirstCol /* spaceId */ );
sqlite3VdbeAddOp2(v, OP_Integer, effective_user()->uid,
@@ -1397,7 +1409,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
* as dynamic and releases memory.
*/
char *raw = sqlite3DbMallocRaw(parse_context->db,
- parent_links_size + child_links_size);
+ parent_links_size + child_links_size + 1);
if (raw == NULL)
return;
memcpy(raw, parent_links, parent_links_size);
@@ -1405,6 +1417,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
raw += parent_links_size;
memcpy(raw, child_links, child_links_size);
child_links = raw;
+ child_links[child_links_size] = '\0';
sqlite3VdbeAddOp4(vdbe, OP_Blob, child_links_size, constr_tuple_reg + 7,
SQL_SUBTYPE_MSGPACK, child_links, P4_STATIC);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 482f40926..cd4d3fb47 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -213,11 +213,11 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
parse->rc = SQL_TARANTOOL_ERROR;
goto cleanup;
}
- char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
+ char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz + 1);
if (opts_buff == NULL)
goto cleanup;
memcpy(opts_buff, data, opts_buff_sz);
-
+ opts_buff[opts_buff_sz] = '\0';
trigger_name = sqlite3DbStrDup(db, trigger_name);
if (trigger_name == NULL) {
sqlite3DbFree(db, opts_buff);
--
2.15.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: terminate with \0 encoded msgpack
2018-12-11 16:21 [tarantool-patches] [PATCH] sql: terminate with \0 encoded msgpack Nikita Pettik
@ 2018-12-12 11:40 ` Vladislav Shpilevoy
2018-12-13 13:53 ` n.pettik
0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-12 11:40 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik
Hi! Thanks for the patch! See 1 comment below.
On 11/12/2018 19:21, Nikita Pettik wrote:
> During table, index, constraint or trigger creation it is required to
> encode entry's parts/opts to msgpack for the next insertion into system
> space. Encoded msgpack is known to lack null termination at the end of
> binary string. Hence, while such string is passed to printf, strlen or
> other function which excpects null termination, buffer overflow may
> occur.
>
> This bug was hard to detect due to several reasons.
> Firstly, SQLite memory allocators align memory to 8-byte border. Hence,
> as a rule even ASAN doesn't detect overflow, since there is additional
> alignment which can be addressable.
> Secondly, msgpack can contain nulls through the encoded string. As a
> result, strlen returns not the real size of binary string, but something
> less.
>
> Note that null termination is required only encoded string is to be
> displayed (i.e. EXPLAIN statement is executed or vdbe_debug mode is
> enabled).
>
> Bug was discovered with help of AddressSanitizer.
>
> Closes #3868
> ---
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3868-buffer-overflow
> Issue: https://github.com/tarantool/tarantool/issues/3868
>
> * Notes to the one who will review *
>
> To enable ASAN on macOS:
>
> cmake -DENABLE_ASAN=ON .
>
> Also it may be required to apply this diff:
>
> diff --git a/src/fiber.c b/src/fiber.c
> index 7658a2294..743ceff7a 100644
> --- a/src/fiber.c
> +++ b/src/fiber.c
> @@ -54,7 +54,7 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
> __sanitizer_start_switch_fiber((will_switch_back) ? &var_name : NULL, \
> (bottom), (size))
> #define ASAN_FINISH_SWITCH_FIBER(var_name) \
> - __sanitizer_finish_switch_fiber(var_name);
> + __sanitizer_finish_switch_fiber(var_name, 0, 0);
>
> To turn off SQLite 8-byte allocation alignment apply this diff:
>
> diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c
> index 9526cce5e..fd206632b 100644
> --- a/src/box/sql/malloc.c
> +++ b/src/box/sql/malloc.c
> @@ -49,7 +49,7 @@ sql_sized_malloc(int nByte)
> {
> sqlite3_int64 *p;
> assert(nByte > 0);
> - nByte = ROUND8(nByte);
> + //nByte = ROUND8(nByte);
> p = malloc(nByte + 8);
> if (p) {
> p[0] = nByte;
> @@ -312,9 +312,9 @@ sqlite3MallocAlarm(int nByte)
> static int
> mallocWithAlarm(int n, void **pp)
> {
> - int nFull;
> + int nFull = n;
> void *p;
> - nFull = ROUND8(n);
> + //nFull = ROUND8(n);
> sqlite3StatusHighwater(SQLITE_STATUS_MALLOC_SIZE, n);
> if (mem0.alarmThreshold > 0) {
> sqlite3_int64 nUsed =
>
> To observe buffer-overflow run this query
> (it is taken from sql-tap/table.test.lua):
>
> EXPLAIN CREATE TABLE test1(f1 int primary key);
Please, show where exactly strlen() is called on msgpack? I
can not find this place. Also, terminating msgpack with 0 is
a crutch, IMO. You will be able to print msgpack and use strlen(),
but as you said, it will not be valid msgpack. I will not be
able to decode it. This is because of msgpack having zeroes in a
middle. So those columns in EXPLAIN with invalid msgpack makes no
sense to show in such a case - invalid msgpack is useless.
I remember, that msgpack 0 termination existed in earlier
SQLite + Tarantool versions, but I removed it deliberately so
as to return it without string methods usage.
All places, which you crutched up with 0 termination, have
SQL_SUBTYPE_MSGPACK subtype, using which you can determine
if it is msgpack.
I understand, that from EXPLAIN we can not return Lua maps/arrays
since 6th column of EXPLAIN output is declared as a string. But
we can decode msgpack into JSON or YAML. This output would be
both parsable and readable.
>
> src/box/sql/build.c | 19 ++++++++++++++++---
> src/box/sql/trigger.c | 4 ++--
> 2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 52f0bde15..be51028e3 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1140,7 +1140,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
> if (index_parts == NULL)
> goto error;
> char *raw = sqlite3DbMallocRaw(parse->db,
> - index_opts_sz +index_parts_sz);
> + index_opts_sz + index_parts_sz + 1);
> if (raw == NULL)
> return;
> memcpy(raw, index_opts, index_opts_sz);
> @@ -1148,6 +1148,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
> raw += index_opts_sz;
> memcpy(raw, index_parts, index_parts_sz);
> index_parts = raw;
> + index_parts[index_parts_sz] = '\0';
>
> if (parse->pNewTable != NULL) {
> sqlite3VdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg);
> @@ -1209,8 +1210,18 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
> char *table_stmt = sql_encode_table(region, table, &table_stmt_sz);
> if (table_stmt == NULL)
> goto error;
> + /*
> + * If we are executing EXPLAIN query or vdbe_debug mode
> + * is enabled, we should terminate string with \0:
> + * in internals it is copied to be displayed.
> + * In turn, to copy string strlen() is called:
> + * see handling of P4 in sqlite3VdbeList().
> + * Without null termination call of strlen() may lead to
> + * buffer-overflow. This bug was detected by ASAN.
> + */
> +
> char *raw = sqlite3DbMallocRaw(pParse->db,
> - table_stmt_sz + table_opts_stmt_sz);
> + table_stmt_sz + table_opts_stmt_sz + 1);
> if (raw == NULL)
> return;
>
> @@ -1219,6 +1230,7 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
> raw += table_opts_stmt_sz;
> memcpy(raw, table_stmt, table_stmt_sz);
> table_stmt = raw;
> + table_stmt[table_stmt_sz] = '\0';
>
> sqlite3VdbeAddOp2(v, OP_SCopy, iSpaceId, iFirstCol /* spaceId */ );
> sqlite3VdbeAddOp2(v, OP_Integer, effective_user()->uid,
> @@ -1397,7 +1409,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
> * as dynamic and releases memory.
> */
> char *raw = sqlite3DbMallocRaw(parse_context->db,
> - parent_links_size + child_links_size);
> + parent_links_size + child_links_size + 1);
> if (raw == NULL)
> return;
> memcpy(raw, parent_links, parent_links_size);
> @@ -1405,6 +1417,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
> raw += parent_links_size;
> memcpy(raw, child_links, child_links_size);
> child_links = raw;
> + child_links[child_links_size] = '\0';
>
> sqlite3VdbeAddOp4(vdbe, OP_Blob, child_links_size, constr_tuple_reg + 7,
> SQL_SUBTYPE_MSGPACK, child_links, P4_STATIC);
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 482f40926..cd4d3fb47 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -213,11 +213,11 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
> parse->rc = SQL_TARANTOOL_ERROR;
> goto cleanup;
> }
> - char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
> + char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz + 1);
> if (opts_buff == NULL)
> goto cleanup;
> memcpy(opts_buff, data, opts_buff_sz);
> -
> + opts_buff[opts_buff_sz] = '\0';
> trigger_name = sqlite3DbStrDup(db, trigger_name);
> if (trigger_name == NULL) {
> sqlite3DbFree(db, opts_buff);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: terminate with \0 encoded msgpack
2018-12-12 11:40 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-13 13:53 ` n.pettik
2018-12-13 16:30 ` Vladislav Shpilevoy
0 siblings, 1 reply; 4+ messages in thread
From: n.pettik @ 2018-12-13 13:53 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> Please, show where exactly strlen() is called on msgpack?
Trace which leads to overflow is shown in issue description:
https://github.com/tarantool/tarantool/issues/3868
==55841==ERROR: AddressSanitizer: heap-buffer-overflow
#0 0x10384026a in wrap_strlen (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x1626a)
#1 0x100ce617b in sqlite3Strlen30 util.c:129
#2 0x100d5acfb in sqlite3VdbeMemSetStr vdbemem.c:908
#3 0x100d3f451 in sqlite3VdbeList vdbeaux.c:1685
#4 0x100d2c86d in sqlite3Step vdbeapi.c:573
#5 0x100d2bab2 in sqlite3_step vdbeapi.c:629
#6 0x100365d7a in lua_sql_execute (tarantool:x86_64+0x100365d7a)
#7 0x10047188c in lj_BC_FUNCC (tarantool:x86_64+0x10047188c)
> I can not find this place. Also, terminating msgpack with 0 is
> a crutch, IMO. You will be able to print msgpack and use strlen(),
> but as you said, it will not be valid msgpack. I will not be
> able to decode it. This is because of msgpack having zeroes in a
> middle. So those columns in EXPLAIN with invalid msgpack makes no
> sense to show in such a case - invalid msgpack is useless.
Well, that’s true. We can operate with Blob in other way: since
its length is always comes as first argument of OP_Blob, we
can rely only on that length and disregard null terminations at all.
Here’s kind of workaround for this approach:
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index fc805e3aa..06f07dbb9 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1213,8 +1213,13 @@ displayComment(const Op * pOp, /* The opcode to be commented */
if (c == 'P') {
c = zSynopsis[++ii];
if (c == '4') {
- sqlite3_snprintf(nTemp - jj, zTemp + jj,
- "%s", zP4);
+ if (pOp->opcode == OP_Blob) {
+ memcpy(zTemp + jj, zP4, pOp->p1);
+ } else {
+ sqlite3_snprintf(nTemp - jj,
+ zTemp + jj,
+ "%s", zP4);
+ }
} else if (c == 'X') {
sqlite3_snprintf(nTemp - jj, zTemp + jj,
"%s", pOp->zComment);
@@ -1418,8 +1423,7 @@ sqlite3VdbePrintOp(FILE * pOut, int pc, Op * pOp)
char *zP4;
char zPtr[50];
char zCom[100];
- static const char *zFormat1 =
- "%4d> %4d %-13s %4d %4d %4d %-13s %.2X %s\n";
+ static const char *zFormat1 = "%4d> %4d %-13s %4d %4d %4d ";
if (pOut == 0)
pOut = stdout;
zP4 = displayP4(pOp, zPtr, sizeof(zPtr));
@@ -1433,8 +1437,13 @@ sqlite3VdbePrintOp(FILE * pOut, int pc, Op * pOp)
* information from the vdbe.c source text
*/
fprintf(pOut, zFormat1, fiber_self()->fid, pc,
- sqlite3OpcodeName(pOp->opcode), pOp->p1, pOp->p2, pOp->p3, zP4,
- pOp->p5, zCom);
+ sqlite3OpcodeName(pOp->opcode), pOp->p1, pOp->p2, pOp->p3);
+ if (pOp->opcode == OP_Blob) {
+ fwrite(zP4, sizeof(char), pOp->p1, pOut);
+ fprintf(pOut, "%.2X %s \n", pOp->p5, zCom);
+ } else {
+ fprintf(pOut, "%-13s %.2X %s \n", zP4, pOp->p5, zCom);
+ }
fflush(pOut);
}
#endif
@@ -1681,8 +1690,13 @@ sqlite3VdbeList(Vdbe * p)
pMem->flags = MEM_Str | MEM_Term;
zP4 = displayP4(pOp, pMem->z, pMem->szMalloc);
if (zP4 != pMem->z) {
- pMem->n = 0;
- sqlite3VdbeMemSetStr(pMem, zP4, -1, 1, 0);
+ /*
+ * BLOB may contain encoded mspack which
+ * disrespects null termination.
+ */
+ int str_len = pOp->opcode == OP_Blob ? pOp->p1 : 0;
+ pMem->n = str_len;
+ sqlite3VdbeMemSetStr(pMem, zP4, str_len, 1, 0);
} else {
It may fail on some tests, I didn’t check it. But it certainly doesn’t result in
buffer-overflow, that I’ve checked.
If this solution still looks ugly/unacceptable, lets consider other options.
> I remember, that msgpack 0 termination existed in earlier
> SQLite + Tarantool versions, but I removed it deliberately so
> as to return it without string methods usage.
>
> All places, which you crutched up with 0 termination, have
> SQL_SUBTYPE_MSGPACK subtype, using which you can determine
> if it is msgpack.
On the other hand I guess not only msgpacks can contain nulls while
being encoded..See sample above.
> I understand, that from EXPLAIN we can not return Lua maps/arrays
> since 6th column of EXPLAIN output is declared as a string. But
> we can decode msgpack into JSON or YAML. This output would be
> both parsable and readable.
I don’t think that decoding msgpack for user is fair.
IMHO blob should come as is.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: terminate with \0 encoded msgpack
2018-12-13 13:53 ` n.pettik
@ 2018-12-13 16:30 ` Vladislav Shpilevoy
0 siblings, 0 replies; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-13 16:30 UTC (permalink / raw)
To: n.pettik, tarantool-patches
On 13/12/2018 16:53, n.pettik wrote:
>
>> Please, show where exactly strlen() is called on msgpack?
>
> Trace which leads to overflow is shown in issue description:
> https://github.com/tarantool/tarantool/issues/3868
>
> ==55841==ERROR: AddressSanitizer: heap-buffer-overflow
> #0 0x10384026a in wrap_strlen (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x1626a)
> #1 0x100ce617b in sqlite3Strlen30 util.c:129
> #2 0x100d5acfb in sqlite3VdbeMemSetStr vdbemem.c:908
> #3 0x100d3f451 in sqlite3VdbeList vdbeaux.c:1685
> #4 0x100d2c86d in sqlite3Step vdbeapi.c:573
> #5 0x100d2bab2 in sqlite3_step vdbeapi.c:629
> #6 0x100365d7a in lua_sql_execute (tarantool:x86_64+0x100365d7a)
> #7 0x10047188c in lj_BC_FUNCC (tarantool:x86_64+0x10047188c)
>
>> I can not find this place. Also, terminating msgpack with 0 is
>> a crutch, IMO. You will be able to print msgpack and use strlen(),
>> but as you said, it will not be valid msgpack. I will not be
>> able to decode it. This is because of msgpack having zeroes in a
>> middle. So those columns in EXPLAIN with invalid msgpack makes no
>> sense to show in such a case - invalid msgpack is useless.
>
> Well, that’s true. We can operate with Blob in other way: since
> its length is always comes as first argument of OP_Blob, we
> can rely only on that length and disregard null terminations at all.
> Here’s kind of workaround for this approach:
>
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index fc805e3aa..06f07dbb9 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1213,8 +1213,13 @@ displayComment(const Op * pOp, /* The opcode to be commented */
> if (c == 'P') {
> c = zSynopsis[++ii];
> if (c == '4') {
> - sqlite3_snprintf(nTemp - jj, zTemp + jj,
> - "%s", zP4);
> + if (pOp->opcode == OP_Blob) {
> + memcpy(zTemp + jj, zP4, pOp->p1);
> + } else {
> + sqlite3_snprintf(nTemp - jj,
> + zTemp + jj,
> + "%s", zP4);
> + }
> } else if (c == 'X') {
> sqlite3_snprintf(nTemp - jj, zTemp + jj,
> "%s", pOp->zComment);
> @@ -1418,8 +1423,7 @@ sqlite3VdbePrintOp(FILE * pOut, int pc, Op * pOp)
> char *zP4;
> char zPtr[50];
> char zCom[100];
> - static const char *zFormat1 =
> - "%4d> %4d %-13s %4d %4d %4d %-13s %.2X %s\n";
> + static const char *zFormat1 = "%4d> %4d %-13s %4d %4d %4d ";
> if (pOut == 0)
> pOut = stdout;
> zP4 = displayP4(pOp, zPtr, sizeof(zPtr));
> @@ -1433,8 +1437,13 @@ sqlite3VdbePrintOp(FILE * pOut, int pc, Op * pOp)
> * information from the vdbe.c source text
> */
> fprintf(pOut, zFormat1, fiber_self()->fid, pc,
> - sqlite3OpcodeName(pOp->opcode), pOp->p1, pOp->p2, pOp->p3, zP4,
> - pOp->p5, zCom);
> + sqlite3OpcodeName(pOp->opcode), pOp->p1, pOp->p2, pOp->p3);
> + if (pOp->opcode == OP_Blob) {
> + fwrite(zP4, sizeof(char), pOp->p1, pOut);
> + fprintf(pOut, "%.2X %s \n", pOp->p5, zCom);
> + } else {
> + fprintf(pOut, "%-13s %.2X %s \n", zP4, pOp->p5, zCom);
> + }
> fflush(pOut);
> }
> #endif
> @@ -1681,8 +1690,13 @@ sqlite3VdbeList(Vdbe * p)
> pMem->flags = MEM_Str | MEM_Term;
> zP4 = displayP4(pOp, pMem->z, pMem->szMalloc);
> if (zP4 != pMem->z) {
> - pMem->n = 0;
> - sqlite3VdbeMemSetStr(pMem, zP4, -1, 1, 0);
> + /*
> + * BLOB may contain encoded mspack which
> + * disrespects null termination.
> + */
> + int str_len = pOp->opcode == OP_Blob ? pOp->p1 : 0;
> + pMem->n = str_len;
> + sqlite3VdbeMemSetStr(pMem, zP4, str_len, 1, 0);
> } else {
>
> It may fail on some tests, I didn’t check it. But it certainly doesn’t result in
> buffer-overflow, that I’ve checked.
>
> If this solution still looks ugly/unacceptable, lets consider other options.
It is better, but msgpack in EXPLAIN still is unreadable.
>
>> I remember, that msgpack 0 termination existed in earlier
>> SQLite + Tarantool versions, but I removed it deliberately so
>> as to return it without string methods usage.
>>
>> All places, which you crutched up with 0 termination, have
>> SQL_SUBTYPE_MSGPACK subtype, using which you can determine
>> if it is msgpack.
>
> On the other hand I guess not only msgpacks can contain nulls while
> being encoded..See sample above.>
>> I understand, that from EXPLAIN we can not return Lua maps/arrays
>> since 6th column of EXPLAIN output is declared as a string. But
>> we can decode msgpack into JSON or YAML. This output would be
>> both parsable and readable.
>
> I don’t think that decoding msgpack for user is fair.
> IMHO blob should come as is.
>
I do not agree that blob should come as is. It does not
already, in fact. We return decoded msgpack as Lua objects when
use box.sql.execute() to select from a space.
What is more, I think that in EXPLAIN, in Vdbe comments, raw
msgpack makes no sense. It is useless, unreadable. A sense of
a comment to be read by a human and help to detect some problems
or something. But I can not read raw msgpack. So I still think
that EXPLAIN should decode msgpack into a readable form. For me
it does not matter: YAML, JSON or lua objects.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-13 16:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-11 16:21 [tarantool-patches] [PATCH] sql: terminate with \0 encoded msgpack Nikita Pettik
2018-12-12 11:40 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-13 13:53 ` n.pettik
2018-12-13 16:30 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox