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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.