* [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode @ 2020-11-10 11:29 sergos 2020-11-13 14:01 ` Nikita Pettik 2020-11-16 10:31 ` Mergen Imeev 0 siblings, 2 replies; 8+ messages in thread From: sergos @ 2020-11-10 11:29 UTC (permalink / raw) To: tarantool-patches, imeevma 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode 2020-11-10 11:29 [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode sergos @ 2020-11-13 14:01 ` Nikita Pettik 2020-11-16 10:31 ` Mergen Imeev 1 sibling, 0 replies; 8+ messages in thread From: Nikita Pettik @ 2020-11-13 14:01 UTC (permalink / raw) To: sergos; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode 2020-11-10 11:29 [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode sergos 2020-11-13 14:01 ` Nikita Pettik @ 2020-11-16 10:31 ` Mergen Imeev 2020-11-16 20:36 ` Sergey Ostanevich 1 sibling, 1 reply; 8+ messages in thread From: Mergen Imeev @ 2020-11-16 10:31 UTC (permalink / raw) To: sergos; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode 2020-11-16 10:31 ` Mergen Imeev @ 2020-11-16 20:36 ` Sergey Ostanevich 2020-11-16 21:09 ` Nikita Pettik 0 siblings, 1 reply; 8+ messages in thread From: Sergey Ostanevich @ 2020-11-16 20:36 UTC (permalink / raw) To: Mergen Imeev; +Cc: tarantool-patches 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode 2020-11-16 20:36 ` Sergey Ostanevich @ 2020-11-16 21:09 ` Nikita Pettik 2020-11-19 7:41 ` Sergey Ostanevich 0 siblings, 1 reply; 8+ messages in thread From: Nikita Pettik @ 2020-11-16 21:09 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches 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 :) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode 2020-11-16 21:09 ` Nikita Pettik @ 2020-11-19 7:41 ` Sergey Ostanevich 2020-11-20 13:47 ` Mergen Imeev 0 siblings, 1 reply; 8+ messages in thread From: Sergey Ostanevich @ 2020-11-19 7:41 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode 2020-11-19 7:41 ` Sergey Ostanevich @ 2020-11-20 13:47 ` Mergen Imeev 2020-11-22 14:55 ` Nikita Pettik 0 siblings, 1 reply; 8+ messages in thread From: Mergen Imeev @ 2020-11-20 13:47 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: tarantool-patches 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) > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode 2020-11-20 13:47 ` Mergen Imeev @ 2020-11-22 14:55 ` Nikita Pettik 0 siblings, 0 replies; 8+ messages in thread From: Nikita Pettik @ 2020-11-22 14:55 UTC (permalink / raw) To: Mergen Imeev; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-22 14:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-10 11:29 [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode sergos 2020-11-13 14:01 ` Nikita Pettik 2020-11-16 10:31 ` Mergen Imeev 2020-11-16 20:36 ` Sergey Ostanevich 2020-11-16 21:09 ` Nikita Pettik 2020-11-19 7:41 ` Sergey Ostanevich 2020-11-20 13:47 ` Mergen Imeev 2020-11-22 14:55 ` Nikita Pettik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox