[tarantool-patches] [PATCH v2 1/1] sql: do not analyze incorrect statistics

[tarantool-patches] [PATCH v2 1/1] sql: do not analyze incorrect statistics

[imeevma]

Hi! Thank you for review! My answers and new version below. There
won't be diff between versions as it was a lot easier and faster
to write this patch anew.

https://github.com/tarantool/tarantool/issues/3866
https://github.com/tarantool/tarantool/tree/imeevma/gh-3866-ignore-wrong-data-in-_sql_stat

> Could you please provide more detailed descriptions? Like
> what was exact reason of crash, what did you do etc.
> From backtrace I see that segfault takes place somewhere
> in lua_pushstring().
Changed commit-message.

> Why do you need all these refactoring things? AFAIU the only thing
> you need to do is return 0 instead of -1; otherwise error will be handled
> without error message which results in segfault.
> Ofc, index_count should be incremented after index verification.
Fixed.

> The same question here: do you really need this refactoring?
> Let’s make patch as small as you can. I don’t think that such
> refactoring makes code cleaner, it only complicates review process.
> Tell me if I am wrong.
Fixed.

> Nit: I would better say “wrong space name leads to segfault”, since
> tests on wrong statistics inserted to stat tables are above your tests.
Fixed.

> Add also test, where name of table is correct, but name of index is wrong.
Added.


New version:

commit 1b43714d03ae2a2f7042415aed3d60e1a41034f0
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Dec 19 21:12:17 2018 +0300

    sql: do not analyze incorrect statistics
    
    Some errors that occurred during the analysis were processed
    without an error message. However, these errors should not be
    processed, as they show that something is wrong with the data
    received. After this patch, entries in _sql_stat* with the wrong
    space or index name will be ignored.
    
    Closes #3866

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 3f49280..8eabe97 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1224,10 +1224,10 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
 		return 0;
 	struct analysis_index_info *info = (struct analysis_index_info *) data;
 	assert(info->stats != NULL);
-	struct index_stat *stat = &info->stats[info->index_count++];
+	struct index_stat *stat = &info->stats[info->index_count];
 	struct space *space = space_by_name(argv[0]);
 	if (space == NULL)
-		return -1;
+		return 0;
 	struct index *index;
 	uint32_t iid = box_index_id_by_name(space->def->id, argv[1],
 					    strlen(argv[1]));
@@ -1239,10 +1239,11 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
 		index = space_index(space, iid);
 	} else {
 		if (sqlite3_stricmp(argv[0], argv[1]) != 0)
-			return -1;
+			return 0;
 		index = space_index(space, 0);
 	}
 	assert(index != NULL);
+	info->index_count++;
 	/*
 	 * Additional field is used to describe total
 	 * count of tuples in index. Although now all
@@ -1395,15 +1396,18 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 			continue;
 		uint32_t sample_count = sqlite3_column_int(stmt, 2);
 		struct space *space = space_by_name(space_name);
-		assert(space != NULL);
+		if (space == NULL)
+			continue;
 		struct index *index;
 		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
-		if (sqlite3_stricmp(space_name, index_name) == 0 &&
-		    iid == BOX_ID_NIL)
+		if (iid == BOX_ID_NIL) {
+			if (sqlite3_stricmp(space_name, index_name) != 0)
+				continue;
 			index = space_index(space, 0);
-		else
+		} else {
 			index = space_index(space, iid);
+		}
 		assert(index != NULL);
 		uint32_t column_count = index->def->key_def->part_count;
 		struct index_stat *stat = &stats[current_idx_count];
@@ -1463,7 +1467,8 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 		if (index_name == NULL)
 			continue;
 		struct space *space = space_by_name(space_name);
-		assert(space != NULL);
+		if (space == NULL)
+			continue;
 		struct index *index;
 		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
@@ -1471,7 +1476,7 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 			index = space_index(space, iid);
 		} else {
 			if (sqlite3_stricmp(space_name, index_name) != 0)
-				return -1;
+				continue;
 			index = space_index(space, 0);
 		}
 		assert(index != NULL);
@@ -1544,7 +1549,8 @@ load_stat_to_index(struct sqlite3 *db, const char *sql_select_load,
 		if (index_name == NULL)
 			continue;
 		struct space *space = space_by_name(space_name);
-		assert(space != NULL);
+		if (space == NULL)
+			continue;
 		struct index *index;
 		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
@@ -1552,7 +1558,7 @@ load_stat_to_index(struct sqlite3 *db, const char *sql_select_load,
 			index = space_index(space, iid);
 		} else {
 			if (sqlite3_stricmp(space_name, index_name) != 0)
-				return -1;
+				continue;
 			index = space_index(space, 0);
 		}
 		assert(index != NULL);
diff --git a/test/sql-tap/analyze1.test.lua b/test/sql-tap/analyze1.test.lua
index ea414e9..7330b93 100755
--- a/test/sql-tap/analyze1.test.lua
+++ b/test/sql-tap/analyze1.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(38)
+test:plan(42)
 
 --!./tcltestrunner.lua
 -- 2005 July 22
@@ -561,4 +561,57 @@ test:do_execsql_test(
 --   }
 -- } {1 {malformed database schema (sqlite_stat1)}}
 
+--
+-- gh-3866 Wrong space name in _sql_stat* leads to segfault
+--
+test:do_execsql_test(
+    "analyze-7.1",
+    [[
+        DELETE FROM "_sql_stat1";
+        DELETE FROM "_sql_stat4";
+        DROP TABLE IF EXISTS t0;
+        CREATE TABLE t0(id INTEGER PRIMARY KEY);
+        INSERT INTO t0 VALUES (1);
+        INSERT INTO "_sql_stat1" VALUES('abc', 'bca', 'cab');
+        ANALYZE t0;
+    ]], {
+        -- <analyze-7.1>
+        -- </analyze-7.1>
+    })
+
+test:do_execsql_test(
+    "analyze-7.2",
+    [[
+        INSERT INTO "_sql_stat4" VALUES('abc', 'bca', 'cab', 'acb', 'bac', 'cba');
+        ANALYZE t0;
+    ]], {
+        -- <analyze-7.2>
+        -- </analyze-7.2>
+    })
+
+test:do_execsql_test(
+    "analyze-7.3",
+    [[
+        DELETE FROM "_sql_stat1";
+        DELETE FROM "_sql_stat4";
+        DROP TABLE IF EXISTS t1;
+        CREATE TABLE t1(id INTEGER PRIMARY KEY);
+        INSERT INTO t1 VALUES (1);
+        INSERT INTO "_sql_stat1" VALUES('T0', 'WRONG_NAME', 'something');
+        ANALYZE t1;
+    ]], {
+        -- <analyze-7.3>
+        -- </analyze-7.3>
+    })
+
+test:do_execsql_test(
+    "analyze-7.4",
+    [[
+        INSERT INTO "_sql_stat4" VALUES('T0', 'WRONG_NAME', 'value', 'value', 'value', 'value');
+        ANALYZE t1;
+    ]], {
+        -- <analyze-7.4>
+        -- </analyze-7.4>
+    })
+
 test:finish_test()
-- 
2.7.4

[tarantool-patches] Re: [PATCH v2 1/1] sql: do not analyze incorrect statistics

[n.pettik]

> New version:
> 
> commit 1b43714d03ae2a2f7042415aed3d60e1a41034f0
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Wed Dec 19 21:12:17 2018 +0300
> 
>    sql: do not analyze incorrect statistics
> 
>    Some errors that occurred during the analysis were processed
>    without an error message. However, these errors should not be
>    processed, as they show that something is wrong with the data
>    received. After this patch, entries in _sql_stat* with the wrong
>    space or index name will be ignored.
> 
>    Closes #3866

Now it looks pretty good. LGTM.

[tarantool-patches] [PATCH v2 1/1] sql: do not analyze incorrect statistics

[imeevma]

Vlad, please, do second review. I rebased it to newest 2.1.

https://github.com/tarantool/tarantool/issues/3866
https://github.com/tarantool/tarantool/tree/imeevma/gh-3866-ignore-wrong-data-in-_sql_stat


On 12/24/18 5:15 PM, n.pettik wrote:
>
>> New version:
>>
>> commit 1b43714d03ae2a2f7042415aed3d60e1a41034f0
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date:   Wed Dec 19 21:12:17 2018 +0300
>>
> Now it looks pretty good. LGTM.
>

commit 9a564cdf194685401f6b57d69408282d6326b856
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Dec 19 21:12:17 2018 +0300

    sql: do not analyze incorrect statistics
    
    Some errors that occurred during the analysis were processed
    without an error message. However, these errors should not be
    processed, as they show that something is wrong with the data
    received. After this patch, entries in _sql_stat* with the wrong
    space or index name will be ignored.
    
    Closes #3866

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 51c63fa..d1fa4ec 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1224,10 +1224,10 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
 		return 0;
 	struct analysis_index_info *info = (struct analysis_index_info *) data;
 	assert(info->stats != NULL);
-	struct index_stat *stat = &info->stats[info->index_count++];
+	struct index_stat *stat = &info->stats[info->index_count];
 	struct space *space = space_by_name(argv[0]);
 	if (space == NULL)
-		return -1;
+		return 0;
 	struct index *index;
 	uint32_t iid = box_index_id_by_name(space->def->id, argv[1],
 					    strlen(argv[1]));
@@ -1239,10 +1239,11 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
 		index = space_index(space, iid);
 	} else {
 		if (sqlite3_stricmp(argv[0], argv[1]) != 0)
-			return -1;
+			return 0;
 		index = space_index(space, 0);
 	}
 	assert(index != NULL);
+	info->index_count++;
 	/*
 	 * Additional field is used to describe total
 	 * count of tuples in index. Although now all
@@ -1395,15 +1396,18 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 			continue;
 		uint32_t sample_count = sqlite3_column_int(stmt, 2);
 		struct space *space = space_by_name(space_name);
-		assert(space != NULL);
+		if (space == NULL)
+			continue;
 		struct index *index;
 		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
-		if (sqlite3_stricmp(space_name, index_name) == 0 &&
-		    iid == BOX_ID_NIL)
+		if (iid == BOX_ID_NIL) {
+			if (sqlite3_stricmp(space_name, index_name) != 0)
+				continue;
 			index = space_index(space, 0);
-		else
+		} else {
 			index = space_index(space, iid);
+		}
 		assert(index != NULL);
 		uint32_t column_count = index->def->key_def->part_count;
 		struct index_stat *stat = &stats[current_idx_count];
@@ -1463,7 +1467,8 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 		if (index_name == NULL)
 			continue;
 		struct space *space = space_by_name(space_name);
-		assert(space != NULL);
+		if (space == NULL)
+			continue;
 		struct index *index;
 		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
@@ -1471,7 +1476,7 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
 			index = space_index(space, iid);
 		} else {
 			if (sqlite3_stricmp(space_name, index_name) != 0)
-				return -1;
+				continue;
 			index = space_index(space, 0);
 		}
 		assert(index != NULL);
@@ -1544,7 +1549,8 @@ load_stat_to_index(struct sqlite3 *db, const char *sql_select_load,
 		if (index_name == NULL)
 			continue;
 		struct space *space = space_by_name(space_name);
-		assert(space != NULL);
+		if (space == NULL)
+			continue;
 		struct index *index;
 		uint32_t iid = box_index_id_by_name(space->def->id, index_name,
 						    strlen(index_name));
@@ -1552,7 +1558,7 @@ load_stat_to_index(struct sqlite3 *db, const char *sql_select_load,
 			index = space_index(space, iid);
 		} else {
 			if (sqlite3_stricmp(space_name, index_name) != 0)
-				return -1;
+				continue;
 			index = space_index(space, 0);
 		}
 		assert(index != NULL);
diff --git a/test/sql-tap/analyze1.test.lua b/test/sql-tap/analyze1.test.lua
index ea414e9..7330b93 100755
--- a/test/sql-tap/analyze1.test.lua
+++ b/test/sql-tap/analyze1.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(38)
+test:plan(42)
 
 --!./tcltestrunner.lua
 -- 2005 July 22
@@ -561,4 +561,57 @@ test:do_execsql_test(
 --   }
 -- } {1 {malformed database schema (sqlite_stat1)}}
 
+--
+-- gh-3866 Wrong space name in _sql_stat* leads to segfault
+--
+test:do_execsql_test(
+    "analyze-7.1",
+    [[
+        DELETE FROM "_sql_stat1";
+        DELETE FROM "_sql_stat4";
+        DROP TABLE IF EXISTS t0;
+        CREATE TABLE t0(id INTEGER PRIMARY KEY);
+        INSERT INTO t0 VALUES (1);
+        INSERT INTO "_sql_stat1" VALUES('abc', 'bca', 'cab');
+        ANALYZE t0;
+    ]], {
+        -- <analyze-7.1>
+        -- </analyze-7.1>
+    })
+
+test:do_execsql_test(
+    "analyze-7.2",
+    [[
+        INSERT INTO "_sql_stat4" VALUES('abc', 'bca', 'cab', 'acb', 'bac', 'cba');
+        ANALYZE t0;
+    ]], {
+        -- <analyze-7.2>
+        -- </analyze-7.2>
+    })
+
+test:do_execsql_test(
+    "analyze-7.3",
+    [[
+        DELETE FROM "_sql_stat1";
+        DELETE FROM "_sql_stat4";
+        DROP TABLE IF EXISTS t1;
+        CREATE TABLE t1(id INTEGER PRIMARY KEY);
+        INSERT INTO t1 VALUES (1);
+        INSERT INTO "_sql_stat1" VALUES('T0', 'WRONG_NAME', 'something');
+        ANALYZE t1;
+    ]], {
+        -- <analyze-7.3>
+        -- </analyze-7.3>
+    })
+
+test:do_execsql_test(
+    "analyze-7.4",
+    [[
+        INSERT INTO "_sql_stat4" VALUES('T0', 'WRONG_NAME', 'value', 'value', 'value', 'value');
+        ANALYZE t1;
+    ]], {
+        -- <analyze-7.4>
+        -- </analyze-7.4>
+    })
+
 test:finish_test()
-- 
2.7.4