Re: [Tarantool-patches] [PATCH 1/1] txn: don't trust group id in remote request header

[Georgy Kirichenko <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 7045 bytes --]

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')


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]