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

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- 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 --]