[Tarantool-patches] [PATCH 0/3] Credentials follow up

[Tarantool-patches] [PATCH 0/3] Credentials follow up

[Vladislav Shpilevoy]

The patchset fixes a flaky test, a C++ exception thrown in C,
and invalid memory access problem left after the patch making
struct credentials a complex object with a constructor and a
destructor.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4597-credentials-follow-up-2
Issue: https://github.com/tarantool/tarantool/issues/4597

Vladislav Shpilevoy (3):
  test: fix flaky box/access_sysview.test.lua
  user: don't throw C++ exception from user_find_by_name
  session: su left dangling credentials object on stack

 src/box/lua/session.c            |  3 +--
 src/box/user.cc                  |  2 +-
 test/box/access.result           | 11 +++++++++++
 test/box/access.test.lua         |  6 ++++++
 test/box/access_sysview.result   |  2 +-
 test/box/access_sysview.test.lua |  2 +-
 6 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH 1/3] test: fix flaky box/access_sysview.test.lua

[Vladislav Shpilevoy]

Some guest user privileges were not revoked in the
end.
---
 test/box/access_sysview.result   | 2 +-
 test/box/access_sysview.test.lua | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
index 3072b7394..1f33deceb 100644
--- a/test/box/access_sysview.result
+++ b/test/box/access_sysview.result
@@ -689,7 +689,7 @@ box.session.su('admin')
 ---
 ...
 -- _vcollation is readable anyway.
-box.schema.user.revoke("guest", "read", "space", "_collation")
+box.schema.user.revoke("guest", "read, write, alter, execute", "space", "_collation")
 ---
 ...
 box.session.su("guest")
diff --git a/test/box/access_sysview.test.lua b/test/box/access_sysview.test.lua
index 031df28aa..17420ae33 100644
--- a/test/box/access_sysview.test.lua
+++ b/test/box/access_sysview.test.lua
@@ -287,7 +287,7 @@ box.space._vcollation:select{0}
 box.session.su('admin')
 
 -- _vcollation is readable anyway.
-box.schema.user.revoke("guest", "read", "space", "_collation")
+box.schema.user.revoke("guest", "read, write, alter, execute", "space", "_collation")
 box.session.su("guest")
 #box.space._vcollation:select{}
 session.su('admin')
-- 
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH 2/3] user: don't throw C++ exception from user_find_by_name

[Vladislav Shpilevoy]

This function is supposed to return NULL on an error.
For exceptions there is user_find_by_name_xc.
---
 src/box/user.cc          |  2 +-
 test/box/access.result   | 11 +++++++++++
 test/box/access.test.lua |  6 ++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/box/user.cc b/src/box/user.cc
index 6b562b1f0..03b4b2e3b 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -519,7 +519,7 @@ user_find_by_name(const char *name, uint32_t len)
 {
 	uint32_t uid;
 	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
-		diag_raise();
+		return NULL;
 	struct user *user = user_by_id(uid);
 	if (user == NULL || user->def->type != SC_USER) {
 		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
diff --git a/test/box/access.result b/test/box/access.result
index 3976adfde..dc339038d 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -239,6 +239,17 @@ session.user()
 ---
 - admin
 ...
+-- Invalid user.
+session.su('does not exist')
+---
+- error: User 'does not exist' is not found
+...
+-- The point of this test is to try a name > max
+-- allowed name.
+session.su(string.rep('a', 66000))
+---
+- error: name length 66000 is greater than BOX_NAME_MAX
+...
 --------------------------------------------------------------------------------
 -- Check if identifiers obey the common constraints
 --------------------------------------------------------------------------------
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 89a63c904..a9843d155 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -100,6 +100,12 @@ session.su('admin')
 box.schema.user.drop('tester')
 session.user()
 
+-- Invalid user.
+session.su('does not exist')
+-- The point of this test is to try a name > max
+-- allowed name.
+session.su(string.rep('a', 66000))
+
 --------------------------------------------------------------------------------
 -- Check if identifiers obey the common constraints
 --------------------------------------------------------------------------------
-- 
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH 3/3] session: su left dangling credentials object on stack

[Vladislav Shpilevoy]

Box.session.su() worked like following: check user
existence, create its credentials on the stack, check
the function, call the function, destroy the
credentials, restore the old credentials.

After creating the credentials on the stack the
function check could raise a Lua error. It led to the
credentials object not being destroyed. As a result,
user.credentials_list was pointing at invalid memory.

Now there is no errors between creating the temporary
credentials and its destruction.

Closes #4597
---
 src/box/lua/session.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index de5eb9adc..c6a600f6f 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -189,14 +189,13 @@ lbox_session_su(struct lua_State *L)
 		fiber_set_user(fiber(), &session->credentials);
 		return 0; /* su */
 	}
+	luaL_checktype(L, 2, LUA_TFUNCTION);
 
 	struct credentials su_credentials;
 	struct credentials *old_credentials = fiber()->storage.credentials;
 	credentials_create(&su_credentials, user);
 	fiber()->storage.credentials = &su_credentials;
 
-	/* sudo */
-	luaL_checktype(L, 2, LUA_TFUNCTION);
 	int error = lua_pcall(L, top - 2, LUA_MULTRET, 0);
 	/* Restore the original credentials. */
 	fiber_set_user(fiber(), old_credentials);
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH 0/3] Credentials follow up

[Kirill Yukhin]

Hello,

On 31 окт 22:42, Vladislav Shpilevoy wrote:
> The patchset fixes a flaky test, a C++ exception thrown in C,
> and invalid memory access problem left after the patch making
> struct credentials a complex object with a constructor and a
> destructor.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4597-credentials-follow-up-2
> Issue: https://github.com/tarantool/tarantool/issues/4597

I've checked your patches into 2.1 2.2 and master.

I've also checked in `session` patch into 1.10.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 1/3] test: fix flaky box/access_sysview.test.lua

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/01 00:39]:
> Some guest user privileges were not revoked in the
> end.
> ---
>  test/box/access_sysview.result   | 2 +-
>  test/box/access_sysview.test.lua | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
> index 3072b7394..1f33deceb 100644
> --- a/test/box/access_sysview.result
> +++ b/test/box/access_sysview.result
> @@ -689,7 +689,7 @@ box.session.su('admin')
>  ---
>  ...
>  -- _vcollation is readable anyway.
> -box.schema.user.revoke("guest", "read", "space", "_collation")
> +box.schema.user.revoke("guest", "read, write, alter, execute", "space", "_collation")
>  ---
>  ...
>  box.session.su("guest")
> diff --git a/test/box/access_sysview.test.lua b/test/box/access_sysview.test.lua
> index 031df28aa..17420ae33 100644
> --- a/test/box/access_sysview.test.lua
> +++ b/test/box/access_sysview.test.lua
> @@ -287,7 +287,7 @@ box.space._vcollation:select{0}
>  box.session.su('admin')
>  
>  -- _vcollation is readable anyway.
> -box.schema.user.revoke("guest", "read", "space", "_collation")
> +box.schema.user.revoke("guest", "read, write, alter, execute", "space", "_collation")
>  box.session.su("guest")
>  #box.space._vcollation:select{}
>  session.su('admin')

lgtm


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 2/3] user: don't throw C++ exception from user_find_by_name

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/01 00:39]:
> +-- Invalid user.
> +session.su('does not exist')
> +---
> +- error: User 'does not exist' is not found
> +...
> +-- The point of this test is to try a name > max
> +-- allowed name.
> +session.su(string.rep('a', 66000))
> +---
> +- error: name length 66000 is greater than BOX_NAME_MAX
> +...

That's pretty strange, I think we had such tests before.

Did you actually verify that the tests do not pass before your
fix? Or they do pass because there is a catch clause somewhere
up the stack anyway?

Please clarify in the comment.

Otherwise LGTM.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 3/3] session: su left dangling credentials object on stack

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/01 00:39]:
> Box.session.su() worked like following: check user
> existence, create its credentials on the stack, check
> the function, call the function, destroy the
> credentials, restore the old credentials.
> 
> After creating the credentials on the stack the
> function check could raise a Lua error. It led to the
> credentials object not being destroyed. As a result,
> user.credentials_list was pointing at invalid memory.
> 
> Now there is no errors between creating the temporary
> credentials and its destruction.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 0/3] Credentials follow up

[Konstantin Osipov]

* Kirill Yukhin <kyukhin@tarantool.org> [19/11/01 16:59]:
> Hello,
> 
> On 31 окт 22:42, Vladislav Shpilevoy wrote:
> > The patchset fixes a flaky test, a C++ exception thrown in C,
> > and invalid memory access problem left after the patch making
> > struct credentials a complex object with a constructor and a
> > destructor.
> > 
> > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4597-credentials-follow-up-2
> > Issue: https://github.com/tarantool/tarantool/issues/4597
> 
> I've checked your patches into 2.1 2.2 and master.
> 
> I've also checked in `session` patch into 1.10.

yet another check in without a public review.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 2/3] user: don't throw C++ exception from user_find_by_name

[Vladislav Shpilevoy]

On 04/11/2019 20:22, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/01 00:39]:
>> +-- Invalid user.
>> +session.su('does not exist')
>> +---
>> +- error: User 'does not exist' is not found
>> +...
>> +-- The point of this test is to try a name > max
>> +-- allowed name.
>> +session.su(string.rep('a', 66000))
>> +---
>> +- error: name length 66000 is greater than BOX_NAME_MAX
>> +...
> 
> That's pretty strange, I think we had such tests before.
> 
> Did you actually verify that the tests do not pass before your
> fix? Or they do pass because there is a catch clause somewhere
> up the stack anyway?

That test fails before my patch. The error was 'C++ exception'.
Now it is a ClientError object.

> 
> Please clarify in the comment.
> 
> Otherwise LGTM.
> 
>