From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 E4155469719 for ; Mon, 16 Nov 2020 13:31:31 +0300 (MSK) Date: Mon, 16 Nov 2020 13:31:29 +0300 From: Mergen Imeev Message-ID: <20201116103129.GA324907@tarantool.org> References: <20201110112913.28083-1-sergos@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201110112913.28083-1-sergos@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: sergos@tarantool.org Cc: tarantool-patches@dev.tarantool.org 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 > > 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.