Reworked to eliminate all potential memory problems, including
deletion of object from list argument during callout to fileno().
diff --git a/Modules/selectmodule.c b/Modules/selectmodule.c
index c248848..a1e3cf0 100644
--- a/Modules/selectmodule.c
+++ b/Modules/selectmodule.c
@@ -57,11 +57,26 @@
static PyObject *SelectError;
-typedef struct { /* list of Python objects and their file descriptor */
- PyObject *obj;
+/* list of Python objects and their file descriptor */
+typedef struct {
+ PyObject *obj; /* owned reference */
SOCKET fd;
+ int sentinel; /* -1 == sentinel */
} pylist;
+static void
+reap_obj(fd2obj)
+ pylist fd2obj[FD_SETSIZE + 3];
+{
+ int i;
+ for (i = 0; i < FD_SETSIZE + 3 && fd2obj[i].sentinel >= 0; i++) {
+ Py_XDECREF(fd2obj[i].obj);
+ fd2obj[i].obj = NULL;
+ }
+ fd2obj[0].sentinel = -1;
+}
+
+
/* returns -1 and sets the Python exception if an error occurred, otherwise
returns a number >= 0
*/
@@ -71,67 +86,76 @@
fd_set *set;
pylist fd2obj[FD_SETSIZE + 3];
{
- int i, len, index, max = -1;
- PyObject *o, *fno;
- PyObject *meth;
- SOCKET v;
+ int i;
+ int max = -1;
+ int index = 0;
+ int len = PyList_Size(list);
+ PyObject* o = NULL;
- index = 0;
fd2obj[0].obj = (PyObject*)0; /* set list to zero size */
-
FD_ZERO(set);
- len = PyList_Size(list);
- for (i=0; i<len; i++) {
+
+ for (i = 0; i < len; i++) {
+ PyObject *meth;
+ SOCKET v;
+
+ /* any intervening fileno() calls could decr this refcnt */
o = PyList_GetItem(list, i);
+ Py_INCREF(o);
+
if (PyInt_Check(o)) {
v = PyInt_AsLong(o);
}
else if ((meth = PyObject_GetAttrString(o, "fileno")) != NULL)
{
- fno = PyEval_CallObject(meth, NULL);
+ PyObject *fno = PyEval_CallObject(meth, NULL);
Py_DECREF(meth);
if (fno == NULL)
- return -1;
+ goto finally;
+
if (!PyInt_Check(fno)) {
- PyErr_SetString(PyExc_TypeError,
+ PyErr_SetString(PyExc_TypeError,
"fileno method returned a non-integer");
- Py_DECREF(fno);
- return -1;
+ Py_DECREF(fno);
+ goto finally;
}
v = PyInt_AsLong(fno);
Py_DECREF(fno);
}
else {
PyErr_SetString(PyExc_TypeError,
- "argument must be an int, or have a fileno() method."
- );
- return -1;
+ "argument must be an int, or have a fileno() method.");
+ goto finally;
}
#ifdef _MSC_VER
- max = 0; /* not used for Win32 */
-#else
+ max = 0; /* not used for Win32 */
+#else /* !_MSC_VER */
if (v < 0 || v >= FD_SETSIZE) {
- PyErr_SetString(
- PyExc_ValueError,
- "filedescriptor out of range in select()");
- return -1;
+ PyErr_SetString(PyExc_ValueError,
+ "filedescriptor out of range in select()");
+ goto finally;
}
if (v > max)
max = v;
-#endif
+#endif /* _MSC_VER */
FD_SET(v, set);
+
/* add object and its file descriptor to the list */
if (index >= FD_SETSIZE) {
- PyErr_SetString(
- PyExc_ValueError,
- "too many file descriptors in select()");
- return -1;
+ PyErr_SetString(PyExc_ValueError,
+ "too many file descriptors in select()");
+ goto finally;
}
fd2obj[index].obj = o;
fd2obj[index].fd = v;
- fd2obj[++index].obj = (PyObject *)0; /* sentinel */
+ fd2obj[index].sentinel = 0;
+ fd2obj[++index].sentinel = -1;
}
return max+1;
+
+ finally:
+ Py_XDECREF(o);
+ return -1;
}
/* returns NULL and sets the Python exception if an error occurred */
@@ -140,40 +164,44 @@
fd_set *set;
pylist fd2obj[FD_SETSIZE + 3];
{
- int j, num=0;
+ int i, j, count=0;
PyObject *list, *o;
SOCKET fd;
- for (j=0; fd2obj[j].obj; j++)
+ for (j = 0; fd2obj[j].sentinel >= 0; j++) {
if (FD_ISSET(fd2obj[j].fd, set))
- num++;
-
- list = PyList_New(num);
+ count++;
+ }
+ list = PyList_New(count);
if (!list)
return NULL;
- num = 0;
- for (j=0; fd2obj[j].obj; j++) {
+ i = 0;
+ for (j = 0; fd2obj[j].sentinel >= 0; j++) {
fd = fd2obj[j].fd;
if (FD_ISSET(fd, set)) {
#ifndef _MSC_VER
if (fd > FD_SETSIZE) {
PyErr_SetString(PyExc_SystemError,
"filedescriptor out of range returned in select()");
- return NULL;
+ goto finally;
}
#endif
o = fd2obj[j].obj;
- Py_INCREF(o);
- if (PyList_SetItem(list, num, o) < 0) {
- Py_DECREF(list);
- return NULL;
- }
- num++;
+ fd2obj[j].obj = NULL;
+ /* transfer ownership */
+ if (PyList_SetItem(list, i, o) < 0)
+ goto finally;
+
+ i++;
}
}
return list;
+ finally:
+ Py_DECREF(list);
+ return NULL;
}
+
static PyObject *
select_select(self, args)
@@ -184,7 +212,7 @@
pylist wfd2obj[FD_SETSIZE + 3];
pylist efd2obj[FD_SETSIZE + 3];
PyObject *ifdlist, *ofdlist, *efdlist;
- PyObject *ret;
+ PyObject *ret = NULL;
PyObject *tout = Py_None;
fd_set ifdset, ofdset, efdset;
double timeout;
@@ -200,9 +228,11 @@
if (tout == Py_None)
tvp = (struct timeval *)0;
- else if (!PyArg_Parse(tout, "d;timeout must be float or None",
- &timeout))
+ else if (!PyArg_Parse(tout, "d", &timeout)) {
+ PyErr_SetString(PyExc_TypeError,
+ "timeout must be a float or None");
return NULL;
+ }
else {
seconds = (int)timeout;
timeout = timeout - (double)seconds;
@@ -224,12 +254,15 @@
/* Convert lists to fd_sets, and get maximum fd number
* propagates the Python exception set in list2set()
*/
+ rfd2obj[0].sentinel = -1;
+ wfd2obj[0].sentinel = -1;
+ efd2obj[0].sentinel = -1;
if ((imax=list2set(ifdlist, &ifdset, rfd2obj)) < 0)
- return NULL;
+ goto finally;
if ((omax=list2set(ofdlist, &ofdset, wfd2obj)) < 0)
- return NULL;
+ goto finally;
if ((emax=list2set(efdlist, &efdset, efd2obj)) < 0)
- return NULL;
+ goto finally;
max = imax;
if (omax > max) max = omax;
if (emax > max) max = emax;
@@ -240,33 +273,37 @@
if (n < 0) {
PyErr_SetFromErrno(SelectError);
- return NULL;
}
-
- if (n == 0) { /* Speedup hack */
+ else if (n == 0) {
+ /* optimization */
ifdlist = PyList_New(0);
- if (!ifdlist)
- return NULL;
- ret = Py_BuildValue("OOO", ifdlist, ifdlist, ifdlist);
- Py_XDECREF(ifdlist);
- return ret;
+ if (ifdlist) {
+ ret = Py_BuildValue("OOO", ifdlist, ifdlist, ifdlist);
+ Py_DECREF(ifdlist);
+ }
}
+ else {
+ /* any of these three calls can raise an exception. it's more
+ convenient to test for this after all three calls... but
+ is that acceptable?
+ */
+ ifdlist = set2list(&ifdset, rfd2obj);
+ ofdlist = set2list(&ofdset, wfd2obj);
+ efdlist = set2list(&efdset, efd2obj);
+ if (PyErr_Occurred())
+ ret = NULL;
+ else
+ ret = Py_BuildValue("OOO", ifdlist, ofdlist, efdlist);
- /* any of these three calls can raise an exception. it's more
- convenient to test for this after all three calls... but is that
- acceptable?
- */
- ifdlist = set2list(&ifdset, rfd2obj);
- ofdlist = set2list(&ofdset, wfd2obj);
- efdlist = set2list(&efdset, efd2obj);
- if (PyErr_Occurred())
- ret = NULL;
- else
- ret = Py_BuildValue("OOO", ifdlist, ofdlist, efdlist);
-
- Py_DECREF(ifdlist);
- Py_DECREF(ofdlist);
- Py_DECREF(efdlist);
+ Py_DECREF(ifdlist);
+ Py_DECREF(ofdlist);
+ Py_DECREF(efdlist);
+ }
+
+ finally:
+ reap_obj(rfd2obj);
+ reap_obj(wfd2obj);
+ reap_obj(efd2obj);
return ret;
}