Bug 5232 - getpwent_r, getpwuid_r, getpwnam_r and getgrgid_r not reentrant.
getpwent_r, getpwuid_r, getpwnam_r and getgrgid_r not reentrant.
Status: NEW
Product: Samba 3.0
Classification: Unclassified
Component: winbind
3.0.25b
All Linux
: P3 critical
: none
Assigned To: Samba Bugzilla Account
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-29 11:38 UTC by Dominique Leroux
Modified: 2008-01-29 12:31 UTC (History)
0 users

See Also:


Attachments
Pass a path to the program and it will try to get the owner of each file from multiple threads (4.47 KB, text/plain)
2008-01-29 11:55 UTC, Dominique Leroux
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dominique Leroux 2008-01-29 11:38:47 UTC
The linux implementation for these functions for winbind, in source/nssswitch/winbind_nss_linux.c, contains static variables.  In a multi-threaded context, the statics get overwritten by the various threads and this invariably leads to a crash.  The associated winbind entry points are:

_nss_winbind_getpwent_r
_nss_winbind_getpwuid_r
_nss_windbind_getpwnam_r
_nss_winbind_getgrgid_r

It looks like the implementation assumes that it will only ever be called by the glibc implementation of the non-reentrant versions (i.e. the ones without the _r suffix).  glibc will call the _r version repeatedly, increasing the buffer size until there are no more ERANGE errors.  But the entry points can actually be called directly when the calling program uses the _r versions (_r meaning "reentrant") and the sequence of events in this situation is completely different and unpredictable.

In short, there should be no static variables in there.  I understand the point of caching the value of the request to avoid asking multiple times, but if this must absolutely be done, then the linux implementation should define its own entry points for the non-reentrant versions getpwent, getpwuid and so on.  It would then perform this optimization locally without interfering with the reentrant versions.  Alternatively, a more evolved implementation of a thread-safe list of "in-processing requests" could be done, I'm just not sure of the benefit.  Programs should use sysconf( _SC_GETPW_R_SIZE_MAX ) to find out about the needed buffer size instead of relying on this try-and-error strategy.

I have also looked at versions 3.0.10, 3.0.28 and 4.0.0alpha2 and the same implementation problem appears in there.  I have written a simple multi-threaded program that proves this for getpwuid_r (which is my actual problem: I only found about the other calls by inspecting the code).  I can supply it if needed.

I will have to serialize all accesses to these functions in my program for the time being, but I don't believe there is a user-level workaround for this.
Comment 1 Gerald (Jerry) Carter 2008-01-29 11:42:52 UTC
Please check the 3.2 code base as this should have already been addressed.
Comment 2 Dominique Leroux 2008-01-29 11:55:02 UTC
Created attachment 3122 [details]
Pass a path to the program and it will try to get the owner of each file from multiple threads

Compile with: g++ -o nssTest nssTest.cpp -lpthread

This uses getpwuid_r to get all files owner's name.  There will be as many threads as there are files in the directory passed in, so avoid directories with millions of files..

Use '-s' as the first parameter to serialize accesses to getpwuid_r.
Comment 3 Dominique Leroux 2008-01-29 12:15:04 UTC
(In reply to comment #1)
> Please check the 3.2 code base as this should have already been addressed.
> 

I have just checked in the subversion repository, and it has not been fixed in the branch SAMBA_3_2_RELEASE.  I have even found one more offending entry point that I had missed the first time: getgrnam_r.  Any access to static data in this file should be done under the protection of a mutex (if the implementation with the static globals getpwent_response, ndx_pw_cache and num_pw_cache is to be kept).  As mentioned in the initial report, I have also checked 4.0.0alpha 2.
Comment 4 Gerald (Jerry) Carter 2008-01-29 12:19:32 UTC
We don't use svn anymore.  Please read http://wiki.samba.org/index.php/Using_Git_for_Samba_Development
Comment 5 Dominique Leroux 2008-01-29 12:27:09 UTC
(In reply to comment #1)
> Please check the 3.2 code base as this should have already been addressed.

I have just checked in SAMBA_3_2 (as opposed to SAMBA_3_2_RELEASE, as in my previous post) and you are right: mutexes have been introduce to serialize access to static data.  Sorry about the confusion.

I leave it up to you to close the bug as I'm not sure how it works: it's still broken for 3.0 stream and don't know if this fix is going to be backported (at which point it would become fixed).

Comment 6 Dominique Leroux 2008-01-29 12:31:05 UTC
(In reply to comment #4)
> We don't use svn anymore.  Please read
> http://wiki.samba.org/index.php/Using_Git_for_Samba_Development

OK, thanks for the info.  Still, svn worked for me (and I was pointed there by http://us3.samba.org/samba/subversion.html)