[issue7900] posix.getgroups() failure on Mac OS X

[Alexander Belopolsky]

Alexander Belopolsky <alexander.belopolsky at gmail.com> added the comment:

On Tue, Feb 23, 2010 at 2:20 AM, Ronald Oussoren <report at bugs.python.org> wrote:
> ..
> Alexander: Â What makes you think r63955 introduced the problem?

That revision introduced _DARWIN_C_SOURCE which, as I explained, has
two effects on get/setgroups:

"""
It turns out that defining _DARWIN_C_SOURCE not only allows
getgroups() output to exceed NGROUPS_MAX (as documented), but also
effectively disables setgroups() which is not documented.
"""

I further added:

"""
In order to have both working os.setgroups() and os.getgroups()
supporting more than NGROUPS_MAX results, it appears that the two
functions should be compiled in separate compilation units which is
probably too big of a price to pay for the functionality.
"""

I agree that this is a important distinction, but I seem to recall
discussion on python-dev or elsewhere on the tracker that concluded
that unexpected exceptions can be classified as crashes.  For most
users the difference between hard and soft crash is purely academic -
in either case their python program terminates due to a bug in python
and the "type" menu does not have separate entries for hard and soft
crashes.

> The Apple fix for getgroups in python2.6 is odd, it uses an undocumented API
> (getgrouplist_2).

I agree, Apple patch is not acceptable.  Not only it is undocumented,
but I suspect it is simply wrong because getgrouplist_2 is likely to
be a variant of getgrouplist that handles memory allocation and
returns group list from system's group database rather than from
per-process setting. (I have not verified that point, though.)

>
> If I read the manpage correctly there is a posixly correct way to implement os.getgroups:
>
> * call getgroups(MAX_GROUPS,...)

Note that MAX_GROUPS is 65536 on Linux and may be even higher on other
systems.  Allocating 256K on heap is wasteful and allocating the same
on the stack may even hard crash python.

> * if that fails: call getgroups(0,...), the result is groupcount

Please note the following comment I found in GNU coreutils source code:

/* On at least Ultrix 4.3 and NextStep 3.2, getgroups (0, NULL) always
   fails.  On other systems, it returns the number of supplemental
   groups for the process.  This function handles that special case
   and lets the system-provided function handle all others.  However,
   it can fail with ENOMEM if memory is tight.  It is unspecified
   whether the effective group id is included in the list.  */

This is more or less what my original patch did, but it suffers from a
race condition.

I am attaching a patch that I wrote yesterday, but did not submit
because it does not fix setgroups.  I am attaching it now so that we
can compare our approaches.

----------
Added file: http://bugs.python.org/file16338/issue7900-1.diff

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue7900>
_______________________________________
-------------- next part --------------
Index: Modules/posixmodule.c
===================================================================
--- Modules/posixmodule.c	(revision 78265)
+++ Modules/posixmodule.c	(working copy)
@@ -3833,17 +3833,20 @@
 posix_getgroups(PyObject *self, PyObject *noargs)
 {
     PyObject *result = NULL;
+        gid_t *grouplist;
+        int n, nbuf;
 
-#ifdef NGROUPS_MAX
-#define MAX_GROUPS NGROUPS_MAX
-#else
-        /* defined to be 16 on Solaris7, so this should be a small number */
-#define MAX_GROUPS 64
-#endif
-        gid_t grouplist[MAX_GROUPS];
-        int n;
-
-        n = getgroups(MAX_GROUPS, grouplist);
+	nbuf = 16;
+	while (1) {
+	    grouplist = PyMem_MALLOC(nbuf * sizeof(gid_t));
+	    if (grouplist == NULL)
+	        return PyErr_NoMemory();
+	    n = getgroups(nbuf, grouplist);
+	    if (n > 0 || errno != EINVAL)
+		break;
+	    PyMem_FREE(grouplist);
+	    nbuf *= 2;
+	}
         if (n < 0)
             posix_error();
         else {
@@ -3861,6 +3864,7 @@
                 }
             }
         }
+	PyMem_FREE(grouplist);
 
     return result;
 }
@@ -5713,35 +5717,35 @@
 posix_setgroups(PyObject *self, PyObject *groups)
 {
 	int i, len;
-        gid_t grouplist[MAX_GROUPS];
+        gid_t *grouplist;
+	PyObject *ret = NULL;
 
 	if (!PySequence_Check(groups)) {
 		PyErr_SetString(PyExc_TypeError, "setgroups argument must be a sequence");
 		return NULL;
 	}
 	len = PySequence_Size(groups);
-	if (len > MAX_GROUPS) {
-		PyErr_SetString(PyExc_ValueError, "too many groups");
-		return NULL;
-	}
+	grouplist = PyMem_MALLOC(len * sizeof(gid_t));
+	if (grouplist == NULL)
+	        return PyErr_NoMemory();
 	for(i = 0; i < len; i++) {
 		PyObject *elem;
 		elem = PySequence_GetItem(groups, i);
 		if (!elem)
-			return NULL;
+			goto done;
 		if (!PyInt_Check(elem)) {
 			if (!PyLong_Check(elem)) {
 				PyErr_SetString(PyExc_TypeError,
 						"groups must be integers");
 				Py_DECREF(elem);
-				return NULL;
+				goto done;
 			} else {
 				unsigned long x = PyLong_AsUnsignedLong(elem);
 				if (PyErr_Occurred()) {
 					PyErr_SetString(PyExc_TypeError, 
 							"group id too big");
 					Py_DECREF(elem);
-					return NULL;
+					goto done;
 				}
 				grouplist[i] = x;
 				/* read back to see if it fits in gid_t */
@@ -5749,7 +5753,7 @@
 					PyErr_SetString(PyExc_TypeError,
 							"group id too big");
 					Py_DECREF(elem);
-					return NULL;
+					goto done;
 				}
 			}
 		} else {
@@ -5759,7 +5763,7 @@
 				PyErr_SetString(PyExc_TypeError,
 						"group id too big");
 				Py_DECREF(elem);
-				return NULL;
+				goto done;
 			}
 		}
 		Py_DECREF(elem);
@@ -5767,8 +5771,11 @@
 
 	if (setgroups(len, grouplist) < 0)
 		return posix_error();
-	Py_INCREF(Py_None);
-	return Py_None;
+	ret = Py_None;
+	Py_INCREF(ret);
+ done:
+	PyMem_FREE(grouplist);
+	return ret;
 }
 #endif /* HAVE_SETGROUPS */