Issue #15301: Parsing fd, uid, and gid parameters for builtins

in Modules/posixmodule.c is now far more robust.

3 files changed, 205 insertions, 81 deletions

diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index 8916305..13ff18f 100644
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -24,6 +24,8 @@ import itertools
 import stat
 import locale
 import codecs
+import decimal
+import fractions
 try:
     import threading
 except ImportError:
@@ -865,6 +867,16 @@ class MakedirTests(unittest.TestCase):
         os.makedirs(path, mode=mode, exist_ok=True)
         os.umask(old_mask)
 
+    def test_chown_uid_gid_arguments_must_be_index(self):
+        stat = os.stat(support.TESTFN)
+        uid = stat.st_uid
+        gid = stat.st_gid
+        for value in (-1.0, -1j, decimal.Decimal(-1), fractions.Fraction(-2, 2)):
+            self.assertRaises(TypeError, os.chown, support.TESTFN, value, gid)
+            self.assertRaises(TypeError, os.chown, support.TESTFN, uid, value)
+        self.assertIsNone(os.chown(support.TESTFN, uid, gid))
+        self.assertIsNone(os.chown(support.TESTFN, -1, -1))
+
     def test_exist_ok_s_isgid_directory(self):
         path = os.path.join(support.TESTFN, 'dir1')
         S_ISGID = stat.S_ISGID
diff --git a/Misc/NEWS b/Misc/NEWS
index 09b7e3a..f9129b6 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -10,8 +10,11 @@ Projected Release date: 2013-09-08
 Core and Builtins
 -----------------
 
+- Issue #15301: Parsing fd, uid, and gid parameters for builtins
+  in Modules/posixmodule.c is now far more robust.
+
 - Issue #18368: PyOS_StdioReadline() no longer leaks memory when realloc()
-  fail
+  fail.
 
 - Issue #17934: Add a clear() method to frame objects, to help clean up
   expensive details (local variables) and break reference cycles.
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index e5d7ae1..5145e3a 100644
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -417,108 +417,213 @@ _PyLong_FromGid(gid_t gid)
 int
 _Py_Uid_Converter(PyObject *obj, void *p)
 {
+    uid_t uid;
+    PyObject *index;
     int overflow;
     long result;
-    if (PyFloat_Check(obj)) {
-        PyErr_SetString(PyExc_TypeError,
-                        "integer argument expected, got float");
+    unsigned long uresult;
+
+    index = PyNumber_Index(obj);
+    if (index == NULL) {
+        PyErr_Format(PyExc_TypeError,
+                     "uid should be integer, not %.200s",
+                     Py_TYPE(obj)->tp_name);
         return 0;
     }
-    result = PyLong_AsLongAndOverflow(obj, &overflow);
-    if (overflow < 0)
-        goto OverflowDown;
-    if (!overflow && result == -1) {
-        /* error or -1 */
-        if (PyErr_Occurred())
-            return 0;
-        *(uid_t *)p = (uid_t)-1;
-    }
-    else {
-        /* unsigned uid_t */
-        unsigned long uresult;
-        if (overflow > 0) {
-            uresult = PyLong_AsUnsignedLong(obj);
-            if (PyErr_Occurred()) {
-                if (PyErr_ExceptionMatches(PyExc_OverflowError))
-                    goto OverflowUp;
-                return 0;
-            }
-            if ((uid_t)uresult == (uid_t)-1)
-                goto OverflowUp;
-        } else {
-            if (result < 0)
-                goto OverflowDown;
-            uresult = result;
+
+    /*
+     * Handling uid_t is complicated for two reasons:
+     *  * Although uid_t is (always?) unsigned, it still
+     *    accepts -1.
+     *  * We don't know its size in advance--it may be
+     *    bigger than an int, or it may be smaller than
+     *    a long.
+     *
+     * So a bit of defensive programming is in order.
+     * Start with interpreting the value passed
+     * in as a signed long and see if it works.
+     */
+
+    result = PyLong_AsLongAndOverflow(index, &overflow);
+
+    if (!overflow) {
+        uid = (uid_t)result;
+
+        if (result == -1) {
+            if (PyErr_Occurred())
+                goto fail;
+            /* It's a legitimate -1, we're done. */
+            goto success;
         }
+
+        /* Any other negative number is disallowed. */
+        if (result < 0)
+            goto underflow;
+
+        /* Ensure the value wasn't truncated. */
         if (sizeof(uid_t) < sizeof(long) &&
-            (unsigned long)(uid_t)uresult != uresult)
-            goto OverflowUp;
-        *(uid_t *)p = (uid_t)uresult;
+            (long)uid != result)
+            goto underflow;
+        goto success;
+    }
+
+    if (overflow < 0)
+        goto underflow;
+
+    /*
+     * Okay, the value overflowed a signed long.  If it
+     * fits in an *unsigned* long, it may still be okay,
+     * as uid_t may be unsigned long on this platform.
+     */
+    uresult = PyLong_AsUnsignedLong(index);
+    if (PyErr_Occurred()) {
+        if (PyErr_ExceptionMatches(PyExc_OverflowError))
+            goto overflow;
+        goto fail;
     }
+
+    uid = (uid_t)uresult;
+
+    /*
+     * If uid == (uid_t)-1, the user actually passed in ULONG_MAX,
+     * but this value would get interpreted as (uid_t)-1  by chown
+     * and its siblings.   That's not what the user meant!  So we
+     * throw an overflow exception instead.   (We already
+     * handled a real -1 with PyLong_AsLongAndOverflow() above.) 
+     */
+    if (uid == (uid_t)-1)
+        goto overflow;
+
+    /* Ensure the value wasn't truncated. */
+    if (sizeof(uid_t) < sizeof(long) &&
+        (unsigned long)uid != uresult)
+        goto overflow;
+    /* fallthrough */
+
+success:
+    Py_DECREF(index);
+    *(uid_t *)p = uid;
     return 1;
 
-OverflowDown:
+underflow:
     PyErr_SetString(PyExc_OverflowError,
-                    "user id is less than minimum");
-    return 0;
+                    "uid is less than minimum");
+    goto fail;
 
-OverflowUp:
+overflow:
     PyErr_SetString(PyExc_OverflowError,
-                    "user id is greater than maximum");
+                    "uid is greater than maximum");
+    /* fallthrough */
+
+fail:
+    Py_DECREF(index);
     return 0;
 }
 
 int
 _Py_Gid_Converter(PyObject *obj, void *p)
 {
+    gid_t gid;
+    PyObject *index;
     int overflow;
     long result;
-    if (PyFloat_Check(obj)) {
-        PyErr_SetString(PyExc_TypeError,
-                        "integer argument expected, got float");
+    unsigned long uresult;
+
+    index = PyNumber_Index(obj);
+    if (index == NULL) {
+        PyErr_Format(PyExc_TypeError,
+                     "gid should be integer, not %.200s",
+                     Py_TYPE(obj)->tp_name);
         return 0;
     }
-    result = PyLong_AsLongAndOverflow(obj, &overflow);
-    if (overflow < 0)
-        goto OverflowDown;
-    if (!overflow && result == -1) {
-        /* error or -1 */
-        if (PyErr_Occurred())
-            return 0;
-        *(gid_t *)p = (gid_t)-1;
-    }
-    else {
-        /* unsigned gid_t */
-        unsigned long uresult;
-        if (overflow > 0) {
-            uresult = PyLong_AsUnsignedLong(obj);
-            if (PyErr_Occurred()) {
-                if (PyErr_ExceptionMatches(PyExc_OverflowError))
-                    goto OverflowUp;
-                return 0;
-            }
-            if ((gid_t)uresult == (gid_t)-1)
-                goto OverflowUp;
-        } else {
-            if (result < 0)
-                goto OverflowDown;
-            uresult = result;
+
+    /*
+     * Handling gid_t is complicated for two reasons:
+     *  * Although gid_t is (always?) unsigned, it still
+     *    accepts -1.
+     *  * We don't know its size in advance--it may be
+     *    bigger than an int, or it may be smaller than
+     *    a long.
+     *
+     * So a bit of defensive programming is in order.
+     * Start with interpreting the value passed
+     * in as a signed long and see if it works.
+     */
+
+    result = PyLong_AsLongAndOverflow(index, &overflow);
+
+    if (!overflow) {
+        gid = (gid_t)result;
+
+        if (result == -1) {
+            if (PyErr_Occurred())
+                goto fail;
+            /* It's a legitimate -1, we're done. */
+            goto success;
+        }
+
+        /* Any other negative number is disallowed. */
+        if (result < 0) {
+            goto underflow;
         }
+
+        /* Ensure the value wasn't truncated. */
         if (sizeof(gid_t) < sizeof(long) &&
-            (unsigned long)(gid_t)uresult != uresult)
-            goto OverflowUp;
-        *(gid_t *)p = (gid_t)uresult;
+            (long)gid != result)
+            goto underflow;
+        goto success;
+    }
+
+    if (overflow < 0)
+        goto underflow;
+
+    /*
+     * Okay, the value overflowed a signed long.  If it
+     * fits in an *unsigned* long, it may still be okay,
+     * as gid_t may be unsigned long on this platform.
+     */
+    uresult = PyLong_AsUnsignedLong(index);
+    if (PyErr_Occurred()) {
+        if (PyErr_ExceptionMatches(PyExc_OverflowError))
+            goto overflow;
+        goto fail;
     }
+
+    gid = (gid_t)uresult;
+
+    /*
+     * If gid == (gid_t)-1, the user actually passed in ULONG_MAX,
+     * but this value would get interpreted as (gid_t)-1  by chown
+     * and its siblings.   That's not what the user meant!  So we
+     * throw an overflow exception instead.   (We already
+     * handled a real -1 with PyLong_AsLongAndOverflow() above.) 
+     */
+    if (gid == (gid_t)-1)
+        goto overflow;
+
+    /* Ensure the value wasn't truncated. */
+    if (sizeof(gid_t) < sizeof(long) &&
+        (unsigned long)gid != uresult)
+        goto overflow;
+    /* fallthrough */
+
+success:
+    Py_DECREF(index);
+    *(gid_t *)p = gid;
     return 1;
 
-OverflowDown:
+underflow:
     PyErr_SetString(PyExc_OverflowError,
-                    "group id is less than minimum");
-    return 0;
+                    "gid is less than minimum");
+    goto fail;
 
-OverflowUp:
+overflow:
     PyErr_SetString(PyExc_OverflowError,
-                    "group id is greater than maximum");
+                    "gid is greater than maximum");
+    /* fallthrough */
+
+fail:
+    Py_DECREF(index);
     return 0;
 }
 #endif /* MS_WINDOWS */
@@ -541,25 +646,29 @@ static int
 _fd_converter(PyObject *o, int *p, const char *allowed)
 {
     int overflow;
-    long long_value = PyLong_AsLongAndOverflow(o, &overflow);
-    if (PyFloat_Check(o) ||
-        (long_value == -1 && !overflow && PyErr_Occurred())) {
-        PyErr_Clear();
+    long long_value;
+
+    PyObject *index = PyNumber_Index(o);
+    if (index == NULL) {
         PyErr_Format(PyExc_TypeError,
-                        "argument should be %s, not %.200s",
-                        allowed, Py_TYPE(o)->tp_name);
+                     "argument should be %s, not %.200s",
+                     allowed, Py_TYPE(o)->tp_name);
         return 0;
     }
+
+    long_value = PyLong_AsLongAndOverflow(index, &overflow);
+    Py_DECREF(index);
     if (overflow > 0 || long_value > INT_MAX) {
         PyErr_SetString(PyExc_OverflowError,
-                        "signed integer is greater than maximum");
+                        "fd is greater than maximum");
         return 0;
     }
     if (overflow < 0 || long_value < INT_MIN) {
         PyErr_SetString(PyExc_OverflowError,
-                        "signed integer is less than minimum");
+                        "fd is less than minimum");
         return 0;
     }
+
     *p = (int)long_value;
     return 1;
 }