This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

use initgroups_dyn-less nss impls in a thread-safer way


AFAICT, nscd may call getgrouplist() from multiple threads
simultaneously.

This is fine for all (?) nss implementations provided by glibc itself,
for they supply a thread-safe implementation of the inigroups_dyn entry
point.

However, for other nss impementations that don't, as well as for older
versions of glibc that didn't have _nss_files_initgroups_dyn, this means
nscd threads may interfere with one another as they iterate over the
group list with the not-really-reentrant getgrent_r, so that they get
non-overlapping subsets of the group entries.

I realize this might not be the best place for this fix; arguably, nscd
should avoid concurrency issues when using such not-really-reentrant
interfaces like any other program, but since its whole point is to
improve performance and holding an unnecessary lock around every
getgrouplist call would hurt it, I propose a change that will have a
minimal impact that may be easily avoided by introducing an
initgroups_dyn entry point, and that will increase the odds of
correctness for general uses that don't take care of locking
getgrouplist() themselves (as long as they don't getgrent*
concurrently).

Is this ok?

for  ChangeLog
2012-01-19  Alexandre Oliva  <aoliva@redhat.com>

	* grp/compat-initgroups.c (compat_call): Avoid concurrency
	between setgrent and endgrent.

--- grp/compat-initgroups.c
+++ grp/compat-initgroups.c
@@ -1,3 +1,5 @@
+#include <bits/libc-lock.h>
+
 /* Prototype for the setgrent functions we use here.  */
 typedef enum nss_status (*set_function) (void);
 
@@ -20,17 +22,22 @@ compat_call (service_user *nip, const char *user, gid_t group, long int *start,
   get_function getgrent_fct;
   end_function endgrent_fct;
   gid_t *groups = *groupsp;
+  __libc_lock_define_initialized(static, compat_lock);
 
   getgrent_fct = __nss_lookup_function (nip, "getgrent_r");
   if (getgrent_fct == NULL)
     return NSS_STATUS_UNAVAIL;
 
   setgrent_fct = __nss_lookup_function (nip, "setgrent");
+  __libc_lock_lock (compat_lock);
   if (setgrent_fct)
     {
       status = DL_CALL_FCT (setgrent_fct, ());
       if (status != NSS_STATUS_SUCCESS)
-	return status;
+	{
+	  __libc_lock_unlock (compat_lock);
+	  return status;
+	}
     }
 
   endgrent_fct = __nss_lookup_function (nip, "endgrent");
@@ -122,5 +129,7 @@ compat_call (service_user *nip, const char *user, gid_t group, long int *start,
   if (endgrent_fct)
     DL_CALL_FCT (endgrent_fct, ());
 
+  __libc_lock_unlock (compat_lock);
+
   return result;
 }
-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]