[Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode
Sergey Ostanevich
sergos at tarantool.org
Mon Nov 16 23:36:17 MSK 2020
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 at tarantool.org> wrote:
>
> Hi! Thank you for the patch. See 5 comments below.
>
> On Tue, Nov 10, 2020 at 02:29:13PM +0300, sergos at tarantool.org wrote:
>> From: Sergey Ostanevich <sergos at 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)
More information about the Tarantool-patches
mailing list