Instead of using mutexes and blocking other requests and risking dealocks, wouldn't it be easier to remove the global file descriptor and using a local one?

If I understand the client code correctly as-is, the problem is that the calls into NSS from glibc don't carry a state object that we can rely on. We have no way of knowing whether two sequential calls to e.g. {{{setpwent()}}} are coming from the same or different threads. So in that case, locking the global context is the only way to maintain thread-safety.

The problem I see with this (this is mainly for enumerations) is that we need to lock in {{{setpwent()}}} (explicit or implicit) and unlock in {{{getpwent()}}}. If a badly-written program fails to call {{{getpwent()}}}, we'll be in a deadlock situation for that process. Also, enumerations can take a long time and we need to hold the lock for the entire duration or else we could end up with incorrect results.

Replying to [comment:2 sgallagh]:

If I understand the client code correctly as-is, the problem is that the calls into NSS from glibc don't carry a state object that we can rely on. We have no way of knowing whether two sequential calls to e.g. {{{setpwent()}}} are coming from the same or different threads. So in that case, locking the global context is the only way to maintain thread-safety.

We could use thread local storage if we really needed to, but the point is that the interface is explicitly thread agnostic. If we allowed indipendent query by threads we would behave differently from the rest of nsswitch. An application written with the knowledge that 2 threads doing getpwent() will get answers from the same enumeration would break if, instead we started 2 separate enumerations.
We do not want to change semantics here, just avoid one thread stomping over another. (for example by reading out part of the buffer from fd with a getpwnam() while the other thread is doing a getpwent().

The problem I see with this (this is mainly for enumerations) is that we need to lock in {{{setpwent()}}} (explicit or implicit) and unlock in {{{getpwent()}}}. If a badly-written program fails to call {{{getpwent()}}}, we'll be in a deadlock situation for that process. Also, enumerations can take a long time and we need to hold the lock for the entire duration or else we could end up with incorrect results.

We just need mutexes around a single operation, spanning multiple operations may indeed lead to deadlocks and is really not necessary. Normally the mutex starts the moments we transmit something over the fd, and ends as soon as we got the last byte of the result we were waiting for on the fd.
For getpwent() we also need a mutex the moment we access data, because we cache results in advance in order to reduce the number of roundtrips, so multiple threads are going to access the same data and advance the same pointers into it.

Fields changed

milestone: NEEDS_TRIAGE => SSSD 1.6.0

Fields changed

milestone: SSSD 1.6.0 => SSSD 1.5.0
owner: somebody => simo
status: new => assigned

Fields changed

resolution: => fixed
status: assigned => closed

Fixed by c89589f

Fields changed

rhbz: => 0

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/1682

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.