[tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Nov 22 21:31:14 MSK 2018
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 at 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()
> -
>
More information about the Tarantool-patches
mailing list