[Tarantool-patches] [PATCH 0/3] lua/pwd: workaround the systemd bug

[Tarantool-patches] [PATCH 0/3] lua/pwd: workaround the systemd bug

[Cyrill Gorcunov]

Guys, take a look please once time permit. The first two patches
are optional but I think while we're at this code and we could
improve the code base we should.

Also I've been thinking on how to test the issue -- we need a buggy
systemd instance running and we actually have Fedora 29 on gitlab
where tests are passed but it is unclear for me how exactly we could
use it. Initially I thought of injecting some error code into pwd module
itself but then I left this idea of making code more ugly just because
of some bug in the particular systemd version.

So my criteria was that the change must not break anything in existing
tests but if you have some idea for testing, please share.

issue https://github.com/tarantool/tarantool/issues/5034
branch gorcunov/gh-5034-passwd

Cyrill Gorcunov (3):
  lua/errno: use lengthof helper
  lua/errno: shrink memory usage on error declaration
  lua/pwd: workaround the systemd bug

 src/lua/errno.c |  5 ++---
 src/lua/pwd.lua | 52 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 15 deletions(-)


base-commit: c1f72aeb7657a95cbce94f700ab432ba16cecb92
-- 
2.26.2

[Tarantool-patches] [PATCH 1/3] lua/errno: use lengthof helper

[Cyrill Gorcunov]

No need for ending empty entry.

Part-of #5034

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lua/errno.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/lua/errno.c b/src/lua/errno.c
index 5c9101c64..3d3947e75 100644
--- a/src/lua/errno.c
+++ b/src/lua/errno.c
@@ -281,14 +281,13 @@ tarantool_lua_errno_init(struct lua_State *L)
 #ifdef	EXDEV
 		{ "EXDEV",		EXDEV		},
 #endif
-		{ "",			0		}
 	};
 
 	static const luaL_Reg errnolib[] = {
 		{ NULL, NULL}
 	};
 	luaL_register_module(L, "errno", errnolib);
-	for (int i = 0; elist[i].name[0]; i++) {
+	for (int i = 0; i < (int)lengthof(elist); i++) {
 		lua_pushstring(L, elist[i].name);
 		lua_pushinteger(L, elist[i].value);
 		lua_rawset(L, -3);
-- 
2.26.2

[Tarantool-patches] [PATCH 2/3] lua/errno: shrink memory usage on error declaration

[Cyrill Gorcunov]

There is no need to allocate 32 bytes per each string,
the backend lua does copy the string internally thus
plain pointer is enough here no need to allocate redundant
memory.

Part-of #5034

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lua/errno.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lua/errno.c b/src/lua/errno.c
index 3d3947e75..c0a416c11 100644
--- a/src/lua/errno.c
+++ b/src/lua/errno.c
@@ -42,7 +42,7 @@ extern char errno_lua[];
 void
 tarantool_lua_errno_init(struct lua_State *L)
 {
-	static const struct { char name[32]; int value; } elist[] = {
+	static const struct { char *name; int value; } elist[] = {
 
 #ifdef	E2BIG
 		{ "E2BIG",		E2BIG		},
-- 
2.26.2

[Tarantool-patches] [PATCH 3/3] lua/pwd: workaround the systemd bug

[Cyrill Gorcunov]

There is a bug in systemd-209 source code: it returns
ENOENT when no more entries in a password database left.

Later the issue been fixed but we still meet the systems
where it hits. The problem affects getpwent/getgrent calls
only thus we can expect them to return the buggy error code
to skip.

Notes:

1) See systemd's commit where issue been fixed

   | commit 06202b9e659e5cc72aeecc5200155b7c012fccbc
   | Author: Yu Watanabe <watanabe.yu+github@gmail.com>
   | Date:   Sun Jul 15 23:00:00 2018 +0900
   |
   |     nss: do not modify errno when NSS_STATUS_NOTFOUND or NSS_STATUS_SUCCESS

2) Another option is to call getpwall on Tarantool startup
   unconditionally where we could simply ignore any errors. This
   is a very bad choise since traversig a password database might
   introduce significant lags if backend does some network activiy
   or have expired caches. Thus drop getpwall() unconditional call
   run it iif a user does an explicit request.

Fixes #5034

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lua/pwd.lua | 52 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/src/lua/pwd.lua b/src/lua/pwd.lua
index 31cfbd8bf..907eb5a8e 100644
--- a/src/lua/pwd.lua
+++ b/src/lua/pwd.lua
@@ -196,6 +196,44 @@ local function getpw(uid)
     return pwtotable(pw, gr)
 end
 
+--
+-- systemd v209 sets errno is set to ENOENT in
+-- _nss_systemd_getpwent_r and _nss_systemd_getgrent_r
+-- when there are no more entries to enumerate.
+--
+-- This is a bug which has been fixed later in
+-- systemd code but we have to deal with buggy
+-- systems thus ignore such error if appears.
+--
+-- See the reference for details
+-- https://github.com/systemd/systemd/issues/9585
+--
+-- This issue affects getpwent/getgrent calls only
+-- thus provide own wrappings to keep workaround
+-- in one place and do not affect any other calls
+-- where ENOENT might become a valid error case.
+--
+-- Initially we observed this issue on Fedora 29
+-- when a password database is traversed for the
+-- first time.
+local function getpwent()
+    errno(0)
+    local pw = ffi.C.getpwent()
+    if pw == nil and errno() == errno.ENOENT then
+        errno(0)
+    end
+    return pw
+end
+
+local function getgrent()
+    errno(0)
+    local gr = ffi.C.getgrent()
+    if gr == nil and errno() == errno.ENOENT then
+        errno(0)
+    end
+    return gr
+end
+
 local function getpwall()
     ffi.C.setpwent()
     local pws = {}
@@ -203,8 +241,7 @@ local function getpwall()
     -- of a getpwent() current entry to a first one on CentOS 6
     -- and FreeBSD 12.
     while true do
-        errno(0)
-        local pw = ffi.C.getpwent()
+        local pw = getpwent()
         if pw == nil then
             pwgrcheck('getpwall', pw)
             break
@@ -223,8 +260,7 @@ local function getgrall()
     -- of a getgrent() current entry to a first one on CentOS 6
     -- and FreeBSD 12.
     while true do
-        errno(0)
-        local gr = ffi.C.getgrent()
+        local gr = getgrent()
         if gr == nil then
             pwgrcheck('getgrall', gr)
             break
@@ -237,14 +273,6 @@ end
 
 -- }}}
 
--- Workaround pwd.getpwall() issue on Fedora 29: successful
--- getgrent() call that should normally return NULL and preserve
--- errno, set it to ENOENT due to systemd-nss issue [1] when a
--- password database is traversed first time.
---
--- [1]: https://github.com/systemd/systemd/issues/9585
-pcall(getpwall)
-
 return {
     getpw = getpw,
     getgr = getgr,
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 3/3] lua/pwd: workaround the systemd bug

[Cyrill Gorcunov]

On Thu, Sep 17, 2020 at 04:51:21PM +0300, Cyrill Gorcunov wrote:
...
>  
> +--
> +-- systemd v209 sets errno is set to ENOENT in
> +-- _nss_systemd_getpwent_r and _nss_systemd_getgrent_r
> +-- when there are no more entries to enumerate.

There is a typo in comment ("is set" phrase). Updated to

+--
+-- systemd v209 sets errno to ENOENT in
+-- _nss_systemd_getpwent_r and _nss_systemd_getgrent_r
+-- when there are no more entries left to enumerate.
+--
+-- This is a bug which has been fixed later in
+-- systemd code but we have to deal with buggy
+-- environment thus ignore such error if appears.
+--
+-- See the reference for details
+-- https://github.com/systemd/systemd/issues/9585
+--
+-- The issue affects getpwent/getgrent calls only
+-- thus provide own wrappings to keep workaround
+-- in one place and do not affect any other calls
+-- where ENOENT might become a valid error case.
+--
+-- Initially we observed this issue on Fedora 29
+-- when a password database is traversed for the
+-- first time.

and force pushed.

Re: [Tarantool-patches] [PATCH 0/3] lua/pwd: workaround the systemd bug

[Kirill Yukhin]

Hello,

On 17 сен 16:51, Cyrill Gorcunov wrote:
> Guys, take a look please once time permit. The first two patches
> are optional but I think while we're at this code and we could
> improve the code base we should.
> 
> Also I've been thinking on how to test the issue -- we need a buggy
> systemd instance running and we actually have Fedora 29 on gitlab
> where tests are passed but it is unclear for me how exactly we could
> use it. Initially I thought of injecting some error code into pwd module
> itself but then I left this idea of making code more ugly just because
> of some bug in the particular systemd version.
> 
> So my criteria was that the change must not break anything in existing
> tests but if you have some idea for testing, please share.
> 
> issue https://github.com/tarantool/tarantool/issues/5034
> branch gorcunov/gh-5034-passwd

LGTM.
I've checked your patch into 1.10, 2.4, 2.5 and master.

Could you please update release notes since I see no entry in cover letter.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 0/3] lua/pwd: workaround the systemd bug

[Cyrill Gorcunov]

On Fri, Sep 18, 2020 at 03:26:29PM +0300, Kirill Yukhin wrote:
> 
> LGTM.
> I've checked your patch into 1.10, 2.4, 2.5 and master.
> 
> Could you please update release notes since I see no entry in cover letter.

Sure, will do.

Re: [Tarantool-patches] [PATCH 0/3] lua/pwd: workaround the systemd bug

[Alexander Turenko]

Thanks! This was the annoying issue.

> Also I've been thinking on how to test the issue -- we need a buggy
> systemd instance running and we actually have Fedora 29 on gitlab
> where tests are passed but it is unclear for me how exactly we could
> use it. Initially I thought of injecting some error code into pwd module
> itself but then I left this idea of making code more ugly just because
> of some bug in the particular systemd version.

The only choice I see is to ssh into some of our machine, where
integration with large LDAP database is enabled, find how to drop a
local cache of the database and test manually that freezes are gone.
However I guess the change is straightforward enough to just push it.