[Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[sergos]

From: Sergey Ostanevich <sergos@tarantool.org>

A number of places in sql.c uses direct access to box_process_rw() that
does not check read-only setting. Fixed by use of an intended interface
of box_process1().

Closes #5231

Branch: https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro
Issue: https://github.com/tarantool/tarantool/issues/5231

---
 src/box/sql.c                                 |  9 ++-
 .../gh-5231-box-execute-writes-in-ro.test.lua | 74 +++++++++++++++++++
 2 files changed, 79 insertions(+), 4 deletions(-)
 create mode 100755 test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index a551bffc3..11f4b8ee4 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -54,6 +54,7 @@
 #include "fk_constraint.h"
 #include "mpstream/mpstream.h"
 #include "sql_stmt_cache.h"
+#include "backtrace.h"
 
 static sql *db = NULL;
 
@@ -453,7 +454,7 @@ insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,
 	request.space_id = space->def->id;
 	request.type = type;
 	mp_tuple_assert(request.tuple, request.tuple_end);
-	return box_process_rw(&request, space, NULL);
+	return box_process1(&request, NULL);
 }
 
 int tarantoolsqlInsert(struct space *space, const char *tuple,
@@ -531,7 +532,7 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key,
 	request.space_id = space->def->id;
 	request.index_id = iid;
 	assert(space_index(space, iid)->def->opts.is_unique);
-	return box_process_rw(&request, space, &unused);
+	return box_process1(&request, &unused);
 }
 
 /*
@@ -597,7 +598,7 @@ int tarantoolsqlClearTable(struct space *space, uint32_t *tuple_count)
 		request.key = tuple_extract_key(tuple, pk->def->key_def,
 						MULTIKEY_NONE, &key_size);
 		request.key_end = request.key + key_size;
-		rc = box_process_rw(&request, space, &unused);
+		rc = box_process1(&request, &unused);
 		if (rc != 0) {
 			iterator_delete(iter);
 			return -1;
@@ -834,7 +835,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id)
 	request.key_end = key + sizeof(key);
 	request.type = IPROTO_UPDATE;
 	request.space_id = space_schema->def->id;
-	if (box_process_rw(&request, space_schema, &res) != 0 || res == NULL ||
+	if (box_process1(&request, &res) != 0 || res == NULL ||
 	    tuple_field_u64(res, 1, space_max_id) != 0)
 		return -1;
 	return 0;
diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
new file mode 100755
index 000000000..913bc0981
--- /dev/null
+++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
@@ -0,0 +1,74 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+
+local test = tap.test('gh-5231-box-execute-writes-in-ro')
+local expected_err = "Can't modify data because this instance is in read-only mode."
+
+box.cfg()
+box.execute("CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A))")
+local res, err = box.execute("INSERT INTO TEST (A, B) VALUES (3, 3)")
+box.cfg{read_only = true}
+
+test:plan(9)
+
+res, err = box.execute("INSERT INTO TEST (A, B) VALUES (1, 1)")
+test:is(
+    tostring(err),
+    expected_err,
+    "insert should fail in read-only mode"
+)
+res, err = box.execute("DELETE FROM TEST")
+test:is(
+    tostring(err),
+    expected_err,
+    "delete should fail in read-only mode"
+)
+res, err = box.execute("REPLACE INTO TEST VALUES (1, 2)")
+test:is(
+    tostring(err),
+    expected_err,
+    "replace should fail in read-only mode"
+)
+res, err = box.execute("UPDATE TEST SET B=4 WHERE A=3")
+test:is(
+    tostring(err),
+    expected_err,
+    "update should fail in read-only mode"
+)
+res, err = box.execute("TRUNCATE TABLE TEST")
+test:is(
+    tostring(err), 
+    expected_err,
+    "truncate should fail in read-only mode"
+)
+box.execute("CREATE TABLE TEST2 (A INT, PRIMARY KEY (A))")
+test:is(
+    tostring(err),
+    expected_err,
+    "create should fail in read-only mode"
+)
+box.execute("ALTER TABLE TEST ADD CONSTRAINT 'uk' UNIQUE (B)")
+test:is(
+    tostring(err),
+    expected_err,
+    "add constraint should fail in read-only mode"
+)
+box.execute("ALTER TABLE TEST RENAME TO TEST2")
+test:is(
+    tostring(err),
+    expected_err,
+    "rename should fail in read-only mode"
+)
+res, err = box.execute("DROP TABLE TEST")
+test:is(
+    tostring(err),
+    expected_err,
+    "drop table should fail in read-only mode"
+)
+
+-- cleanup
+box.cfg{read_only=false}
+res, err = box.execute("DROP TABLE TEST")
+
+os.exit(test:check() and 0 or 1)
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Nikita Pettik]

On 10 Nov 14:29, sergos@tarantool.org wrote:
> From: Sergey Ostanevich <sergos@tarantool.org>
> 
> +box.execute("ALTER TABLE TEST ADD CONSTRAINT 'uk' UNIQUE (B)")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "add constraint should fail in read-only mode"
> +)
> +box.execute("ALTER TABLE TEST RENAME TO TEST2")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "rename should fail in read-only mode"
> +)
> +res, err = box.execute("DROP TABLE TEST")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "drop table should fail in read-only mode"
> +)
> +
> +-- cleanup
> +box.cfg{read_only=false}
> +res, err = box.execute("DROP TABLE TEST")
> +
> +os.exit(test:check() and 0 or 1)

Not sure if we have to call os.exit() at the end of tap tests.
Otherwise LGTM

Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Mergen Imeev]

Hi! Thank you for the patch. See 5 comments below.

On Tue, Nov 10, 2020 at 02:29:13PM +0300, sergos@tarantool.org wrote:
> From: Sergey Ostanevich <sergos@tarantool.org>
> 
> A number of places in sql.c uses direct access to box_process_rw() that
> does not check read-only setting. Fixed by use of an intended interface
> of box_process1().
> 
> Closes #5231
> 
> Branch: https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro
> Issue: https://github.com/tarantool/tarantool/issues/5231
> 
1. As far as I know, these links should be located below '---'.

2. Shouldn't there be a ChangeLog here? After all it is a user-visible change.

> ---
>  src/box/sql.c                                 |  9 ++-
>  .../gh-5231-box-execute-writes-in-ro.test.lua | 74 +++++++++++++++++++
>  2 files changed, 79 insertions(+), 4 deletions(-)
>  create mode 100755 test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index a551bffc3..11f4b8ee4 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -54,6 +54,7 @@
>  #include "fk_constraint.h"
>  #include "mpstream/mpstream.h"
>  #include "sql_stmt_cache.h"
> +#include "backtrace.h"
3. What is this for?

>  
>  static sql *db = NULL;
>  
> @@ -453,7 +454,7 @@ insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,
>  	request.space_id = space->def->id;
>  	request.type = type;
>  	mp_tuple_assert(request.tuple, request.tuple_end);
> -	return box_process_rw(&request, space, NULL);
> +	return box_process1(&request, NULL);
>  }
>  
>  int tarantoolsqlInsert(struct space *space, const char *tuple,
> @@ -531,7 +532,7 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key,
>  	request.space_id = space->def->id;
>  	request.index_id = iid;
>  	assert(space_index(space, iid)->def->opts.is_unique);
> -	return box_process_rw(&request, space, &unused);
> +	return box_process1(&request, &unused);
>  }
>  
>  /*
> @@ -597,7 +598,7 @@ int tarantoolsqlClearTable(struct space *space, uint32_t *tuple_count)
>  		request.key = tuple_extract_key(tuple, pk->def->key_def,
>  						MULTIKEY_NONE, &key_size);
>  		request.key_end = request.key + key_size;
> -		rc = box_process_rw(&request, space, &unused);
> +		rc = box_process1(&request, &unused);
>  		if (rc != 0) {
>  			iterator_delete(iter);
>  			return -1;
> @@ -834,7 +835,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id)
>  	request.key_end = key + sizeof(key);
>  	request.type = IPROTO_UPDATE;
>  	request.space_id = space_schema->def->id;
> -	if (box_process_rw(&request, space_schema, &res) != 0 || res == NULL ||
> +	if (box_process1(&request, &res) != 0 || res == NULL ||
>  	    tuple_field_u64(res, 1, space_max_id) != 0)
>  		return -1;
>  	return 0;
> diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> new file mode 100755
> index 000000000..913bc0981
> --- /dev/null
> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> @@ -0,0 +1,74 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5231-box-execute-writes-in-ro')
> +local expected_err = "Can't modify data because this instance is in read-only mode."
> +
> +box.cfg()
> +box.execute("CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A))")
> +local res, err = box.execute("INSERT INTO TEST (A, B) VALUES (3, 3)")
> +box.cfg{read_only = true}
> +
> +test:plan(9)
> +
> +res, err = box.execute("INSERT INTO TEST (A, B) VALUES (1, 1)")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "insert should fail in read-only mode"
> +)
> +res, err = box.execute("DELETE FROM TEST")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "delete should fail in read-only mode"
> +)
> +res, err = box.execute("REPLACE INTO TEST VALUES (1, 2)")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "replace should fail in read-only mode"
> +)
> +res, err = box.execute("UPDATE TEST SET B=4 WHERE A=3")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "update should fail in read-only mode"
> +)
> +res, err = box.execute("TRUNCATE TABLE TEST")
> +test:is(
> +    tostring(err), 
4. Trailing whitespace.

> +    expected_err,
> +    "truncate should fail in read-only mode"
> +)
> +box.execute("CREATE TABLE TEST2 (A INT, PRIMARY KEY (A))")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "create should fail in read-only mode"
> +)
> +box.execute("ALTER TABLE TEST ADD CONSTRAINT 'uk' UNIQUE (B)")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "add constraint should fail in read-only mode"
> +)
> +box.execute("ALTER TABLE TEST RENAME TO TEST2")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "rename should fail in read-only mode"
> +)
> +res, err = box.execute("DROP TABLE TEST")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "drop table should fail in read-only mode"
> +)
> +
> +-- cleanup
> +box.cfg{read_only=false}
> +res, err = box.execute("DROP TABLE TEST")
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.24.3 (Apple Git-128)
> 
5. About this test - almost all sql-tap tests use specialized functions. Is
there a reason we shouldn't be using them here? I propose to rewrite this test
using these functions.

Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Sergey Ostanevich]

Hi Mergen!

Thank you for the review!
Updated patch attached, branch is updated.

Regards,
Sergos


> On 16 Nov 2020, at 13:31, Mergen Imeev <imeevma@tarantool.org> wrote:
> 
> Hi! Thank you for the patch. See 5 comments below.
> 
> On Tue, Nov 10, 2020 at 02:29:13PM +0300, sergos@tarantool.org wrote:
>> From: Sergey Ostanevich <sergos@tarantool.org>
>> 
>> A number of places in sql.c uses direct access to box_process_rw() that
>> does not check read-only setting. Fixed by use of an intended interface
>> of box_process1().
>> 
>> Closes #5231
>> 
>> Branch: https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro
>> Issue: https://github.com/tarantool/tarantool/issues/5231
>> 
> 1. As far as I know, these links should be located below '—'.
> 
Fixed

> 2. Shouldn't there be a ChangeLog here? After all it is a user-visible change.
> 
Added

>> ---
>> src/box/sql.c                                 |  9 ++-
>> .../gh-5231-box-execute-writes-in-ro.test.lua | 74 +++++++++++++++++++
>> 2 files changed, 79 insertions(+), 4 deletions(-)
>> create mode 100755 test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
>> 
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index a551bffc3..11f4b8ee4 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -54,6 +54,7 @@
>> #include "fk_constraint.h"
>> #include "mpstream/mpstream.h"
>> #include "sql_stmt_cache.h"
>> +#include "backtrace.h"
> 3. What is this for?
> 
Used for debugging, removed.

>> 
>> static sql *db = NULL;
>> 
>> @@ -453,7 +454,7 @@ insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,
>> 	request.space_id = space->def->id;
>> 	request.type = type;
>> 	mp_tuple_assert(request.tuple, request.tuple_end);
>> -	return box_process_rw(&request, space, NULL);
>> +	return box_process1(&request, NULL);
>> }
>> 
>> int tarantoolsqlInsert(struct space *space, const char *tuple,
>> @@ -531,7 +532,7 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key,
>> 	request.space_id = space->def->id;
>> 	request.index_id = iid;
>> 	assert(space_index(space, iid)->def->opts.is_unique);
>> -	return box_process_rw(&request, space, &unused);
>> +	return box_process1(&request, &unused);
>> }
>> 
>> /*
>> @@ -597,7 +598,7 @@ int tarantoolsqlClearTable(struct space *space, uint32_t *tuple_count)
>> 		request.key = tuple_extract_key(tuple, pk->def->key_def,
>> 						MULTIKEY_NONE, &key_size);
>> 		request.key_end = request.key + key_size;
>> -		rc = box_process_rw(&request, space, &unused);
>> +		rc = box_process1(&request, &unused);
>> 		if (rc != 0) {
>> 			iterator_delete(iter);
>> 			return -1;
>> @@ -834,7 +835,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id)
>> 	request.key_end = key + sizeof(key);
>> 	request.type = IPROTO_UPDATE;
>> 	request.space_id = space_schema->def->id;
>> -	if (box_process_rw(&request, space_schema, &res) != 0 || res == NULL ||
>> +	if (box_process1(&request, &res) != 0 || res == NULL ||
>> 	    tuple_field_u64(res, 1, space_max_id) != 0)
>> 		return -1;
>> 	return 0;
>> diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
>> new file mode 100755
>> index 000000000..913bc0981
>> --- /dev/null
>> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
>> @@ -0,0 +1,74 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local tap = require('tap')
>> +
>> +local test = tap.test('gh-5231-box-execute-writes-in-ro')
>> +local expected_err = "Can't modify data because this instance is in read-only mode."
>> +
>> +box.cfg()
>> +box.execute("CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A))")
>> +local res, err = box.execute("INSERT INTO TEST (A, B) VALUES (3, 3)")
>> +box.cfg{read_only = true}
>> +
>> +test:plan(9)
>> +
>> +res, err = box.execute("INSERT INTO TEST (A, B) VALUES (1, 1)")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "insert should fail in read-only mode"
>> +)
>> +res, err = box.execute("DELETE FROM TEST")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "delete should fail in read-only mode"
>> +)
>> +res, err = box.execute("REPLACE INTO TEST VALUES (1, 2)")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "replace should fail in read-only mode"
>> +)
>> +res, err = box.execute("UPDATE TEST SET B=4 WHERE A=3")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "update should fail in read-only mode"
>> +)
>> +res, err = box.execute("TRUNCATE TABLE TEST")
>> +test:is(
>> +    tostring(err), 
> 4. Trailing whitespace.
> 
Done with test rewrite.

>> +    expected_err,
>> +    "truncate should fail in read-only mode"
>> +)
>> +box.execute("CREATE TABLE TEST2 (A INT, PRIMARY KEY (A))")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "create should fail in read-only mode"
>> +)
>> +box.execute("ALTER TABLE TEST ADD CONSTRAINT 'uk' UNIQUE (B)")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "add constraint should fail in read-only mode"
>> +)
>> +box.execute("ALTER TABLE TEST RENAME TO TEST2")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "rename should fail in read-only mode"
>> +)
>> +res, err = box.execute("DROP TABLE TEST")
>> +test:is(
>> +    tostring(err),
>> +    expected_err,
>> +    "drop table should fail in read-only mode"
>> +)
>> +
>> +-- cleanup
>> +box.cfg{read_only=false}
>> +res, err = box.execute("DROP TABLE TEST")
>> +
>> +os.exit(test:check() and 0 or 1)
>> -- 
>> 2.24.3 (Apple Git-128)
>> 
> 5. About this test - almost all sql-tap tests use specialized functions. Is
> there a reason we shouldn't be using them here? I propose to rewrite this test
> using these functions.
Ditto


===============

A number of places in sql.c uses direct access to box_process_rw() that
does not check read-only setting. Fixed by use of an intended interface
of box_process1().

Closes #5231
---

Branch: https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro
Issue: https://github.com/tarantool/tarantool/issues/5231

@ChangeLog
- Data changes are blocked for SQL if node is read-only (gh-5231).

 src/box/sql.c                                 |   8 +-
 .../gh-5231-box-execute-writes-in-ro.test.lua | 105 ++++++++++++++++++
 2 files changed, 109 insertions(+), 4 deletions(-)
 create mode 100755 test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index a551bffc3..c55d0f4e5 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -453,7 +453,7 @@ insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,
 	request.space_id = space->def->id;
 	request.type = type;
 	mp_tuple_assert(request.tuple, request.tuple_end);
-	return box_process_rw(&request, space, NULL);
+	return box_process1(&request, NULL);
 }
 
 int tarantoolsqlInsert(struct space *space, const char *tuple,
@@ -531,7 +531,7 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key,
 	request.space_id = space->def->id;
 	request.index_id = iid;
 	assert(space_index(space, iid)->def->opts.is_unique);
-	return box_process_rw(&request, space, &unused);
+	return box_process1(&request, &unused);
 }
 
 /*
@@ -597,7 +597,7 @@ int tarantoolsqlClearTable(struct space *space, uint32_t *tuple_count)
 		request.key = tuple_extract_key(tuple, pk->def->key_def,
 						MULTIKEY_NONE, &key_size);
 		request.key_end = request.key + key_size;
-		rc = box_process_rw(&request, space, &unused);
+		rc = box_process1(&request, &unused);
 		if (rc != 0) {
 			iterator_delete(iter);
 			return -1;
@@ -834,7 +834,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id)
 	request.key_end = key + sizeof(key);
 	request.type = IPROTO_UPDATE;
 	request.space_id = space_schema->def->id;
-	if (box_process_rw(&request, space_schema, &res) != 0 || res == NULL ||
+	if (box_process1(&request, &res) != 0 || res == NULL ||
 	    tuple_field_u64(res, 1, space_max_id) != 0)
 		return -1;
 	return 0;
diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
new file mode 100755
index 000000000..6d95adc2e
--- /dev/null
+++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
@@ -0,0 +1,105 @@
+#!/usr/bin/env tarantool
+
+test = require("sqltester")
+test:plan(9)
+
+local expected_err = "Can't modify data because this instance is in read-only mode."
+
+test:execsql([[
+    CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A));
+    INSERT INTO TEST (A, B) VALUES (3, 3);
+]])
+
+box.cfg{read_only = true}
+
+test:do_catchsql_test(
+    "sql-errors-1.1",
+    [[
+        INSERT INTO TEST (A, B) VALUES (1, 1);
+    ]], {
+        -- <sql-errors-1.1>
+        1, expected_err
+        -- </sql-errors-1.1>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.2",
+    [[
+        DELETE FROM TEST;
+    ]], {
+        -- <sql-errors-1.2>
+        1, expected_err
+        -- </sql-errors-1.2>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.3",
+    [[
+        REPLACE INTO TEST VALUES (1, 2);
+    ]], {
+        -- <sql-errors-1.3>
+        1, expected_err
+        -- </sql-errors-1.3>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.4",
+    [[
+        UPDATE TEST SET B=4 WHERE A=3;
+    ]], {
+        -- <sql-errors-1.4>
+        1, expected_err
+        -- </sql-errors-1.4>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.5",
+    [[
+        TRUNCATE TABLE TEST;
+    ]], {
+        -- <sql-errors-1.5>
+        1, expected_err
+        -- </sql-errors-1.5>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.6",
+    [[
+        CREATE TABLE TEST2 (A INT, PRIMARY KEY (A));
+    ]], {
+        -- <sql-errors-1.6>
+        1, expected_err
+        -- </sql-errors-1.6>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.7",
+    [[
+        ALTER TABLE TEST ADD CONSTRAINT UK_CON UNIQUE (B);
+    ]], {
+        -- <sql-errors-1.7>
+        1, expected_err
+        -- </sql-errors-1.7>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.8",
+    [[
+        ALTER TABLE TEST RENAME TO TEST2;
+    ]], {
+        -- <sql-errors-1.8>
+        1, expected_err
+        -- </sql-errors-1.8>
+    })
+
+test:do_catchsql_test(
+    "sql-errors-1.9",
+    [[
+        DROP TABLE TEST;
+    ]], {
+        -- <sql-errors-1.9>
+        1, expected_err
+        -- </sql-errors-1.9>
+    })
+
+test:finish_test()
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Nikita Pettik]

On 16 Nov 23:36, Sergey Ostanevich wrote:
> diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> new file mode 100755
> index 000000000..6d95adc2e
> --- /dev/null
> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> @@ -0,0 +1,105 @@
> +#!/usr/bin/env tarantool
> +
> +test = require("sqltester")
> +test:plan(9)
> +
> +local expected_err = "Can't modify data because this instance is in read-only mode."
> +
> +test:execsql([[
> +    CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A));
> +    INSERT INTO TEST (A, B) VALUES (3, 3);
> +]])
> +
> +box.cfg{read_only = true}
> +
> +test:do_catchsql_test(
> +    "sql-errors-1.1",
> +    [[
> +        INSERT INTO TEST (A, B) VALUES (1, 1);
> +    ]], {
> +        -- <sql-errors-1.1>

These comments are artifacts from porting SQLite test suite with
auto-tools, they are not really needed in new tests :)

Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Sergey Ostanevich]

Hi!

Thanks for review! Updated, force-pushed, new patch attached.

Sergos

> On 17 Nov 2020, at 00:09, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> On 16 Nov 23:36, Sergey Ostanevich wrote:
>> diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
>> new file mode 100755
>> index 000000000..6d95adc2e
>> --- /dev/null
>> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
>> @@ -0,0 +1,105 @@
>> +#!/usr/bin/env tarantool
>> +
>> +test = require("sqltester")
>> +test:plan(9)
>> +
>> +local expected_err = "Can't modify data because this instance is in read-only mode."
>> +
>> +test:execsql([[
>> +    CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A));
>> +    INSERT INTO TEST (A, B) VALUES (3, 3);
>> +]])
>> +
>> +box.cfg{read_only = true}
>> +
>> +test:do_catchsql_test(
>> +    "sql-errors-1.1",
>> +    [[
>> +        INSERT INTO TEST (A, B) VALUES (1, 1);
>> +    ]], {
>> +        -- <sql-errors-1.1>
> 
> These comments are artifacts from porting SQLite test suite with
> auto-tools, they are not really needed in new tests :)
> 
=============

From 4804ac485b38ddd280735970f6b9422bad1b30b6 Mon Sep 17 00:00:00 2001
From: Sergey Ostanevich <sergos@tarantool.org>
Date: Tue, 10 Nov 2020 13:52:10 +0300
Subject: [PATCH] sql: forbid data changes in read-only mode

A number of places in sql.c uses direct access to box_process_rw() that
does not check read-only setting. Fixed by use of an intended interface
of box_process1().

Closes #5231
---

Branch: https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro
Issue: https://github.com/tarantool/tarantool/issues/5231

@ChangeLog
- Data changes are blocked for SQL if node is read-only (gh-5231).

 src/box/sql.c                                 |  8 +-
 .../gh-5231-box-execute-writes-in-ro.test.lua | 87 +++++++++++++++++++
 2 files changed, 91 insertions(+), 4 deletions(-)
 create mode 100755 test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index a551bffc3..c55d0f4e5 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -453,7 +453,7 @@ insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,
 	request.space_id = space->def->id;
 	request.type = type;
 	mp_tuple_assert(request.tuple, request.tuple_end);
-	return box_process_rw(&request, space, NULL);
+	return box_process1(&request, NULL);
 }
 
 int tarantoolsqlInsert(struct space *space, const char *tuple,
@@ -531,7 +531,7 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key,
 	request.space_id = space->def->id;
 	request.index_id = iid;
 	assert(space_index(space, iid)->def->opts.is_unique);
-	return box_process_rw(&request, space, &unused);
+	return box_process1(&request, &unused);
 }
 
 /*
@@ -597,7 +597,7 @@ int tarantoolsqlClearTable(struct space *space, uint32_t *tuple_count)
 		request.key = tuple_extract_key(tuple, pk->def->key_def,
 						MULTIKEY_NONE, &key_size);
 		request.key_end = request.key + key_size;
-		rc = box_process_rw(&request, space, &unused);
+		rc = box_process1(&request, &unused);
 		if (rc != 0) {
 			iterator_delete(iter);
 			return -1;
@@ -834,7 +834,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id)
 	request.key_end = key + sizeof(key);
 	request.type = IPROTO_UPDATE;
 	request.space_id = space_schema->def->id;
-	if (box_process_rw(&request, space_schema, &res) != 0 || res == NULL ||
+	if (box_process1(&request, &res) != 0 || res == NULL ||
 	    tuple_field_u64(res, 1, space_max_id) != 0)
 		return -1;
 	return 0;
diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
new file mode 100755
index 000000000..05c23c2f8
--- /dev/null
+++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
@@ -0,0 +1,87 @@
+#!/usr/bin/env tarantool
+
+test = require("sqltester")
+test:plan(9)
+
+local expected_err = "Can't modify data because this instance is in read-only mode."
+
+test:execsql([[
+    CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A));
+    INSERT INTO TEST (A, B) VALUES (3, 3);
+]])
+
+box.cfg{read_only = true}
+
+test:do_catchsql_test(
+    "gh-5231-1",
+    [[
+        INSERT INTO TEST (A, B) VALUES (1, 1);
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-2",
+    [[
+        DELETE FROM TEST;
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-3",
+    [[
+        REPLACE INTO TEST VALUES (1, 2);
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-4",
+    [[
+        UPDATE TEST SET B=4 WHERE A=3;
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-5",
+    [[
+        TRUNCATE TABLE TEST;
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-6",
+    [[
+        CREATE TABLE TEST2 (A INT, PRIMARY KEY (A));
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-7",
+    [[
+        ALTER TABLE TEST ADD CONSTRAINT UK_CON UNIQUE (B);
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-8",
+    [[
+        ALTER TABLE TEST RENAME TO TEST2;
+    ]], {
+        1, expected_err
+    })
+
+test:do_catchsql_test(
+    "gh-5231-9",
+    [[
+        DROP TABLE TEST;
+    ]], {
+        1, expected_err
+    })
+
+test:finish_test()
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Mergen Imeev]

Hi! Thank you for the fixes. I have one unimportant comment: next time please
include link to branch
https://github.com/tarantool/tarantool/tree/sergos/gh-5231-box-execute-writes-in-ro
instead of link to compare
https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro

LGTM.


On Thu, Nov 19, 2020 at 10:41:23AM +0300, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for review! Updated, force-pushed, new patch attached.
> 
> Sergos
> 
> > On 17 Nov 2020, at 00:09, Nikita Pettik <korablev@tarantool.org> wrote:
> > 
> > On 16 Nov 23:36, Sergey Ostanevich wrote:
> >> diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> >> new file mode 100755
> >> index 000000000..6d95adc2e
> >> --- /dev/null
> >> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> >> @@ -0,0 +1,105 @@
> >> +#!/usr/bin/env tarantool
> >> +
> >> +test = require("sqltester")
> >> +test:plan(9)
> >> +
> >> +local expected_err = "Can't modify data because this instance is in read-only mode."
> >> +
> >> +test:execsql([[
> >> +    CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A));
> >> +    INSERT INTO TEST (A, B) VALUES (3, 3);
> >> +]])
> >> +
> >> +box.cfg{read_only = true}
> >> +
> >> +test:do_catchsql_test(
> >> +    "sql-errors-1.1",
> >> +    [[
> >> +        INSERT INTO TEST (A, B) VALUES (1, 1);
> >> +    ]], {
> >> +        -- <sql-errors-1.1>
> > 
> > These comments are artifacts from porting SQLite test suite with
> > auto-tools, they are not really needed in new tests :)
> > 
> =============
> 
> From 4804ac485b38ddd280735970f6b9422bad1b30b6 Mon Sep 17 00:00:00 2001
> From: Sergey Ostanevich <sergos@tarantool.org>
> Date: Tue, 10 Nov 2020 13:52:10 +0300
> Subject: [PATCH] sql: forbid data changes in read-only mode
> 
> A number of places in sql.c uses direct access to box_process_rw() that
> does not check read-only setting. Fixed by use of an intended interface
> of box_process1().
> 
> Closes #5231
> ---
> 
> Branch: https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro
> Issue: https://github.com/tarantool/tarantool/issues/5231
> 
> @ChangeLog
> - Data changes are blocked for SQL if node is read-only (gh-5231).
> 
>  src/box/sql.c                                 |  8 +-
>  .../gh-5231-box-execute-writes-in-ro.test.lua | 87 +++++++++++++++++++
>  2 files changed, 91 insertions(+), 4 deletions(-)
>  create mode 100755 test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index a551bffc3..c55d0f4e5 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -453,7 +453,7 @@ insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,
>  	request.space_id = space->def->id;
>  	request.type = type;
>  	mp_tuple_assert(request.tuple, request.tuple_end);
> -	return box_process_rw(&request, space, NULL);
> +	return box_process1(&request, NULL);
>  }
>  
>  int tarantoolsqlInsert(struct space *space, const char *tuple,
> @@ -531,7 +531,7 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key,
>  	request.space_id = space->def->id;
>  	request.index_id = iid;
>  	assert(space_index(space, iid)->def->opts.is_unique);
> -	return box_process_rw(&request, space, &unused);
> +	return box_process1(&request, &unused);
>  }
>  
>  /*
> @@ -597,7 +597,7 @@ int tarantoolsqlClearTable(struct space *space, uint32_t *tuple_count)
>  		request.key = tuple_extract_key(tuple, pk->def->key_def,
>  						MULTIKEY_NONE, &key_size);
>  		request.key_end = request.key + key_size;
> -		rc = box_process_rw(&request, space, &unused);
> +		rc = box_process1(&request, &unused);
>  		if (rc != 0) {
>  			iterator_delete(iter);
>  			return -1;
> @@ -834,7 +834,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id)
>  	request.key_end = key + sizeof(key);
>  	request.type = IPROTO_UPDATE;
>  	request.space_id = space_schema->def->id;
> -	if (box_process_rw(&request, space_schema, &res) != 0 || res == NULL ||
> +	if (box_process1(&request, &res) != 0 || res == NULL ||
>  	    tuple_field_u64(res, 1, space_max_id) != 0)
>  		return -1;
>  	return 0;
> diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> new file mode 100755
> index 000000000..05c23c2f8
> --- /dev/null
> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> @@ -0,0 +1,87 @@
> +#!/usr/bin/env tarantool
> +
> +test = require("sqltester")
> +test:plan(9)
> +
> +local expected_err = "Can't modify data because this instance is in read-only mode."
> +
> +test:execsql([[
> +    CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A));
> +    INSERT INTO TEST (A, B) VALUES (3, 3);
> +]])
> +
> +box.cfg{read_only = true}
> +
> +test:do_catchsql_test(
> +    "gh-5231-1",
> +    [[
> +        INSERT INTO TEST (A, B) VALUES (1, 1);
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-2",
> +    [[
> +        DELETE FROM TEST;
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-3",
> +    [[
> +        REPLACE INTO TEST VALUES (1, 2);
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-4",
> +    [[
> +        UPDATE TEST SET B=4 WHERE A=3;
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-5",
> +    [[
> +        TRUNCATE TABLE TEST;
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-6",
> +    [[
> +        CREATE TABLE TEST2 (A INT, PRIMARY KEY (A));
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-7",
> +    [[
> +        ALTER TABLE TEST ADD CONSTRAINT UK_CON UNIQUE (B);
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-8",
> +    [[
> +        ALTER TABLE TEST RENAME TO TEST2;
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-5231-9",
> +    [[
> +        DROP TABLE TEST;
> +    ]], {
> +        1, expected_err
> +    })
> +
> +test:finish_test()
> -- 
> 2.24.3 (Apple Git-128)
> 
> 

Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Nikita Pettik]

On 20 Nov 16:47, Mergen Imeev wrote:
> Hi! Thank you for the fixes. I have one unimportant comment: next time please
> include link to branch
> https://github.com/tarantool/tarantool/tree/sergos/gh-5231-box-execute-writes-in-ro
> instead of link to compare
> https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro
> 
> LGTM.

Pushed to master, 2.6 and 2.5. Changelogs are updated correspondingly.
Branch is dropped.