From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 361BB46970E for ; Thu, 16 Jan 2020 22:40:45 +0300 (MSK) From: Georgy Kirichenko Date: Thu, 16 Jan 2020 22:40:44 +0300 Message-ID: <2518534.mvXUDI8C0e@localhost> In-Reply-To: References: MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart11451381.O9o76ZdvQC"; micalg="pgp-sha256"; protocol="application/pgp-signature" Subject: Re: [Tarantool-patches] [PATCH 1/1] txn: don't trust group id in remote request header List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org, kostja.osipov@gmail.com Cc: Vladislav Shpilevoy --nextPart11451381.O9o76ZdvQC Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" LGTM On Thursday, 16 January 2020 00:24:53 MSK Vladislav Shpilevoy wrote: > Transaction adds a redo log for each statement. The log is an xrow > header. Some requests don't have a header (local requests), some > do (from remote client, from replication). > > When a request had a header, it was written as is to WAL. But > requests from remote client have an xrow header, however barely > filled. Most of its fields are default values, usually 0. > Including group id. Indeed, remote clients should not care about > setting such deep system fields. > > That led to a problem when a space had group id local (!= 0), but > it was ignored because in a request header from a remote client > the group id was default (== 0). On the summary, it was possible > to force Tarantool to replicate a replica-local space. > > Now group id setting is server-authoritative. Box always sets it > regardless of what is present in an xrow header received from a > client. > > Thanks Kostja Osipov (@kostja) for the diagnostics and the > solution. > > Closes #4729 > --- > Branch: > https://github.com/tarantool/tarantool/tree/gerold103/gh-4729-iproto-group-> id Issue: https://github.com/tarantool/tarantool/issues/4729 > > There is an alternative solution - nullify xrow_header pointer in > tx_process1() before box_process1(), and return it back > afterwards. Then txn.c won't look at the header. Currently there > still is a problem, if a user, for example, sets replica_id field. > > Indeed, xrow_header may create a problem in tx_process1() only, > and it is not really needed by tx thread. For iproto the only > interesting fields here are sync and type. > > src/box/txn.c | 11 +- > .../gh-4729-netbox-group-id.result | 104 ++++++++++++++++++ > .../gh-4729-netbox-group-id.test.lua | 37 +++++++ > 3 files changed, 150 insertions(+), 2 deletions(-) > create mode 100644 test/replication/gh-4729-netbox-group-id.result > create mode 100644 test/replication/gh-4729-netbox-group-id.test.lua > > diff --git a/src/box/txn.c b/src/box/txn.c > index 963ec8eeb..bedb57449 100644 > --- a/src/box/txn.c > +++ b/src/box/txn.c > @@ -72,12 +72,19 @@ txn_add_redo(struct txn *txn, struct txn_stmt *stmt, > struct request *request) /* Initialize members explicitly to save time on > memset() */ > row->type = request->type; > row->replica_id = 0; > - row->group_id = stmt->space != NULL ? > - space_group_id(stmt->space) : 0; > row->lsn = 0; > row->sync = 0; > row->tm = 0; > } > + /* > + * Group ID should be set both for requests not having a > + * header, and for the ones who have it. This is because > + * even if a request has a header, the group id could be > + * omitted in it, and is default - 0. Even if the space's > + * real group id is different. > + */ > + struct space *space = stmt->space; > + row->group_id = space != NULL ? space_group_id(space) : 0; > row->bodycnt = xrow_encode_dml(request, &txn->region, row->body); > if (row->bodycnt < 0) > return -1; > diff --git a/test/replication/gh-4729-netbox-group-id.result > b/test/replication/gh-4729-netbox-group-id.result new file mode 100644 > index 000000000..d7189baaf > --- /dev/null > +++ b/test/replication/gh-4729-netbox-group-id.result > @@ -0,0 +1,104 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > +-- > +-- gh-4729: box should not trust xrow header group id, received > +-- from a remote client on DDL/DML. > +-- > +box.schema.user.grant('guest', 'super') > + | --- > + | ... > +s1 = box.schema.space.create('test_local', {is_local = true}) > + | --- > + | ... > +_ = s1:create_index('pk', {parts = {1, 'unsigned'}}) > + | --- > + | ... > +s2 = box.schema.space.create('test_normal') > + | --- > + | ... > +_ = s2:create_index('pk', {parts = {1, 'unsigned'}}) > + | --- > + | ... > + > +test_run:cmd('create server replica with rpl_master=default, > script="replication/replica.lua"') + | --- > + | - true > + | ... > +test_run:cmd('start server replica') > + | --- > + | - true > + | ... > +test_run:switch('replica') > + | --- > + | - true > + | ... > + > +netbox = require('net.box') > + | --- > + | ... > +c = netbox.connect(box.cfg.replication[1]) > + | --- > + | ... > +c.space.test_local:insert({1}) > + | --- > + | - [1] > + | ... > +c.space.test_normal:insert({1}) > + | --- > + | - [1] > + | ... > +c:close() > + | --- > + | ... > +test_run:wait_cond(function() \ > + return box.space.test_normal ~= nil and \ > + box.space.test_normal.index.pk ~= nil and \ > + box.space.test_normal:count() == 1 \ > +end) > + | --- > + | - true > + | ... > +box.space.test_local:select{} > + | --- > + | - [] > + | ... > +box.space.test_normal:select{} > + | --- > + | - - [1] > + | ... > + > +test_run:switch('default') > + | --- > + | - true > + | ... > +test_run:cmd("stop server replica") > + | --- > + | - true > + | ... > +test_run:cmd("cleanup server replica") > + | --- > + | - true > + | ... > +test_run:cmd("delete server replica") > + | --- > + | - true > + | ... > +s1:select{} > + | --- > + | - - [1] > + | ... > +s2:select{} > + | --- > + | - - [1] > + | ... > +s1:drop() > + | --- > + | ... > +s2:drop() > + | --- > + | ... > +box.schema.user.revoke('guest', 'super') > + | --- > + | ... > diff --git a/test/replication/gh-4729-netbox-group-id.test.lua > b/test/replication/gh-4729-netbox-group-id.test.lua new file mode 100644 > index 000000000..3d8cef65f > --- /dev/null > +++ b/test/replication/gh-4729-netbox-group-id.test.lua > @@ -0,0 +1,37 @@ > +test_run = require('test_run').new() > +-- > +-- gh-4729: box should not trust xrow header group id, received > +-- from a remote client on DDL/DML. > +-- > +box.schema.user.grant('guest', 'super') > +s1 = box.schema.space.create('test_local', {is_local = true}) > +_ = s1:create_index('pk', {parts = {1, 'unsigned'}}) > +s2 = box.schema.space.create('test_normal') > +_ = s2:create_index('pk', {parts = {1, 'unsigned'}}) > + > +test_run:cmd('create server replica with rpl_master=default, > script="replication/replica.lua"') +test_run:cmd('start server replica') > +test_run:switch('replica') > + > +netbox = require('net.box') > +c = netbox.connect(box.cfg.replication[1]) > +c.space.test_local:insert({1}) > +c.space.test_normal:insert({1}) > +c:close() > +test_run:wait_cond(function() \ > + return box.space.test_normal ~= nil and \ > + box.space.test_normal.index.pk ~= nil and \ > + box.space.test_normal:count() == 1 \ > +end) > +box.space.test_local:select{} > +box.space.test_normal:select{} > + > +test_run:switch('default') > +test_run:cmd("stop server replica") > +test_run:cmd("cleanup server replica") > +test_run:cmd("delete server replica") > +s1:select{} > +s2:select{} > +s1:drop() > +s2:drop() > +box.schema.user.revoke('guest', 'super') --nextPart11451381.O9o76ZdvQC Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEECXG+Yw5ArYcP8x5wDVHWG5PoUw4FAl4gvDwACgkQDVHWG5Po Uw5IqggAhbiU9DI0Z8rExFwRYmr5iIQ68t0bEIoaEO6o06TIzrPpjzDvapZKvoKk W6jacEeOuK1qfawfeRkNEnXbO9rRMezL2gp7kOhYgwDc9LsQ530eJ00Ie6zwGtaQ mU6RfQwloFoRPBpNbdE+GGVPxXcgYxim39i+06AClnLkkW+8JelrdU9HfsarA401 W+KEOku01lh7myXucXvUbwGxM6zF+Ic4RdTF1wttXpEHQKo2l274eqcXJ/wa/RmQ H3UKXTk6giSwY1LOhVL/GJmWmxxmCWk5i0Cc8F9QKvOhsuFYwyILfXVNjFIZV886 MwTUYKtXBK0As3M2oybuQWWnwFOq4A== =KOio -----END PGP SIGNATURE----- --nextPart11451381.O9o76ZdvQC--