* [Tarantool-patches] [PATCH] engine: validate key in space_before_replace @ 2020-07-03 13:32 Ilya Kosarev 2020-07-03 21:49 ` Vladislav Shpilevoy 2020-07-14 11:03 ` Kirill Yukhin 0 siblings, 2 replies; 5+ messages in thread From: Ilya Kosarev @ 2020-07-03 13:32 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace 2020-07-03 13:32 [Tarantool-patches] [PATCH] engine: validate key in space_before_replace Ilya Kosarev @ 2020-07-03 21:49 ` Vladislav Shpilevoy 2020-07-12 19:59 ` Ilya Kosarev 2020-07-14 11:03 ` Kirill Yukhin 1 sibling, 1 reply; 5+ messages in thread From: Vladislav Shpilevoy @ 2020-07-03 21:49 UTC (permalink / raw) To: Ilya Kosarev, tarantool-patches 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(). ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace 2020-07-03 21:49 ` Vladislav Shpilevoy @ 2020-07-12 19:59 ` Ilya Kosarev 2020-07-13 22:35 ` Vladislav Shpilevoy 0 siblings, 1 reply; 5+ messages in thread From: Ilya Kosarev @ 2020-07-12 19:59 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace 2020-07-12 19:59 ` Ilya Kosarev @ 2020-07-13 22:35 ` Vladislav Shpilevoy 0 siblings, 0 replies; 5+ messages in thread From: Vladislav Shpilevoy @ 2020-07-13 22:35 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches LGTM. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace 2020-07-03 13:32 [Tarantool-patches] [PATCH] engine: validate key in space_before_replace Ilya Kosarev 2020-07-03 21:49 ` Vladislav Shpilevoy @ 2020-07-14 11:03 ` Kirill Yukhin 1 sibling, 0 replies; 5+ messages in thread From: Kirill Yukhin @ 2020-07-14 11:03 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-14 11:03 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-03 13:32 [Tarantool-patches] [PATCH] engine: validate key in space_before_replace Ilya Kosarev 2020-07-03 21:49 ` Vladislav Shpilevoy 2020-07-12 19:59 ` Ilya Kosarev 2020-07-13 22:35 ` Vladislav Shpilevoy 2020-07-14 11:03 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox