[tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

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

It was just a question, "why did you decide to do not
send type". It was not necessary to start sending UNKNOWN. I
think, that unknown type should be sent never.

On 22/11/2018 21:08, Imeev Mergen wrote:
> Hi! Thank you for review! Diff between versions and new patch
> below.
> 
> On 11/19/18 8:27 PM, n.pettik wrote:
> 
>>> EXPLAIN envokes SEGMENTATION FAULT when being executed through
>> Typo: invokes.
>>
>>> net.box. It happens due to column type of the result of this
>>> function being NULL.
>> I’ve checked and not only EXPLAIN results, but also EXPLAIN QUERY PLAN,
>> several pragmas (for instance, pragma table_info()) etc. So, initially problem
>> was not only in EXPLAIN statement. Please exposed commit message and
>> add tests on other cases.
>>
>> Also, why did you decide to avoid sending type at all, instead of sending
>> UNKNOWN for example?
> 
> *Diff between versions:*
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 5b747cf..cd809f8 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -529,6 +529,9 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>           return -1;
> 
>       for (int i = 0; i < column_count; ++i) {
> +        size_t size = mp_sizeof_map(2) +
> +                  mp_sizeof_uint(IPROTO_FIELD_NAME) +
> +                  mp_sizeof_uint(IPROTO_FIELD_TYPE);
>           const char *name = sqlite3_column_name(stmt, i);
>           const char *type = sqlite3_column_datatype(stmt, i);
>           /*
> @@ -537,27 +540,20 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>            * column_name simply returns them.
>            */
>           assert(name != NULL);
> -        size_t size = mp_sizeof_uint(IPROTO_FIELD_NAME) +
> -                  mp_sizeof_str(strlen(name));
> -        if (type != NULL) {
> -            size += mp_sizeof_map(2) +
> -                mp_sizeof_uint(IPROTO_FIELD_TYPE) +
> -                mp_sizeof_str(strlen(type));
> -        } else {
> -            size += mp_sizeof_map(1);
> -        }
> +        if (type == NULL)
> +            type = "UNKNOWN";
> +        size += mp_sizeof_str(strlen(name));
> +        size += mp_sizeof_str(strlen(type));
>           char *pos = (char *) obuf_alloc(out, size);
>           if (pos == NULL) {
>               diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>               return -1;
>           }
> -        pos = mp_encode_map(pos, 1 + (type != NULL));
> +        pos = mp_encode_map(pos, 2);
>           pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
>           pos = mp_encode_str(pos, name, strlen(name));
> -        if (type != NULL) {
> -            pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
> -            pos = mp_encode_str(pos, type, strlen(type));
> -        }
> +        pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
> +        pos = mp_encode_str(pos, type, strlen(type));
>       }
>       return 0;
>   }
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 342c6a7..c7063d9 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -644,7 +644,8 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
>       lua_createtable(L, count, 0);
>       for (uint32_t i = 0; i < count; ++i) {
>           uint32_t map_size = mp_decode_map(data);
> -        assert(map_size == 1 || map_size == 2);
> +        assert(map_size == 2);
> +        (void) map_size;
>           uint32_t key = mp_decode_uint(data);
>           assert(key == IPROTO_FIELD_NAME);
>           (void) key;
> @@ -653,13 +654,11 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
>           const char *str = mp_decode_str(data, &len);
>           lua_pushlstring(L, str, len);
>           lua_setfield(L, -2, "name");
> -        if (map_size == 2) {
> -            key = mp_decode_uint(data);
> -            assert(key == IPROTO_FIELD_TYPE);
> -            const char *type = mp_decode_str(data, &len);
> -            lua_pushlstring(L, type, len);
> -            lua_setfield(L, -2, "type");
> -        }
> +        key = mp_decode_uint(data);
> +        assert(key == IPROTO_FIELD_TYPE);
> +        const char *type = mp_decode_str(data, &len);
> +        lua_pushlstring(L, type, len);
> +        lua_setfield(L, -2, "type");
>           lua_rawseti(L, -2, i + 1);
>       }
>   }
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index 31e5d2e..f229427 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -774,9 +774,9 @@ cn:close()
>   ---
>   ...
>   --
> --- EXPLAIN could lead to segfault because it does not send column
> --- data type, but looks like DQL. Netbox had been trying to decode
> --- type for any DQL.
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
>   --
>   cn = remote.connect(box.cfg.listen)
>   ---
> @@ -785,6 +785,32 @@ cn = remote.connect(box.cfg.listen)
>   _ = cn:execute("EXPLAIN SELECT 1;")
>   ---
>   ...
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +---
> +...
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +---
> +- metadata:
> +  - name: cid
> +    type: UNKNOWN
> +  - name: name
> +    type: UNKNOWN
> +  - name: type
> +    type: UNKNOWN
> +  - name: notnull
> +    type: UNKNOWN
> +  - name: dflt_value
> +    type: UNKNOWN
> +  - name: pk
> +    type: UNKNOWN
> +  rows:
> +  - [0, 'ID', 'integer', 1, null, 1]
> +...
> +box.sql.execute('DROP TABLE test')
> +---
> +...
>   cn:close()
>   ---
>   ...
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index a0f0f15..9bcc7ef 100644
> --- a/test/sql/iproto.test.lua
> +++ b/test/sql/iproto.test.lua
> @@ -248,15 +248,21 @@ cn:execute('drop table test')
>   cn:close()
> 
>   --
> --- EXPLAIN could lead to segfault because it does not send column
> --- data type, but looks like DQL. Netbox had been trying to decode
> --- type for any DQL.
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
>   --
>   cn = remote.connect(box.cfg.listen)
> 
>   -- Segmentation fault when EXPLAIN executed using net.box.
>   _ = cn:execute("EXPLAIN SELECT 1;")
> 
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +box.sql.execute('DROP TABLE test')
> +
>   cn:close()
> 
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> 
> 
> *New version:*
> 
> commit 29dde0365922bfcec0d159b36c6454854adf34f7
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Nov 16 13:34:46 2018 +0300
> 
>      sql: Print column type even if it is NULL
> 
>      Some commands, e.g. EXPLAIN, invokes SEGMENTATION FAULT when being
>      executed through net.box. It happens due to column type of the
>      result of this function being NULL. This patch adds checks for
>      received column type.
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index e450444..cd809f8 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -540,6 +540,8 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>            * column_name simply returns them.
>            */
>           assert(name != NULL);
> +        if (type == NULL)
> +            type = "UNKNOWN";
>           size += mp_sizeof_str(strlen(name));
>           size += mp_sizeof_str(strlen(type));
>           char *pos = (char *) obuf_alloc(out, size);
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index d077ee8..f229427 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -773,6 +773,47 @@ cn:execute('drop table test')
>   cn:close()
>   ---
>   ...
> +--
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
> +--
> +cn = remote.connect(box.cfg.listen)
> +---
> +...
> +-- Segmentation fault when EXPLAIN executed using net.box.
> +_ = cn:execute("EXPLAIN SELECT 1;")
> +---
> +...
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +---
> +...
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +---
> +- metadata:
> +  - name: cid
> +    type: UNKNOWN
> +  - name: name
> +    type: UNKNOWN
> +  - name: type
> +    type: UNKNOWN
> +  - name: notnull
> +    type: UNKNOWN
> +  - name: dflt_value
> +    type: UNKNOWN
> +  - name: pk
> +    type: UNKNOWN
> +  rows:
> +  - [0, 'ID', 'integer', 1, null, 1]
> +...
> +box.sql.execute('DROP TABLE test')
> +---
> +...
> +cn:close()
> +---
> +...
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>   ---
>   ...
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index 1470eda..9bcc7ef 100644
> --- a/test/sql/iproto.test.lua
> +++ b/test/sql/iproto.test.lua
> @@ -247,10 +247,27 @@ cn:execute('drop table test')
> 
>   cn:close()
> 
> +--
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
> +--
> +cn = remote.connect(box.cfg.listen)
> +
> +-- Segmentation fault when EXPLAIN executed using net.box.
> +_ = cn:execute("EXPLAIN SELECT 1;")
> +
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +box.sql.execute('DROP TABLE test')
> +
> +cn:close()
> +
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>   box.schema.user.revoke('guest', 'create', 'space')
>   space = nil
> 
>   -- Cleanup xlog
>   box.snapshot()
> -
>