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 CC99D2C757 for ; Thu, 22 Nov 2018 13:09:01 -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 9n5JA4gl3H1l for ; Thu, 22 Nov 2018 13:09:01 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 DDCEB27F64 for ; Thu, 22 Nov 2018 13:09:00 -0500 (EST) From: Imeev Mergen Subject: [tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT References: <9DCBE39C-92D1-4093-B8BE-82A3CD6BD9AF@tarantool.org> Message-ID: Date: Thu, 22 Nov 2018 21:08:56 +0300 MIME-Version: 1.0 In-Reply-To: <9DCBE39C-92D1-4093-B8BE-82A3CD6BD9AF@tarantool.org> Content-Type: multipart/alternative; boundary="------------B22295EC3FE551A95B036473" Content-Language: en-US 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: "n.pettik" , tarantool-patches@freelists.org Cc: Vladislav Shpilevoy This is a multi-part message in MIME format. --------------B22295EC3FE551A95B036473 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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() - --------------B22295EC3FE551A95B036473 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit 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()
-

--------------B22295EC3FE551A95B036473--