[SSSD] [sssd PR#5542][opened] nss client: make innetgr() thread safe

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5542
Author: sumit-bose
Title: #5542: 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 open a new connection to the NSS
responder and stores the file descriptor in the data member of
__netgrent struct so that the following getnetgrent() and endgrent() will
use the same connection. Since the NSS responder stores the netgroup
lookups related data in a per connection context and a new thread will open
a new connection the implementation is 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/5542/head:pr5542
git checkout pr5542

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

sumit-bose commented:
"""
Hi,

an alternative implementation can be found in #5541, see https://github.com/SSSD/sssd/pull/5541#issuecomment-801735204 for an explanation about the differences.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-801735953

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

alexey-tikhonov commented:
"""
I was thinking to make [sss_cli_sd](https://github.com/SSSD/sssd/blob/master/src/sss_client/common.c#L68) `thread_local` and get rid of locks completely quite some time ago.

But

My main concern so far was that since we do not close the sockets an application with many threads might end up with many open sockets.

is a fair point.

Still, I think if we go that venue it should be "all or nothing", i.e. individual connection for every thread.
IMO, having this for netgr requests as an exception unjustifiably complicates code. In this sense I prefer #5541 (as it makes code simpler).

And by the way, reading https://man7.org/linux/man-pages/man3/setnetgrent.3.html I'm confused:
it says "innetgr()... is thread-safe."
but
```
if any of the functions setnetgrent(), getnetgrent_r(), **innetgr()**,
getnetgrent(), or endnetgrent() are used in parallel in different
threads of a program, then data races could occur
```
Do those calls use "global" `__netgrent` context (with the exception of `innetgr()`)?

It seems your solution "fd per state" is more robust. IIUC, it handles correctly case:
```
setnetgrent();
innetgr();
getnetgrent_r();
endnetgrent();
```
-- your solution will create own fd and thus sssd_nss context for innetgr(). "Single `thread_local` fd" solution would fail here.
But again, I don't have clear understanding if support of this case is expected.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-809590594

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

alexey-tikhonov commented:
"""

Post by sumit-bose
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

(I guess `endgrent()` is a mistype and should be `endnetgrent()`)

I would appreciate a more detailed description of a race.

IIUC, the issue is that concurrent *netgrent() read and modify the same context in `sssd_nss`?
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-809486328

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

sumit-bose commented:
"""

Wouldn't it be possible to use `thread_local` instead?

Hi,

in general yes, and I was thinking about this as well. I guess you a are thinking of using the `tread_local` file descriptor for other requests as well which do not have a memory context to store the file descriptor? My main concern so far was that since we do not close the sockets an application with many threads might end up with many open sockets.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-809388569

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

pbrezina commented:
"""
Wouldn't it be possible to use `thread_local` instead?
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-809348320

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

alexey-tikhonov commented:
"""
I was thinking to make [sss_cli_sd](https://github.com/SSSD/sssd/blob/master/src/sss_client/common.c#L68) `thread_local` and get rid of locks completely quite some time ago.

But

My main concern so far was that since we do not close the sockets an application with many threads might end up with many open sockets.

is a fair point.

Still, I think if we go that venue it should be "all or nothing", i.e. individual connection for every thread.
IMO, having this for netgr requests as an exception unjustifiably complicates code. In this sense I prefer #5541 (as it makes code simpler).

And by the way, reading https://man7.org/linux/man-pages/man3/setnetgrent.3.html I'm confused:
it says "innetgr()... is thread-safe."
but
```
if any of the functions setnetgrent(), getnetgrent_r(), **innetgr()**,
getnetgrent(), or endnetgrent() are used in parallel in different
threads of a program, then data races could occur
```
Do those calls use "global" `__netgrent` context (with the exception of `innetgr()`)?

It seems your solution "fd per state" is more robust. IIUC, it handles correctly case:
```
setnetgrent();
innetgr();
getnetgrent_r();
endnetgrent();
```
-- your solution will create own fd and thus sssd_nss context for innetgr(). "Single `thread_local` fd" solution would fail here.
But again, I don't have clear understanding if support of this case is expected.
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-809590594

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

sumit-bose commented:
"""

Post by alexey-tikhonov
I was thinking to make [sss_cli_sd](https://github.com/SSSD/sssd/blob/master/src/sss_client/common.c#L68) `thread_local` and get rid of locks completely quite some time ago.
But

My main concern so far was that since we do not close the sockets an application with many threads might end up with many open sockets.

is a fair point.
Still, I think if we go that venue it should be "all or nothing", i.e. individual connection for every thread.
IMO, having this for netgr requests as an exception unjustifiably complicates code. In this sense I prefer #5541 (as it makes code simpler).
it says "innetgr()... is thread-safe."
but
```
if any of the functions setnetgrent(), getnetgrent_r(), **innetgr()**,
getnetgrent(), or endnetgrent() are used in parallel in different
threads of a program, then data races could occur
```
Do those calls use "global" `__netgrent` context (with the exception of `innetgr()`)?

Hi,

yes, only when called from `innetgr()` the `*netgrent*()` calls receive a request specific `__netgrent`, in all other cases it is a global one. So the calls are already not thread-safe in the glibc level.

Post by alexey-tikhonov
```
setnetgrent();
innetgr();
getnetgrent_r();
endnetgrent();
```
-- your solution will create own fd and thus sssd_nss context for innetgr(). "Single `thread_local` fd" solution would fail here.
But again, I don't have clear understanding if support of this case is expected.

At least with libnss_files this would work since the first `setnetgrent()` will load the file content to the global `__netgrent` and `innetgr()` does not overwrite it so it is still available when `getnetgrent_r()` is called. However I would read the sentence

```
In the above table, netgrent in race:netgrent signifies that if any of the functions setnetgrent(), getnetgrent_r(), innetgr(), getnetgrent(), or endnetgrent() are used in parallel in different threads of a program, then data races could occur.
```

from the man page in the sense that a data races can occur in the single thread as well if some of those functions are intermixed in an unexpected way. But there might be different expectations or interpretations.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-818632629

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5542
Author: sumit-bose
Title: #5542: 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/5542/head:pr5542
git checkout pr5542

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/5542
Title: #5542: nss client: make innetgr() thread safe

pbrezina commented:
"""
Closing in favor of https://github.com/SSSD/sssd/pull/5541
"""

See the full comment at https://github.com/SSSD/sssd/pull/5542#issuecomment-823925815