Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Ilya Kosarev <i.kosarev@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace
Date: Fri, 3 Jul 2020 23:49:12 +0200	[thread overview]
Message-ID: <d01dc646-ba76-a026-1987-35d250e13c1b@tarantool.org> (raw)
In-Reply-To: <20200703133255.23820-1-i.kosarev@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.

====================
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().

  reply	other threads:[~2020-07-03 21:49 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-03 13:32 Ilya Kosarev
2020-07-03 21:49 ` Vladislav Shpilevoy [this message]
2020-07-12 19:59   ` Ilya Kosarev
2020-07-13 22:35     ` Vladislav Shpilevoy
2020-07-14 11:03 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d01dc646-ba76-a026-1987-35d250e13c1b@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=i.kosarev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] engine: validate key in space_before_replace' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox