From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 851CA2FFC4 for ; Thu, 22 Nov 2018 13:31:21 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4G7_ffgJ_F82 for ; Thu, 22 Nov 2018 13:31:21 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1BDAA2C0B4 for ; Thu, 22 Nov 2018 13:31:21 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT References: <9DCBE39C-92D1-4093-B8BE-82A3CD6BD9AF@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6104171d-f355-733c-d01b-016127ec389a@tarantool.org> Date: Thu, 22 Nov 2018 21:31:14 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Imeev Mergen , "n.pettik" , tarantool-patches@freelists.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 > 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() > - >