Discussion:
[SSSD] [sssd PR#5541][opened] nss client: make innetgr() thread safe
sumit-bose
2021-03-18 08:25:58 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Author: sumit-bose
Title: #5541: nss client: make innetgr() thread safe
Action: opened

PR body:
"""
The innetgr() call is expected to be thread safe but SSSD's the current
implementation isn't. In glibc innetgr() is implementend by calling the
setnetgrent(), getnetgrent(), endgrent() sequence with a private context
(struct __netgrent) with provides a member where NSS modules can store data
between the calls.

With this patch setnetgrent() will read all required data from the NSS
responder and store it in the data member of the __netgrent struct.
Upcoming getnetgrent() calls will only operate on the stored data and not
connect to the NSS responder anymore. endgrent() will free the data. Since
the netgroup data is read in a single request to the NSS responder
protected by a mutex and stored in private context of innetgr() this call
is now thread-safe.

Resolves: https://github.com/SSSD/sssd/issues/5540
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5541/head:pr5541
git checkout pr5541
sumit-bose
2021-03-18 08:35:34 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

sumit-bose commented:
"""
Hi,

I have send two different implementations, the second is #5542.

Each has its pros and cons. This one requires more memory since all data is read in a single run and kept in the client process between the calls. This is more or less the same as libnss_files.so does, the related data from /etc/netgroup is read by setnetgrent() and processed later.

The second one uses a new connection for each request sequences. Since the socket has to be opened and the connection established this will require a bit more time. My main concern here is that the file descriptor might get lost and the connection stays open if the caller does not implement the setnetgrent(), getentgrent(), endgrent() sequence correctly or calls them in threads because the sequence itself is not thread-safe, only the implementation with innetgr().

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-801735204
alexey-tikhonov
2021-03-29 18:38:35 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

alexey-tikhonov commented:
"""
In general, I prefer this solution as more clear and simple. See https://github.com/SSSD/sssd/pull/5542#issuecomment-809590594
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-809617868
sumit-bose
2021-04-16 17:13:42 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Author: sumit-bose
Title: #5541: nss client: make innetgr() thread safe
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5541/head:pr5541
git checkout pr5541
sumit-bose
2021-04-16 17:15:13 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

sumit-bose commented:
"""
Hi,

thank you for the review. I set `enum_limit` to `0` and added a comment which explains that it is not used for this specific call.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-821319735
sumit-bose
2021-04-16 17:46:26 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Author: sumit-bose
Title: #5541: nss client: make innetgr() thread safe
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5541/head:pr5541
git checkout pr5541
alexey-tikhonov
2021-04-19 14:38:40 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

alexey-tikhonov commented:
"""
Sorry I missed test code last time (I have a couple of minor comments in its regard.)
Main patch looks good to me now (but Pavel self-assigned so probably wants to take a look as well).
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-822518951
sumit-bose
2021-04-20 09:40:25 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Author: sumit-bose
Title: #5541: nss client: make innetgr() thread safe
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5541/head:pr5541
git checkout pr5541
sumit-bose
2021-04-20 09:41:33 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

sumit-bose commented:
"""
Hi,

the latest version fixes the comments for the test program as well and adds a GPLv3 header to the file.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823136621
alexey-tikhonov
2021-04-20 12:45:20 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

alexey-tikhonov commented:
"""
Hi, a couple of other remarks.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823243602
alexey-tikhonov
2021-04-20 12:45:32 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

Label: +Changes requested
sumit-bose
2021-04-20 13:36:56 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Author: sumit-bose
Title: #5541: nss client: make innetgr() thread safe
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5541/head:pr5541
git checkout pr5541
alexey-tikhonov
2021-04-20 15:42:28 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

alexey-tikhonov commented:
"""
Thank you. ACK from my side.

@pbrezina, what is your opinion (in general and on "this PR vs #5542")?
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823377946
alexey-tikhonov
2021-04-20 15:42:31 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

Label: -Changes requested
pbrezina
2021-04-21 09:36:57 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

pbrezina commented:
"""
I'm fine with this. Ack.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823925310
pbrezina
2021-04-21 09:37:16 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

Label: +Ready to push
pbrezina
2021-04-21 09:37:15 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

Label: +Accepted
pbrezina
2021-04-21 10:02:09 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

pbrezina commented:
"""
Pushed PR: https://github.com/SSSD/sssd/pull/5541

* `master`
* 29abf94e3aec2c54c76adf61318099097c41ea77 - intg test: test is innetgr() is thread-safe
* 88eec1c22f188adc49f41b81fe5af03c995c690d - nss client: make innetgr() thread safe

"""

See the full comment at https://github.com/SSSD/sssd/pull/5541#issuecomment-823940736
pbrezina
2021-04-21 10:02:15 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

Label: -Ready to push
pbrezina
2021-04-21 10:02:20 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

Label: +Pushed
pbrezina
2021-04-21 10:02:12 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Title: #5541: nss client: make innetgr() thread safe

Label: -Accepted
pbrezina
2021-04-21 10:02:17 UTC
Permalink
URL: https://github.com/SSSD/sssd/pull/5541
Author: sumit-bose
Title: #5541: nss client: make innetgr() thread safe
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/5541/head:pr5541
git checkout pr5541

Loading...