Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ioctl request argument broken on 64-bit OpenBSD or OS X #45812

Closed
fbvortex mannequin opened this issue Nov 20, 2007 · 15 comments
Closed

ioctl request argument broken on 64-bit OpenBSD or OS X #45812

fbvortex mannequin opened this issue Nov 20, 2007 · 15 comments
Assignees
Labels
easy type-bug An unexpected behavior, bug, or error

Comments

@fbvortex
Copy link
Mannequin

fbvortex mannequin commented Nov 20, 2007

BPO 1471
Nosy @loewis, @birkenfeld, @gpshead

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2008-08-04.00:46:44.943>
created_at = <Date 2007-11-20.00:27:07.941>
labels = ['easy', 'type-bug']
title = 'ioctl request argument broken on 64-bit OpenBSD or OS X'
updated_at = <Date 2008-08-04.00:46:44.942>
user = 'https://bugs.python.org/fbvortex'

bugs.python.org fields:

activity = <Date 2008-08-04.00:46:44.942>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2008-08-04.00:46:44.943>
closer = 'gregory.p.smith'
components = []
creation = <Date 2007-11-20.00:27:07.941>
creator = 'fbvortex'
dependencies = []
files = []
hgrepos = []
issue_num = 1471
keywords = ['patch', '64bit', 'easy']
message_count = 15.0
messages = ['57673', '57676', '57686', '57697', '57699', '58237', '63803', '63897', '63898', '64124', '64155', '70078', '70088', '70089', '70673']
nosy_count = 7.0
nosy_names = ['loewis', 'georg.brandl', 'gregory.p.smith', 'jafo', 'fbvortex', 'nicm', 'msavory']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1471'
versions = ['Python 2.5']

@fbvortex
Copy link
Mannequin Author

fbvortex mannequin commented Nov 20, 2007

The following lines of code work on Linux platforms (amd64), and 32-bit
OpenBSD (i386), but not on 64-bit OpenBSD platforms (at least not on
amd64 or sparc64):

import fcntl,os,pty,termios,select,sys,struct,pwd,signal,os
pid,fd=pty.fork()
fcntl.ioctl(fd, termios.TIOCSWINSZ, struct.pack("HHHH",80,25,0,0))

This gives an "IOError: [Errno 25] Inappropriate ioctl for device"
exception.

The python version in question on OpenBSD is:

Python 2.5.1 (r251:54863, Nov 16 2007, 18:16:44)
[GCC 3.3.5 (propolice)] on openbsd4

The winsize structure as dumped using sizeof(struct winsize) under
OpenBSD/sparc64 is 8 bytes large, and so is the result from the
struct.pack operation. Forcing the endianness big or little in the
struct.pack formatting string has no effect on the error.

@fbvortex fbvortex mannequin added the type-bug An unexpected behavior, bug, or error label Nov 20, 2007
@fbvortex
Copy link
Mannequin Author

fbvortex mannequin commented Nov 20, 2007

The following C code, when compiled with -lutil runs without reporting
any errors on both the sparc64 and i386 platforms on OpenBSD:

#include <util.h>
#include <utmp.h>
#include <termios.h>
#include <sys/ioctl.h>
#include <stdio.h>
 
int main(void)
{
        int fd;
        struct winsize w;
 
        w.ws_row = 25;
        w.ws_col = 80;
        w.ws_xpixel = w.ws_ypixel = 0;
 
        forkpty(&fd, NULL, NULL, NULL);
        if (ioctl(fd,TIOCSWINSZ, &w) == -1)
                perror("ioctl");
        return 0;
}

@loewis
Copy link
Mannequin

loewis mannequin commented Nov 20, 2007

So what's the definition of struct winsize on these systems?

Also, why do you think this is a bug in Python? AFAICT, the specific
ioctl call does not occur in Python, but in your own code.

@loewis loewis mannequin added the invalid label Nov 20, 2007
@nicm
Copy link
Mannequin

nicm mannequin commented Nov 20, 2007

I can also reproduce this on OpenBSD/amd64 and was one of the people who
discussed it with the submitter before he created this report.

So what's the definition of struct winsize on these systems?

The definition of struct winsize on both 32-bit and 64-bit OpenBSD
platforms is:

struct winsize {
        unsigned short  ws_row;         /* rows, in characters */
        unsigned short  ws_col;         /* columns, in characters */
        unsigned short  ws_xpixel;      /* horizontal size, pixels */
        unsigned short  ws_ypixel;      /* vertical size, pixels */
};

We have verified that (sizeof (struct winsize)) is 8 on all three
platforms (32-bit i386, and 64-bit amd64 and sparc64).

Also, why do you think this is a bug in Python? AFAICT, the specific
ioctl call does not occur in Python, but in your own code.

This Python code fails:

fcntl.ioctl(fd, termios.TIOCSWINSZ, struct.pack("HHHH",80,25,0,0))

(Error: IOError: [Errno 25] Inappropriate ioctl for device) This code
also fails with the same error message (I would expect EINVAL instead):

fcntl.ioctl(0, termios.TIOCSWINSZ, "")

Corresponding test C code to call the ioctl (as listed in a previous
message) works without problems on all three platforms. I don't know how
Python fcntl.ioctl works and perhaps the problem is not Python, but I am
having to try fairly hard to think what else could be involved.

The constant appears to be the correct ioctl number. (Python code:

python2.5 -c 'import termios; print "%d" % (termios.TIOCSWINSZ)'

matches C code:

#include <termios.h>
int main(void) { printf("%ld\n", TIOCSWINSZ); }

on all three platforms.)

I am told it works on Linux. However, Linux declares ioctl as:

int ioctl(int d, int request, ...); 

So 'request' is 32-bits, but on OpenBSD ioctl is declared as:

int ioctl(int d, unsigned long request, ...);

So on amd64 and sparc64, 'request' is 64-bits. Could this be a factor?

-- Nicholas.

@nicm
Copy link
Mannequin

nicm mannequin commented Nov 20, 2007

Okay, looks like my guess was correct. The diff at the end of this mail
makes it work on OpenBSD/amd64 and it continues to work on i386, but it
will probably break Linux (due to the problem it was working around
mentioned in the comment at the beginning). I don't know enough about
the Python build system to suggest a correct fix. (SUSv3 seems to
suggest ioctl's request should be int, so perhaps OpenBSD is ultimately
at fault.)

fcntl_ioctl in fcntlmodule.c seems to have some other potential
problems on 64-bit archs too, as it seems to coerce the ioctl argument
(potentially a pointer) into an int.

-- Nicholas.

--- fcntlmodule.c.orig	Tue Nov 20 09:39:12 2007
+++ fcntlmodule.c	Tue Nov 20 09:59:50 2007
@@ -101,7 +101,7 @@
 	   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;
+	unsigned long code;
 	int arg;
 	int ret;
 	char *str;
@@ -109,7 +109,7 @@
 	int mutate_arg = 1;
  	char buf[IOCTL_BUFSZ+1];  /* argument plus NUL byte */
 
-	if (PyArg_ParseTuple(args, "O&Iw#|i:ioctl",
+	if (PyArg_ParseTuple(args, "O&Lw#|i:ioctl",
                              conv_descriptor, &fd, &code, 
 			     &str, &len, &mutate_arg)) {
 		char *arg;
@@ -160,7 +160,7 @@
 	}
 
 	PyErr_Clear();
-	if (PyArg_ParseTuple(args, "O&Is#:ioctl",
+	if (PyArg_ParseTuple(args, "O&Ls#:ioctl",
                              conv_descriptor, &fd, &code, &str, &len)) 
{
 		if (len > IOCTL_BUFSZ) {
 			PyErr_SetString(PyExc_ValueError,
@@ -182,7 +182,7 @@
 	PyErr_Clear();
 	arg = 0;
 	if (!PyArg_ParseTuple(args,
-	     "O&I|i;ioctl requires a file or file descriptor,"
+	     "O&L|i;ioctl requires a file or file descriptor,"
 	     " an integer and optionally an integer or buffer argument",
 			      conv_descriptor, &fd, &code, &arg)) {
 	  return NULL;

@msavory
Copy link
Mannequin

msavory mannequin commented Dec 6, 2007

Similar issue seen in 64 bit OSX 10.5

File "./ajaxterm.py", line 418, in create
fcntl.ioctl(fd,
struct.unpack('i',struct.pack('I',termios.TIOCSWINSZ))[0],
struct.pack("HHHH",h,w,0,0))
IOError: [Errno 25] Inappropriate ioctl for device

$ python
Python 2.5.1 (r251:54863, Oct  5 2007, 21:08:09) 
[GCC 4.0.1 (Apple Inc. build 5465)] on darwin

@jafo
Copy link
Mannequin

jafo mannequin commented Mar 17, 2008

I think this proposed change needs some research into what the standards
say about ioctl's return code, and if the change to a long return code
is done then test it on Linux to see if it breaks things or if it works.
Thoughts?

@nicm
Copy link
Mannequin

nicm mannequin commented Mar 18, 2008

It's not the return value, it's the request argument (second argument).
I thought SUSv3 had it as an int, but looking again its ioctl seems to
be a) optional and b) all about STREAMS, so I don't think it applies:

http://www.opengroup.org/onlinepubs/009695399/functions/ioctl.html

-- Nicholas

@gpshead
Copy link
Member

gpshead commented Mar 18, 2008

i'd say the patch is fine.

on linux ioctl takes an int.
on openbsd it takes an unsigned long.
on something else it might even take its own type like an ioctl_t.

regardless, treating the parameter as either a long or unsigned long
will work properly as that will cast downwards to the actual used
parameter type correctly in the C code.

@gpshead gpshead added easy and removed invalid labels Mar 18, 2008
@gpshead gpshead self-assigned this Mar 18, 2008
@gpshead gpshead changed the title ioctl doesn't work properly on 64-bit OpenBSD ioctl request argument broken on 64-bit OpenBSD or OS X Mar 18, 2008
@gpshead
Copy link
Member

gpshead commented Mar 19, 2008

I am unable to reproduce this problem at all on Mac OS X 10.4 or 10.5
with 32-bit or 64-bit python trunk builds.

I have however checked in a fix that'll prevent negative code values
passed in from being sign extended to 64-bits when converted to an
unsigned long on platforms that use those.

A unittest for fcntl.ioctl behavior when passed both positive and
negative values is included.

trunk r61652

while the Linux man page describe's ioctl's code parameter as an int,
the sys/ioctl.h file on linux has it as an unsigned long int in the
function prototype. So its quite possible it is actually an unsigned
long on all platforms and we should change unsigned int code to unsigned
long code and the 'I' to a 'k'.

However if that is done we -might- have a sign extension problem again
if anyone is passing a negative value in and expecting it to act like a
32-bit value. If the OS kernels only look at the lower 32-bits of the
ioctl code during the syscall it'd work, but if it tested the 64bit
value -123 64bit != -123 32bit. The better answer to that would be to
just reject all negative values and fix any defined ioctl code constants
to be positive on all platforms.

fbvortex. Can you apply the change from svn to your python and test
that it works properly on your 64-bit OpenBSD system? Thanks.

paste a note here once you have with the results.

@gpshead
Copy link
Member

gpshead commented Mar 20, 2008

r61665 moves the test to test_ioctl instead of test_fcntl. it also
forces it to only run when a pty is present rather than assuming fd 0

@birkenfeld
Copy link
Member

Something else to do here?

@gpshead
Copy link
Member

gpshead commented Jul 20, 2008

i believe this is fixed by the two changes mentioned above, i was
waiting for fbvortex to confirm the fix on his 64-bit OpenBSD system.

these changes need backporting to release25-maint.

@nicm
Copy link
Mannequin

nicm mannequin commented Jul 20, 2008

I was going to test this on sparc64 (I no longer have access to amd64)
but I've been busy/on holiday, I'll try to do it this week.

@gpshead
Copy link
Member

gpshead commented Aug 4, 2008

committed to release25-maint as r65466. if testing on 64bit openbsd or
similar reveals that this is not fixed, please reopen the bug.

@gpshead gpshead closed this as completed Aug 4, 2008
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants