[SSSD] [PATCH] Fix client libs thread safety

Simo Sorce ssorce at redhat.com
Mon Nov 22 13:25:16 UTC 2010


On Mon, 22 Nov 2010 10:46:02 +0100
Sumit Bose <sbose at redhat.com> wrote:

> On Sat, Nov 20, 2010 at 03:33:31PM -0500, Simo Sorce wrote:
> > 
> > Had some spare time today and wanted to fix this issue.
> > 
> > The attached patch instruments nss and pam clients to use a pthread
> > mutex to prevent multiple threads from stomping on each other.
> > 
> > The patch is quite simple and basic testing seem to show no issues.
> > It may be worth of back-porting to older versions.
> 
> I think the mutexes cover too much code here, especially for PAM.

I know they cover much, but as a first patch I think it is safer.
We can always better optimize. Remember that for single threaded apps
(or apps that do not use these interfaces from multiple threads)
nothing really changes.

> The
> PAM client does not use any global variables except the file handle to
> to PAM responder of sssd. I would be enough to just protect the I/O to
> sssd with the mutex to avoid other threads getting the response of a
> request from a different thread. I here I would suggest that we reduce
> the socket timeout considerably to avoid long delays if a request gets
> stuck in sssd. We might be able to avoid mutexes here completely by
> introducing request ids, but this can be seen as an enhancement for a
> future version.

No you can't, The mutx is always needed because otherwise you risk
reading half of the reply from one thread and another half from another
breaking communication entirely.

> Protecting a whole PAM task like authentication with a mutex can lead
> to DOS situation when a user logs into a threaded application with his
> username but "forgets" to enter his password immediately because the
> phone is ringing.

Is this better or worse than just breaking if 2 users authenticate at
the same time from 2 separate threads ?

> For NSS it might also be sufficient to protect the I/O to sssd.

It might, I would consider it a performance enhancement though.

> Netgroups do not use a global variable to save the context and for
> passwd and groups I think the global variables are not really
> protected. E.g. while one thread is running a loop with getpwent()
> calls a second thread can sneak in between in the calls and call
> setpwent() and resets the global variable.

setpwent() is mutex protected too. This way if another thread tries to
reset the memory cache, it will have to wait until the getpwent() cache
is done with it. Remember, sss_net_getpwent_data is a global variable
shared between all threads and needs protection.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York



More information about the sssd-devel mailing list