[Tarantool-patches] [PATCH v4] lua/key_def: fix compare_with_key() part count check

[Tarantool-patches] [PATCH v4] lua/key_def: fix compare_with_key() part count check

[Sergey Nikiforov]

Added corresponding test

Fixes: #5307
---

Issue: https://github.com/tarantool/tarantool/issues/5307
Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5307-fix-key_def-part-count-check-v4

 src/box/lua/key_def.c                         |  9 +++----
 .../gh-5307-key_def-part-count-check.test.lua | 26 +++++++++++++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100755 test/box-tap/gh-5307-key_def-part-count-check.test.lua

diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index a781aeff9..5143ef3a4 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -360,17 +360,14 @@ lbox_key_def_compare_with_key(struct lua_State *L)
 	struct region *region = &fiber()->gc;
 	size_t region_svp = region_used(region);
 	size_t key_len;
-	const char *key_end, *key = lbox_encode_tuple_on_gc(L, 3, &key_len);
-	uint32_t part_count = mp_decode_array(&key);
-	if (key_validate_parts(key_def, key, part_count, true,
-			       &key_end) != 0) {
+	const char *key = lbox_encode_tuple_on_gc(L, 3, &key_len);
+	if (box_key_def_validate_key(key_def, key, NULL)) {
 		region_truncate(region, region_svp);
 		tuple_unref(tuple);
 		return luaT_error(L);
 	}
 
-	int rc = tuple_compare_with_key(tuple, HINT_NONE, key,
-					part_count, HINT_NONE, key_def);
+	int rc = box_tuple_compare_with_key(tuple, key, key_def);
 	region_truncate(region, region_svp);
 	tuple_unref(tuple);
 	lua_pushinteger(L, rc);
diff --git a/test/box-tap/gh-5307-key_def-part-count-check.test.lua b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
new file mode 100755
index 000000000..d9458c95a
--- /dev/null
+++ b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
@@ -0,0 +1,26 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local mytest = tap.test('key_def part count tests')
+
+mytest:plan(3)
+
+local key_def = require('key_def')
+local kd = key_def.new({{fieldno = 1, type = 'unsigned'}})
+local ok, res
+
+-- Should succeed
+ok, res = pcall(kd.compare_with_key, kd, {1}, {1})
+mytest:ok(ok and res == 0, "Simple equality")
+
+-- Should succeed
+ok, res = pcall(kd.compare_with_key, kd, {1}, {2})
+mytest:ok(ok and res < 0, "Simple inequality")
+
+-- Should fail
+local exp_err = "Invalid key part count (expected [0..1], got 9)"
+ok, res = pcall(kd.compare_with_key, kd, {1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
+mytest:is_deeply({ok, tostring(res)}, {false, exp_err},
+    "Invalid key part count")
+
+os.exit(mytest:check() and 0 or 1)
-- 
2.25.1

Re: [Tarantool-patches] [PATCH v4] lua/key_def: fix compare_with_key() part count check

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

LGTM.

Re: [Tarantool-patches] [PATCH v4] lua/key_def: fix compare_with_key() part count check

[Alexander Turenko]

> Issue: https://github.com/tarantool/tarantool/issues/5307
> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5307-fix-key_def-part-count-check-v4

LGTM.

I left several style / taste comments regarding the test. It is just for
you. I think they don't worth one more review iteration.

Alexander Ti., please, approve the push from the QA team side.

Kirill, please, push to master and cherry-pick to 2.6 and 2.5.

Changelog entry ('Lua' section):

* Fixed a segfault in `key_def.compare_with_key()` on an invalid key (gh-5307).

WBR, Alexander Turenko.

----

> diff --git a/test/box-tap/gh-5307-key_def-part-count-check.test.lua b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
> new file mode 100755
> index 000000000..d9458c95a
> --- /dev/null
> +++ b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
> @@ -0,0 +1,26 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local mytest = tap.test('key_def part count tests')
> +
> +mytest:plan(3)

Usually 'test'.

> +
> +local key_def = require('key_def')

Usually all requires are go first, when possible.

> +local kd = key_def.new({{fieldno = 1, type = 'unsigned'}})
> +local ok, res
> +
> +-- Should succeed

We trying to write comments in the same style: start with a capital
letter, end with a period.

> +ok, res = pcall(kd.compare_with_key, kd, {1}, {1})
> +mytest:ok(ok and res == 0, "Simple equality")

In my personal taste, it is good to use the same quotes (single or
double) when possible within at least a file. (I prefer single ones, but
without any reason, just get into the habit.)

> +
> +-- Should succeed
> +ok, res = pcall(kd.compare_with_key, kd, {1}, {2})
> +mytest:ok(ok and res < 0, "Simple inequality")
> +
> +-- Should fail
> +local exp_err = "Invalid key part count (expected [0..1], got 9)"
> +ok, res = pcall(kd.compare_with_key, kd, {1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
> +mytest:is_deeply({ok, tostring(res)}, {false, exp_err},
> +    "Invalid key part count")
> +
> +os.exit(mytest:check() and 0 or 1)
> -- 
> 2.25.1
> 

Re: [Tarantool-patches] [PATCH v4] lua/key_def: fix compare_with_key() part count check

[Kirill Yukhin]

Hello,

On 22 Dec 11:28, Sergey Nikiforov via Tarantool-patches wrote:
> Added corresponding test
> 
> Fixes: #5307
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/5307
> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5307-fix-key_def-part-count-check-v4

I've checked your patch into 2.6 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v4] lua/key_def: fix compare_with_key() part count check

[Alexander Turenko]

On Thu, Dec 24, 2020 at 06:07:55PM +0000, Kirill Yukhin wrote:
> Hello,
> 
> On 22 Dec 11:28, Sergey Nikiforov via Tarantool-patches wrote:
> > Added corresponding test
> > 
> > Fixes: #5307
> > ---
> > 
> > Issue: https://github.com/tarantool/tarantool/issues/5307
> > Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5307-fix-key_def-part-count-check-v4
> 
> I've checked your patch into 2.6 and master.

Please, push to 2.5 as well.

Don't forget to mention it is the release notes:

https://lists.tarantool.org/pipermail/tarantool-patches/2020-December/021650.html

Re: [Tarantool-patches] [PATCH v4] lua/key_def: fix compare_with_key() part count check

[Kirill Yukhin]

Hello,

On 24 Dec 21:18, Alexander Turenko wrote:
> On Thu, Dec 24, 2020 at 06:07:55PM +0000, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 22 Dec 11:28, Sergey Nikiforov via Tarantool-patches wrote:
> > > Added corresponding test
> > > 
> > > Fixes: #5307
> > > ---
> > > 
> > > Issue: https://github.com/tarantool/tarantool/issues/5307
> > > Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5307-fix-key_def-part-count-check-v4
> > 
> > I've checked your patch into 2.6 and master.
> 
> Please, push to 2.5 as well.

Done.

--
Regards, Kirill Yukhin