From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8B32E469719 for ; Mon, 16 Nov 2020 23:36:19 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) From: Sergey Ostanevich In-Reply-To: <20201116103129.GA324907@tarantool.org> Date: Mon, 16 Nov 2020 23:36:17 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <05C5398B-E522-4855-AA34-FB9B41606098@tarantool.org> References: <20201110112913.28083-1-sergos@tarantool.org> <20201116103129.GA324907@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Hi Mergen! Thank you for the review! Updated patch attached, branch is updated. Regards, Sergos > On 16 Nov 2020, at 13:31, Mergen Imeev wrote: >=20 > Hi! Thank you for the patch. See 5 comments below. >=20 > On Tue, Nov 10, 2020 at 02:29:13PM +0300, sergos@tarantool.org wrote: >> From: Sergey Ostanevich >>=20 >> 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(). >>=20 >> Closes #5231 >>=20 >> Branch: = https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-= writes-in-ro >> Issue: https://github.com/tarantool/tarantool/issues/5231 >>=20 > 1. As far as I know, these links should be located below '=E2=80=94'. >=20 Fixed > 2. Shouldn't there be a ChangeLog here? After all it is a user-visible = change. >=20 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 >>=20 >> 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? >=20 Used for debugging, removed. >>=20 >> static sql *db =3D NULL; >>=20 >> @@ -453,7 +454,7 @@ insertOrReplace(struct space *space, const char = *tuple, const char *tuple_end, >> request.space_id =3D space->def->id; >> request.type =3D type; >> mp_tuple_assert(request.tuple, request.tuple_end); >> - return box_process_rw(&request, space, NULL); >> + return box_process1(&request, NULL); >> } >>=20 >> 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 =3D space->def->id; >> request.index_id =3D iid; >> assert(space_index(space, iid)->def->opts.is_unique); >> - return box_process_rw(&request, space, &unused); >> + return box_process1(&request, &unused); >> } >>=20 >> /* >> @@ -597,7 +598,7 @@ int tarantoolsqlClearTable(struct space *space, = uint32_t *tuple_count) >> request.key =3D tuple_extract_key(tuple, = pk->def->key_def, >> MULTIKEY_NONE, = &key_size); >> request.key_end =3D request.key + key_size; >> - rc =3D box_process_rw(&request, space, &unused); >> + rc =3D box_process1(&request, &unused); >> if (rc !=3D 0) { >> iterator_delete(iter); >> return -1; >> @@ -834,7 +835,7 @@ tarantoolsqlIncrementMaxid(uint64_t = *space_max_id) >> request.key_end =3D key + sizeof(key); >> request.type =3D IPROTO_UPDATE; >> request.space_id =3D space_schema->def->id; >> - if (box_process_rw(&request, space_schema, &res) !=3D 0 || res = =3D=3D NULL || >> + if (box_process1(&request, &res) !=3D 0 || res =3D=3D NULL || >> tuple_field_u64(res, 1, space_max_id) !=3D 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 =3D require('tap') >> + >> +local test =3D tap.test('gh-5231-box-execute-writes-in-ro') >> +local expected_err =3D "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 =3D box.execute("INSERT INTO TEST (A, B) VALUES (3, = 3)") >> +box.cfg{read_only =3D true} >> + >> +test:plan(9) >> + >> +res, err =3D 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 =3D box.execute("DELETE FROM TEST") >> +test:is( >> + tostring(err), >> + expected_err, >> + "delete should fail in read-only mode" >> +) >> +res, err =3D box.execute("REPLACE INTO TEST VALUES (1, 2)") >> +test:is( >> + tostring(err), >> + expected_err, >> + "replace should fail in read-only mode" >> +) >> +res, err =3D box.execute("UPDATE TEST SET B=3D4 WHERE A=3D3") >> +test:is( >> + tostring(err), >> + expected_err, >> + "update should fail in read-only mode" >> +) >> +res, err =3D box.execute("TRUNCATE TABLE TEST") >> +test:is( >> + tostring(err),=20 > 4. Trailing whitespace. >=20 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 =3D box.execute("DROP TABLE TEST") >> +test:is( >> + tostring(err), >> + expected_err, >> + "drop table should fail in read-only mode" >> +) >> + >> +-- cleanup >> +box.cfg{read_only=3Dfalse} >> +res, err =3D box.execute("DROP TABLE TEST") >> + >> +os.exit(test:check() and 0 or 1) >> --=20 >> 2.24.3 (Apple Git-128) >>=20 > 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D 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 =3D space->def->id; request.type =3D type; mp_tuple_assert(request.tuple, request.tuple_end); - return box_process_rw(&request, space, NULL); + return box_process1(&request, NULL); } =20 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 =3D space->def->id; request.index_id =3D iid; assert(space_index(space, iid)->def->opts.is_unique); - return box_process_rw(&request, space, &unused); + return box_process1(&request, &unused); } =20 /* @@ -597,7 +597,7 @@ int tarantoolsqlClearTable(struct space *space, = uint32_t *tuple_count) request.key =3D tuple_extract_key(tuple, = pk->def->key_def, MULTIKEY_NONE, = &key_size); request.key_end =3D request.key + key_size; - rc =3D box_process_rw(&request, space, &unused); + rc =3D box_process1(&request, &unused); if (rc !=3D 0) { iterator_delete(iter); return -1; @@ -834,7 +834,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id) request.key_end =3D key + sizeof(key); request.type =3D IPROTO_UPDATE; request.space_id =3D space_schema->def->id; - if (box_process_rw(&request, space_schema, &res) !=3D 0 || res = =3D=3D NULL || + if (box_process1(&request, &res) !=3D 0 || res =3D=3D NULL || tuple_field_u64(res, 1, space_max_id) !=3D 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 =3D require("sqltester") +test:plan(9) + +local expected_err =3D "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 =3D true} + +test:do_catchsql_test( + "sql-errors-1.1", + [[ + INSERT INTO TEST (A, B) VALUES (1, 1); + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.2", + [[ + DELETE FROM TEST; + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.3", + [[ + REPLACE INTO TEST VALUES (1, 2); + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.4", + [[ + UPDATE TEST SET B=3D4 WHERE A=3D3; + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.5", + [[ + TRUNCATE TABLE TEST; + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.6", + [[ + CREATE TABLE TEST2 (A INT, PRIMARY KEY (A)); + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.7", + [[ + ALTER TABLE TEST ADD CONSTRAINT UK_CON UNIQUE (B); + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.8", + [[ + ALTER TABLE TEST RENAME TO TEST2; + ]], { + -- + 1, expected_err + -- + }) + +test:do_catchsql_test( + "sql-errors-1.9", + [[ + DROP TABLE TEST; + ]], { + -- + 1, expected_err + -- + }) + +test:finish_test() --=20 2.24.3 (Apple Git-128)