[Tarantool-patches] [PATCH] engine: validate key in space_before_replace

[Tarantool-patches] [PATCH] engine: validate key in space_before_replace

[Ilya Kosarev]

Since 9fba29abb4e05babb9b23b4413bd8083f0fba933 (memtx: introduce tuple
compare hint) index_get uses key_hint to get a comparison hint for the
key. It requires key type to be validated. However in
space_before_replace index_get was used without key validation on some
execution paths. Now it is fixed and validation is being performed if
needed. Corresponding test case is introduced.

Closes #5093
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5093-before_replace-key-validation
Issue: https://github.com/tarantool/tarantool/issues/5093

@ChangeLog:
 * Add needed key validation to space_before_replace.

 src/box/space.c                               |  6 +++---
 .../gh-5093-before_replace-key-type.result    | 21 +++++++++++++++++++
 .../gh-5093-before_replace-key-type.test.lua  |  7 +++++++
 3 files changed, 31 insertions(+), 3 deletions(-)
 create mode 100644 test/engine/gh-5093-before_replace-key-type.result
 create mode 100644 test/engine/gh-5093-before_replace-key-type.test.lua

diff --git a/src/box/space.c b/src/box/space.c
index eecbde7fa7..66768208b3 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -359,9 +359,6 @@ space_before_replace(struct space *space, struct txn *txn,
 			return -1;
 		key = request->key;
 		part_count = mp_decode_array(&key);
-		if (exact_key_validate(index->def->key_def,
-				       key, part_count) != 0)
-			return -1;
 		break;
 	case IPROTO_INSERT:
 	case IPROTO_REPLACE:
@@ -380,6 +377,9 @@ space_before_replace(struct space *space, struct txn *txn,
 		/* Unknown request type, nothing to do. */
 		return 0;
 	}
+	if (key != NULL &&
+	    exact_key_validate(index->def->key_def, key, part_count) != 0)
+		return -1;
 
 	struct tuple *old_tuple = NULL;
 	if (index != NULL &&
diff --git a/test/engine/gh-5093-before_replace-key-type.result b/test/engine/gh-5093-before_replace-key-type.result
new file mode 100644
index 0000000000..abf5b2c194
--- /dev/null
+++ b/test/engine/gh-5093-before_replace-key-type.result
@@ -0,0 +1,21 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+trigger = function() end
+ | ---
+ | ...
+
+s = box.schema.space.create('gh-5093', {engine=test_run:get_cfg('engine')})
+ | ---
+ | ...
+_ = s:create_index('value', {parts={{1, type='decimal'}}})
+ | ---
+ | ...
+_ = s:before_replace(trigger)
+ | ---
+ | ...
+s:replace{'1111.1111'}
+ | ---
+ | - error: 'Supplied key type of part 0 does not match index part type: expected decimal'
+ | ...
diff --git a/test/engine/gh-5093-before_replace-key-type.test.lua b/test/engine/gh-5093-before_replace-key-type.test.lua
new file mode 100644
index 0000000000..9cba3c1597
--- /dev/null
+++ b/test/engine/gh-5093-before_replace-key-type.test.lua
@@ -0,0 +1,7 @@
+test_run = require('test_run').new()
+trigger = function() end
+
+s = box.schema.space.create('gh-5093', {engine=test_run:get_cfg('engine')})
+_ = s:create_index('value', {parts={{1, type='decimal'}}})
+_ = s:before_replace(trigger)
+s:replace{'1111.1111'}
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 2 comments below.

On 03/07/2020 15:32, Ilya Kosarev wrote:
> Since 9fba29abb4e05babb9b23b4413bd8083f0fba933 (memtx: introduce tuple
> compare hint) index_get uses key_hint to get a comparison hint for the
> key. It requires key type to be validated. However in
> space_before_replace index_get was used without key validation on some
> execution paths. Now it is fixed and validation is being performed if
> needed. Corresponding test case is introduced.
> 
> Closes #5093
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5093-before_replace-key-validation
> Issue: https://github.com/tarantool/tarantool/issues/5093
> 
> @ChangeLog:
>  * Add needed key validation to space_before_replace.
> 
>  src/box/space.c                               |  6 +++---
>  .../gh-5093-before_replace-key-type.result    | 21 +++++++++++++++++++
>  .../gh-5093-before_replace-key-type.test.lua  |  7 +++++++
>  3 files changed, 31 insertions(+), 3 deletions(-)
>  create mode 100644 test/engine/gh-5093-before_replace-key-type.result
>  create mode 100644 test/engine/gh-5093-before_replace-key-type.test.lua
> 
> diff --git a/src/box/space.c b/src/box/space.c
> index eecbde7fa7..66768208b3 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -359,9 +359,6 @@ space_before_replace(struct space *space, struct txn *txn,
>  			return -1;
>  		key = request->key;
>  		part_count = mp_decode_array(&key);
> -		if (exact_key_validate(index->def->key_def,
> -				       key, part_count) != 0)
> -			return -1;
>  		break;
>  	case IPROTO_INSERT:
>  	case IPROTO_REPLACE:
> @@ -380,6 +377,9 @@ space_before_replace(struct space *space, struct txn *txn,
>  		/* Unknown request type, nothing to do. */
>  		return 0;
>  	}
> +	if (key != NULL &&
> +	    exact_key_validate(index->def->key_def, key, part_count) != 0)
> +		return -1;

1. The patch is technically correct. But I don't like that you
add key != NULL check to the hot path. Even though it is always != NULL
except blackhole engine space, which doesn't have indexes either.

Consider the refactoring below. I know it causes bigger diff, but
I consider it ok for the sake performance. As we learned recently,
even +1 or 2 ifs in a hot path may cost.

====================
diff --git a/src/box/space.c b/src/box/space.c
index 66768208b..73172ddd2 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -347,6 +347,7 @@ space_before_replace(struct space *space, struct txn *txn,
 	const char *key = NULL;
 	uint32_t part_count = 0;
 	struct index *index = NULL;
+	struct tuple *old_tuple = NULL;
 
 	/*
 	 * Lookup the old tuple.
@@ -364,7 +365,7 @@ space_before_replace(struct space *space, struct txn *txn,
 	case IPROTO_REPLACE:
 	case IPROTO_UPSERT:
 		if (pk == NULL)
-			break;
+			goto after_getting_old;
 		index = pk;
 		key = tuple_extract_key_raw(request->tuple, request->tuple_end,
 					    index->def->key_def, MULTIKEY_NONE,
@@ -377,15 +378,11 @@ space_before_replace(struct space *space, struct txn *txn,
 		/* Unknown request type, nothing to do. */
 		return 0;
 	}
-	if (key != NULL &&
-	    exact_key_validate(index->def->key_def, key, part_count) != 0)
+	if (exact_key_validate(index->def->key_def, key, part_count) != 0)
 		return -1;
-
-	struct tuple *old_tuple = NULL;
-	if (index != NULL &&
-	    index_get(index, key, part_count, &old_tuple) != 0)
+	if (index_get(index, key, part_count, &old_tuple) != 0)
 		return -1;
-
+after_getting_old:;
 	/*
 	 * Create the new tuple.
 	 */
====================

> diff --git a/test/engine/gh-5093-before_replace-key-type.test.lua b/test/engine/gh-5093-before_replace-key-type.test.lua
> new file mode 100644
> index 0000000000..9cba3c1597
> --- /dev/null
> +++ b/test/engine/gh-5093-before_replace-key-type.test.lua
> @@ -0,0 +1,7 @@
> +test_run = require('test_run').new()
> +trigger = function() end
> +
> +s = box.schema.space.create('gh-5093', {engine=test_run:get_cfg('engine')})
> +_ = s:create_index('value', {parts={{1, type='decimal'}}})
> +_ = s:before_replace(trigger)
> +s:replace{'1111.1111'}

2. Please, cleanup. Do s:drop().

Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace

[Ilya Kosarev]

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


 Hi!
 
Thanks for the review.
 
Send v2 of the the patch considering your comments, answers below. 
>Суббота, 4 июля 2020, 0:49 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>Hi! Thanks for the patch!
>
>See 2 comments below.
>
>On 03/07/2020 15:32, Ilya Kosarev wrote:
>> Since 9fba29abb4e05babb9b23b4413bd8083f0fba933 (memtx: introduce tuple
>> compare hint) index_get uses key_hint to get a comparison hint for the
>> key. It requires key type to be validated. However in
>> space_before_replace index_get was used without key validation on some
>> execution paths. Now it is fixed and validation is being performed if
>> needed. Corresponding test case is introduced.
>>
>> Closes #5093
>> ---
>> Branch:  https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5093-before_replace-key-validation
>> Issue:  https://github.com/tarantool/tarantool/issues/5093
>>
>> @ChangeLog:
>> * Add needed key validation to space_before_replace.
>>
>> src/box/space.c | 6 +++---
>> .../gh-5093-before_replace-key-type.result | 21 +++++++++++++++++++
>> .../gh-5093-before_replace-key-type.test.lua | 7 +++++++
>> 3 files changed, 31 insertions(+), 3 deletions(-)
>> create mode 100644 test/engine/gh-5093-before_replace-key-type.result
>> create mode 100644 test/engine/gh-5093-before_replace-key-type.test.lua
>>
>> diff --git a/src/box/space.c b/src/box/space.c
>> index eecbde7fa7..66768208b3 100644
>> --- a/src/box/space.c
>> +++ b/src/box/space.c
>> @@ -359,9 +359,6 @@ space_before_replace(struct space *space, struct txn *txn,
>> return -1;
>> key = request->key;
>> part_count = mp_decode_array(&key);
>> - if (exact_key_validate(index->def->key_def,
>> - key, part_count) != 0)
>> - return -1;
>> break;
>> case IPROTO_INSERT:
>> case IPROTO_REPLACE:
>> @@ -380,6 +377,9 @@ space_before_replace(struct space *space, struct txn *txn,
>> /* Unknown request type, nothing to do. */
>> return 0;
>> }
>> + if (key != NULL &&
>> + exact_key_validate(index->def->key_def, key, part_count) != 0)
>> + return -1;
>
>1. The patch is technically correct. But I don't like that you
>add key != NULL check to the hot path. Even though it is always != NULL
>except blackhole engine space, which doesn't have indexes either.
>
>Consider the refactoring below. I know it causes bigger diff, but
>I consider it ok for the sake performance. As we learned recently,
>even +1 or 2 ifs in a hot path may cost.
Right, I totally agree here, didn’t give this enough thought.
>
>====================
>diff --git a/src/box/space.c b/src/box/space.c
>index 66768208b..73172ddd2 100644
>--- a/src/box/space.c
>+++ b/src/box/space.c
>@@ -347,6 +347,7 @@ space_before_replace(struct space *space, struct txn *txn,
>  const char *key = NULL;
>  uint32_t part_count = 0;
>  struct index *index = NULL;
>+ struct tuple *old_tuple = NULL;
> 
>  /*
>  * Lookup the old tuple.
>@@ -364,7 +365,7 @@ space_before_replace(struct space *space, struct txn *txn,
>  case IPROTO_REPLACE:
>  case IPROTO_UPSERT:
>  if (pk == NULL)
>- break;
>+ goto after_getting_old;
>  index = pk;
>  key = tuple_extract_key_raw(request->tuple, request->tuple_end,
>  index->def->key_def, MULTIKEY_NONE,
>@@ -377,15 +378,11 @@ space_before_replace(struct space *space, struct txn *txn,
>  /* Unknown request type, nothing to do. */
>  return 0;
>  }
>- if (key != NULL &&
>- exact_key_validate(index->def->key_def, key, part_count) != 0)
>+ if (exact_key_validate(index->def->key_def, key, part_count) != 0)
>  return -1;
>-
>- struct tuple *old_tuple = NULL;
>- if (index != NULL &&
>- index_get(index, key, part_count, &old_tuple) != 0)
>+ if (index_get(index, key, part_count, &old_tuple) != 0)
>  return -1;
>-
>+after_getting_old:;
>  /*
>  * Create the new tuple.
>  */
>====================
>
>> diff --git a/test/engine/gh-5093-before_replace-key-type.test.lua b/test/engine/gh-5093-before_replace-key-type.test.lua
>> new file mode 100644
>> index 0000000000..9cba3c1597
>> --- /dev/null
>> +++ b/test/engine/gh-5093-before_replace-key-type.test.lua
>> @@ -0,0 +1,7 @@
>> +test_run = require('test_run').new()
>> +trigger = function() end
>> +
>> +s = box.schema.space.create('gh-5093', {engine=test_run:get_cfg('engine')})
>> +_ = s:create_index('value', {parts={{1, type='decimal'}}})
>> +_ = s:before_replace(trigger)
>> +s:replace{'1111.1111'}
>
>2. Please, cleanup. Do s:drop().
Right.
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 5863 bytes --]

Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace

[Vladislav Shpilevoy]

LGTM.

Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace

[Kirill Yukhin]

Hello,

On 03 июл 16:32, Ilya Kosarev wrote:
> Since 9fba29abb4e05babb9b23b4413bd8083f0fba933 (memtx: introduce tuple
> compare hint) index_get uses key_hint to get a comparison hint for the
> key. It requires key type to be validated. However in
> space_before_replace index_get was used without key validation on some
> execution paths. Now it is fixed and validation is being performed if
> needed. Corresponding test case is introduced.
> 
> Closes #5093
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5093-before_replace-key-validation
> Issue: https://github.com/tarantool/tarantool/issues/5093

LGTM.

I've checked your patch into 2.3, 2.4 and master.

--
Regards, Kirill Yukhin