classification
Title: (FreeBSD/OSX) Fix fcntl module to accept 'unsigned long' type commands for ioctl(2).
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.6, Python 3.5, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: koobs, larry, martin.panter, serhiy.storchaka
Priority: normal Keywords: easy

Created on 2015-09-08 06:35 by koobs, last changed 2017-06-15 03:44 by martin.panter.

Messages (9)
msg250163 - (view) Author: Kubilay Kocak (koobs) Date: 2015-09-08 06:35
In my current attempt to create a FreeBSD port for python35, I've come across a patch rejection for the fcntlmodule.c for a local port patch we've been carrying since Python 2.6:

https://svnweb.freebsd.org/ports/head/lang/python27/files/patch-Modules__fcntlmodule.c?revision=391238&view=markup

The original commit log for this change is:

====================================

  Fix fcntl module to accept 'unsigned long' type commands for ioctl(2).
  
  Although POSIX says the type is 'int', all BSD variants (including Mac OS X)
  have been using 'unsigned long' type for very long time and its use predates
  the standard long enough.  For certain commands (e.g., TIOCSWINSZ, FIONBIO),
  the Python value may get sign-extended on 64-bit platforms (by implicit type
  promotion) and it causes annoying warnings from kernel such as this:
  
  WARNING pid 24509 (python2.6): ioctl sign-extension ioctl ffffffff8004667e

====================================

I'm not sure how this should be fixed upstream, nor clear on how to re-patch it given recent changes to  fcntlmodule.c
msg250164 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-08 07:24
You need just replace unsigned_int with unsigned_long in Clinic declaration for fcntl.ioctl in Modules/fcntlmodule.c and regenerate Clinic code (make clinic).
msg250166 - (view) Author: Kubilay Kocak (koobs) Date: 2015-09-08 07:29
Thanks for the insight Serhiy. A few questions ..

Is clinic code updated based on *.c declarations at build time? If not when/how is the best place/method to run this for our ports/package builds?

Does your suggestion to switch unsigned_int to unsigned_long imply that the following lines are no longer necessary?

-    if (PyArg_ParseTuple(args, "O&Iw#|i:ioctl",
+    if (PyArg_ParseTuple(args, "O&kw#|i:ioctl",

How do we get something like this fixed upstream for FreeBSD/OSX?

If left un-patched, what is the impact on the user/system? Just a warning on console?
msg250170 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-08 08:06
No, the clinic code is not updated at build time. Your should either update clinic code just after patching Modules/fcntlmodule.c (Python 3 is needed), or update clinic code at patch creation time and include changes of generated clinic files in the patch.

Generated clinic files contain a code for argument parsing (PyArg_ParseTuple...).

I don't know if there is easy way to make this change conditionally for FreeBSD/OSX. The only way that I know is writing custom converters.

Perhaps the original commit log is outdated. What was the type of code at the time of this log? Now it is unsigned int and this doesn't make sign extension when casted to unsigned long. While the code parameter is in the range 0..0xffffffff, unpatched code shouldn't have any visible effects.
msg250173 - (view) Author: Kubilay Kocak (koobs) Date: 2015-09-08 09:19
@Serhiy

If by "type of code at the time of commit" you mean upstream python code, msg250163 contains a link to what bits we replace in fcntlmodule.c and that "I -> k" has always been the same.
msg250174 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-08 09:36
Originally the type of the code variable in fcntl_ioctl() was int. In cbad1f5cabb1 it was changed to unsigned int. I guess that the log message that you cited was written before this.
msg250178 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-09-08 10:54
If necessary, perhaps we could unconditionally change the Python-level argument to unsigned_long, and then add a conditional bit in the C code to convert it to int if appropriate.

But I wonder if most of the problem is fixed by Issue 1471 (linked from the commit Serhiy identified). As I see it, your patch should now only be needed to support “code” values outside of the range of unsigned_int, e.g. that require > 32 bits.
msg250322 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-09 16:19
The question is: are ioctl codes outside of the unsigned int range used on BSD family or Mac OS X?
msg296062 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-06-15 03:44
Maybe Issue 16124 is related; it mentions 64-bit values, but it sounds like an obscure use case.
History
Date User Action Args
2017-06-15 03:44:28martin.pantersetmessages: + msg296062
2015-09-09 16:19:15serhiy.storchakasetmessages: + msg250322
2015-09-08 10:54:55martin.pantersetnosy: + martin.panter
messages: + msg250178
2015-09-08 09:36:51serhiy.storchakasetmessages: + msg250174
2015-09-08 09:19:23koobssetmessages: + msg250173
2015-09-08 08:06:37serhiy.storchakasetmessages: + msg250170
2015-09-08 07:29:51koobssetmessages: + msg250166
versions: + Python 3.6
2015-09-08 07:24:18serhiy.storchakasetnosy: + larry, serhiy.storchaka
messages: + msg250164
2015-09-08 06:35:17koobscreate