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

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

[Sergey Nikiforov]

Added corresponding test

Fixes: #5307
---
 src/box/lua/key_def.c                         |  9 +++++++
 .../gh-5307-key_def-part-count-check.result   |  1 +
 .../gh-5307-key_def-part-count-check.test.lua | 25 +++++++++++++++++++
 3 files changed, 35 insertions(+)
 create mode 100644 test/app-tap/gh-5307-key_def-part-count-check.result
 create mode 100755 test/app-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..674891a85 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
 	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 (part_count > key_def->part_count) {
+		region_truncate(region, region_svp);
+		tuple_unref(tuple);
+		diag_set(ClientError, ER_KEY_PART_COUNT,
+			 key_def->part_count, part_count);
+		return luaT_error(L);
+	}
+
 	if (key_validate_parts(key_def, key, part_count, true,
 			       &key_end) != 0) {
 		region_truncate(region, region_svp);
diff --git a/test/app-tap/gh-5307-key_def-part-count-check.result b/test/app-tap/gh-5307-key_def-part-count-check.result
new file mode 100644
index 000000000..d47a905a5
--- /dev/null
+++ b/test/app-tap/gh-5307-key_def-part-count-check.result
@@ -0,0 +1 @@
+Failed (as it should), error message: Invalid key part count (expected [0..1], got 9)
diff --git a/test/app-tap/gh-5307-key_def-part-count-check.test.lua b/test/app-tap/gh-5307-key_def-part-count-check.test.lua
new file mode 100755
index 000000000..8c8f6f34b
--- /dev/null
+++ b/test/app-tap/gh-5307-key_def-part-count-check.test.lua
@@ -0,0 +1,25 @@
+#!/usr/bin/env tarantool
+
+key_def = require('key_def')
+kd = key_def.new({{fieldno = 1, type = 'unsigned'}})
+
+-- Should succeed
+if (kd:compare_with_key({1}, {1}) ~= 0) then
+    print("Error: should be equal")
+end
+
+-- Should succeed
+if (kd:compare_with_key({1}, {2}) >= 0) then
+    print("Error: should be less")
+end
+
+-- Should fail
+ok, err = pcall(function()
+	kd:compare_with_key({1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
+    end)
+if (ok) then
+    print("Error: it should not have succeeded")
+else
+    print("Failed (as it should), error message: " .. err)
+end
+
-- 
2.25.1

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

[Vladislav Shpilevoy]

Hi!

On 14.12.2020 16:35, Sergey Nikiforov wrote:
> Added corresponding test
> 
> Fixes: #5307
> ---

Please, try to follow the guidelines.
https://github.com/tarantool/tarantool/wiki/Code-review-procedure

I don't see the issue and branch links. Also I can't find the
branch in `git branch -a | grep 5307`. Also in the ticket's
webpage I don't see any links to the commit, which github usually
adds automatically. So I suspect you simply didn't push it anywhere.

> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
> index a781aeff9..674891a85 100644
> --- a/src/box/lua/key_def.c
> +++ b/src/box/lua/key_def.c
> @@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>  	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 (part_count > key_def->part_count) {
> +		region_truncate(region, region_svp);
> +		tuple_unref(tuple);
> +		diag_set(ClientError, ER_KEY_PART_COUNT,
> +			 key_def->part_count, part_count);
> +		return luaT_error(L);
> +	}

Why this check and the call below can't be all simply
replaces with box_key_def_validate_key() call?

> +
>  	if (key_validate_parts(key_def, key, part_count, true,
>  			       &key_end) != 0) {
>  		region_truncate(region, region_svp);

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

[Alexander Turenko]

> > diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
> > index a781aeff9..674891a85 100644
> > --- a/src/box/lua/key_def.c
> > +++ b/src/box/lua/key_def.c
> > @@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
> >  	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 (part_count > key_def->part_count) {
> > +		region_truncate(region, region_svp);
> > +		tuple_unref(tuple);
> > +		diag_set(ClientError, ER_KEY_PART_COUNT,
> > +			 key_def->part_count, part_count);
> > +		return luaT_error(L);
> > +	}
> 
> Why this check and the call below can't be all simply
> replaces with box_key_def_validate_key() call?

I agree.

I introduced this function to use in the corresponding place of the
tuple-keydef code:

https://github.com/tarantool/tuple-keydef/blob/10c00a9aa289bb16446239edfa056ee48b467161/tuple/keydef.c#L517

> 
> > +
> >  	if (key_validate_parts(key_def, key, part_count, true,
> >  			       &key_end) != 0) {
> >  		region_truncate(region, region_svp);

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

[Alexander Turenko]

> diff --git a/test/app-tap/gh-5307-key_def-part-count-check.result b/test/app-tap/gh-5307-key_def-part-count-check.result
> new file mode 100644
> index 000000000..d47a905a5
> --- /dev/null
> +++ b/test/app-tap/gh-5307-key_def-part-count-check.result
> @@ -0,0 +1 @@
> +Failed (as it should), error message: Invalid key part count (expected [0..1], got 9)
> diff --git a/test/app-tap/gh-5307-key_def-part-count-check.test.lua b/test/app-tap/gh-5307-key_def-part-count-check.test.lua
> new file mode 100755
> index 000000000..8c8f6f34b
> --- /dev/null
> +++ b/test/app-tap/gh-5307-key_def-part-count-check.test.lua

box-tap fit better here, because key_def (C part and the Lua module) is
part of the database implementation ('box').

> @@ -0,0 +1,25 @@
> +#!/usr/bin/env tarantool
> +
> +key_def = require('key_def')
> +kd = key_def.new({{fieldno = 1, type = 'unsigned'}})

`x = 42` is equivalent of `_G.x = 42` if there is no definition of `x`
in the scope.

I would use 'local' for such code hunks. It does not change any
behaviour here, just for the sake of our usual style for Lua modules.

In a module it is undesirable to set a global variable, which accessible
for other modules. At least for a variable that is not intended to be
exported.

> +
> +-- Should succeed
> +if (kd:compare_with_key({1}, {1}) ~= 0) then
> +    print("Error: should be equal")
> +end

I would prefer to use 'tap' built-in module ([1]) for such tests. It
produces TAP13 output and does not require a result file. Most of tests
in the '*-tap' test suites procude TAP13 output.

[1]: https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/

> +
> +-- Should succeed
> +if (kd:compare_with_key({1}, {2}) >= 0) then
> +    print("Error: should be less")
> +end
> +
> +-- Should fail
> +ok, err = pcall(function()
> +	kd:compare_with_key({1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
> +    end)

The indentation is strange here. I would write it like so:

 | local exp_err = '<..expected error message..>'
 | local ok, err = pcall(kd.compare_with_key, kd, {1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
 | test:is_deeply({ok, tostring(err)}, {false, exp_err}, '<..short description..>')

tostring() is to convert a box.error object to a string, otherwise
is_deeply() fails to find them equal.

Or, with extra function:

 | local exp_err = '<..expected error message..>'
 | local ok, err = pcall(function()
 |     return kd:compare_with_key({1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
 | end)
 | test:is_deeply({ok, tostring(err)}, {false, exp_err}, '<..short description..>')

(Personally I prefer to avoid extra wrapping function, when we want to
just call one function under pcall).

> +if (ok) then
> +    print("Error: it should not have succeeded")
> +else
> +    print("Failed (as it should), error message: " .. err)
> +end

An if condition may be written without parentheses in Lua.

I would explicitly set an exit code:

 | os.exit(test:check() and 0 or 1)

This way we have more freedom in automation: say, we can run a subset of
tests without test-run and check their exit code without checks for
output. To be honest, I just find it more intuitive, when a failing test
process set a non-zero exit code.

It would be great to also open a PR to the tuple-keydef module with the
new test. I fixed the corresponding problem in the module, but don't add
any test for it.

https://github.com/tarantool/tuple-keydef/issues/7

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

[Sergey Nikiforov]

Hi!

On 17.12.2020 1:34, Vladislav Shpilevoy wrote:
> Hi!
> 
> On 14.12.2020 16:35, Sergey Nikiforov wrote:
>> Added corresponding test
>>
>> Fixes: #5307
>> ---
> 
> Please, try to follow the guidelines.
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure
> 
> I don't see the issue and branch links. Also I can't find the
> branch in `git branch -a | grep 5307`. Also in the ticket's
> webpage I don't see any links to the commit, which github usually
> adds automatically. So I suspect you simply didn't push it anywhere.

Of course I did not push my branches for the first batch of patches - 
because no one said I had to. I am new to github and Tarantool workflow, 
sorry.

>> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
>> index a781aeff9..674891a85 100644
>> --- a/src/box/lua/key_def.c
>> +++ b/src/box/lua/key_def.c
>> @@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>>   	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 (part_count > key_def->part_count) {
>> +		region_truncate(region, region_svp);
>> +		tuple_unref(tuple);
>> +		diag_set(ClientError, ER_KEY_PART_COUNT,
>> +			 key_def->part_count, part_count);
>> +		return luaT_error(L);
>> +	}
> 
> Why this check and the call below can't be all simply
> replaces with box_key_def_validate_key() call?

Because we need part_count later. With box_key_def_validate_key() we 
would have to call mp_decode_array() twice or add yet another parameter 
to box_key_def_validate_key(). Is that good idea?

>> +
>>   	if (key_validate_parts(key_def, key, part_count, true,
>>   			       &key_end) != 0) {
>>   		region_truncate(region, region_svp);

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

[Alexander Turenko]

On Thu, Dec 17, 2020 at 04:27:09PM +0300, Sergey Nikiforov wrote:
> > > diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
> > > index a781aeff9..674891a85 100644
> > > --- a/src/box/lua/key_def.c
> > > +++ b/src/box/lua/key_def.c
> > > @@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
> > >   	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 (part_count > key_def->part_count) {
> > > +		region_truncate(region, region_svp);
> > > +		tuple_unref(tuple);
> > > +		diag_set(ClientError, ER_KEY_PART_COUNT,
> > > +			 key_def->part_count, part_count);
> > > +		return luaT_error(L);
> > > +	}
> > 
> > Why this check and the call below can't be all simply
> > replaces with box_key_def_validate_key() call?
> 
> Because we need part_count later. With box_key_def_validate_key() we would
> have to call mp_decode_array() twice or add yet another parameter to
> box_key_def_validate_key(). Is that good idea?

We can't change box_key_def_validate_key() parameters, it is in the
public C API.

The code would look more accurate if we'll reuse the public functions
(box_key_def_validate_key() and box_tuple_compare_with_key()) here.
However, right, it'll decode the msgpack array size twice.

I think it is negligible comparing to the validation of the key against
given key_def. If'll need maximum performance from the module, we'll add
an option to skip validation at all.

> 
> > > +
> > >   	if (key_validate_parts(key_def, key, part_count, true,
> > >   			       &key_end) != 0) {
> > >   		region_truncate(region, region_svp);

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

[Vladislav Shpilevoy]

On 17.12.2020 14:27, Sergey Nikiforov via Tarantool-patches wrote:
> Hi!
> 
> On 17.12.2020 1:34, Vladislav Shpilevoy wrote:
>> Hi!
>>
>> On 14.12.2020 16:35, Sergey Nikiforov wrote:
>>> Added corresponding test
>>>
>>> Fixes: #5307
>>> ---
>>
>> Please, try to follow the guidelines.
>> https://github.com/tarantool/tarantool/wiki/Code-review-procedure
>>
>> I don't see the issue and branch links. Also I can't find the
>> branch in `git branch -a | grep 5307`. Also in the ticket's
>> webpage I don't see any links to the commit, which github usually
>> adds automatically. So I suspect you simply didn't push it anywhere.
> 
> Of course I did not push my branches for the first batch of patches - because no one said I had to. I am new to github and Tarantool workflow, sorry.

Ok, now you know! But where is the branch? I don't see a link,
and `git branch -a | grep 5307` is empty.

>>> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
>>> index a781aeff9..674891a85 100644
>>> --- a/src/box/lua/key_def.c
>>> +++ b/src/box/lua/key_def.c
>>> @@ -362,6 +362,15 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>>>       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 (part_count > key_def->part_count) {
>>> +        region_truncate(region, region_svp);
>>> +        tuple_unref(tuple);
>>> +        diag_set(ClientError, ER_KEY_PART_COUNT,
>>> +             key_def->part_count, part_count);
>>> +        return luaT_error(L);
>>> +    }
>>
>> Why this check and the call below can't be all simply
>> replaces with box_key_def_validate_key() call?
> 
> Because we need part_count later. With box_key_def_validate_key() we would have to call mp_decode_array() twice or add yet another parameter to box_key_def_validate_key(). Is that good idea?

Ok, then the patch looks fine in the email. I would like
to see it on the branch to check how it works.

You may also try to move region_truncate + tuple_unref to
some common place and do goto to there. These actions are
done 3 times now.