Prevent ioctl op codes from being sign extended from int to unsigned long

when used on platforms that actually define ioctl as taking an unsigned long.
(the BSDs and OS X / Darwin)

Adds a unittest for fcntl.ioctl that tests what happens with both positive and
negative numbers.

This was done because of issue1471 but I'm not able to reproduce -that- problem
in the first place on Linux 32bit or 64bit or OS X 10.4 & 10.5 32bit or 64 bit.

diff --git a/Doc/library/fcntl.rst b/Doc/library/fcntl.rst
index 5050a7f48783e18b6a9729897d4abd2183c0b882..b3b977f9345d9223b48fe5169e0582c8b45de5d8 100644 (file)
--- a/Doc/library/fcntl.rst
+++ b/Doc/library/fcntl.rst
@@ -50,6 +50,8 @@ The module defines the following functions:
    operations are typically defined in the library module :mod:`termios` and the
    argument handling is even more complicated.
 
+   The op parameter is limited to values that can fit in 32-bits.
+
    The parameter *arg* can be one of an integer, absent (treated identically to the
    integer ``0``), an object supporting the read-only buffer interface (most likely
    a plain Python string) or an object supporting the read-write buffer interface.

diff --git a/Lib/test/test_fcntl.py b/Lib/test/test_fcntl.py
index feed870ebee12e9e3bdf6fcf48a7bf73fdd076e1..08b59ec26df3862ddb3c4ad42504ed8e0882704b 100755 (executable)
--- a/Lib/test/test_fcntl.py
+++ b/Lib/test/test_fcntl.py
@@ -5,12 +5,18 @@ OS/2+EMX doesn't support the file locking operations.
 """
 import struct
 import fcntl
-import os, sys
+import os
+import struct
+import sys
 import unittest
 from test.test_support import verbose, TESTFN, unlink, run_unittest
 
 # TODO - Write tests for ioctl(), flock() and lockf().
 
+try:
+    import termios
+except ImportError:
+    termios = None
 
 def get_lockdata():
     if sys.platform.startswith('atheos'):
@@ -82,8 +88,29 @@ class TestFcntl(unittest.TestCase):
         self.f.close()
 
 
+class TestIoctl(unittest.TestCase):
+    if termios:
+        def test_ioctl_signed_unsigned_code_param(self):
+            if termios.TIOCSWINSZ < 0:
+                set_winsz_opcode_maybe_neg = termios.TIOCSWINSZ
+                set_winsz_opcode_pos = termios.TIOCSWINSZ & 0xffffffffL
+            else:
+                set_winsz_opcode_pos = termios.TIOCSWINSZ
+                set_winsz_opcode_maybe_neg, = struct.unpack("i",
+                        struct.pack("I", termios.TIOCSWINSZ))
+
+            # We're just testing that these calls do not raise exceptions.
+            saved_winsz = fcntl.ioctl(0, termios.TIOCGWINSZ, "\0"*8)
+            our_winsz = struct.pack("HHHH",80,25,0,0)
+            # test both with a positive and potentially negative ioctl code
+            new_winsz = fcntl.ioctl(0, set_winsz_opcode_pos, our_winsz)
+            new_winsz = fcntl.ioctl(0, set_winsz_opcode_maybe_neg, our_winsz)
+            fcntl.ioctl(0, set_winsz_opcode_maybe_neg, saved_winsz)
+
+
 def test_main():
     run_unittest(TestFcntl)
+    run_unittest(TestIoctl)
 
 if __name__ == '__main__':
     test_main()

diff --git a/Modules/fcntlmodule.c b/Modules/fcntlmodule.c
index 1e4a254fd174449504ff6e22fef30c1aee6529f5..96a6cc884bc9e9aa7b8d4fab9a75064d140e2a12 100644 (file)
--- a/Modules/fcntlmodule.c
+++ b/Modules/fcntlmodule.c
@@ -97,11 +97,20 @@ fcntl_ioctl(PyObject *self, PyObject *args)
 {
 #define IOCTL_BUFSZ 1024
        int fd;
-       /* In PyArg_ParseTuple below, use the unsigned int 'I' format for
-          the signed int 'code' variable, because Python turns 0x8000000
-          into a large positive number (PyLong, or PyInt on 64-bit
-          platforms,) whereas C expects it to be a negative int */
-       int code;
+       /* In PyArg_ParseTuple below, we use the unsigned non-checked 'I'
+          format for the 'code' parameter because Python turns 0x8000000
+          into either a large positive number (PyLong or PyInt on 64-bit
+          platforms) or a negative number on others (32-bit PyInt)
+          whereas the system expects it to be a 32bit bit field value
+          regardless of it being passed as an int or unsigned long on
+          various platforms.  See the termios.TIOCSWINSZ constant across
+          platforms for an example of thise.
+
+          If any of the 64bit platforms ever decide to use more than 32bits
+          in their unsigned long ioctl codes this will break and need
+          special casing based on the platform being built on.
+        */
+       unsigned int code;
        int arg;
        int ret;
        char *str;