[tarantool-patches] Re: [PATCH] sql: terminate with \0 encoded msgpack

[n.pettik]

> 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.