Make fcntl work properly on AMD64

[bradh]

BPO	1309352
Nosy	@loewis, @pitrou, @bitdancer
Files	fcntl-2005-10-06.patch: Patch correcting problem dnotify.py: dnotify.py from buildbot.sf.net, showing problem fcntl.patch2.txt: nn version of patch fcntl.patch3.txt

^{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/pitrou'
closed_at = <Date 2009-05-24.15:47:53.026>
created_at = <Date 2005-09-30.10:18:49.000>
labels = ['extension-modules', 'type-bug']
title = 'Make fcntl work properly on AMD64'
updated_at = <Date 2009-05-24.15:47:53.005>
user = 'https://bugs.python.org/bradh'

bugs.python.org fields:

activity = <Date 2009-05-24.15:47:53.005>
actor = 'pitrou'
assignee = 'pitrou'
closed = True
closed_date = <Date 2009-05-24.15:47:53.026>
closer = 'pitrou'
components = ['Extension Modules']
creation = <Date 2005-09-30.10:18:49.000>
creator = 'bradh'
dependencies = []
files = ['6801', '6802', '6803', '13988']
hgrepos = []
issue_num = 1309352
keywords = ['patch']
message_count = 13.0
messages = ['48806', '48807', '48808', '48809', '48810', '87811', '87812', '87813', '88068', '88082', '88083', '88259', '88264']
nosy_count = 5.0
nosy_names = ['loewis', 'nnorwitz', 'pitrou', 'bradh', 'r.david.murray']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1309352'
versions = ['Python 2.6', 'Python 3.1', 'Python 2.7']

[bradh]

The fcntl call doesn't work correctly on AMD-64, because of an
unsigned int conversion problem. I found the problem using the
dnotify.py code from buildbot.sf.net (attached). It (roughly) does:
self.flags = reduce(lambda x, y: x | y, flags) |
fcntl.DN_MULTISHOT
self.fd = os.open(dirname, os.O_RDONLY)
fcntl.fcntl(self.fd, fcntl.F_NOTIFY, self.flags)

fcntl.DN_MULTISHOT is 0x80000000, which causes
OverflowError: signed integer is greater than maximum

There is a similar fix already committed for ioctl - see
http://cvs.sourceforge.net/viewcvs.py/python/python/dist/src/Modules/fcntlmodule.c?r1=2.43&r2=2.44

[nnorwitz]

Logged In: YES
user_id=33168

Thanks for your patch.

I agree there's a problem, though I disagree with the fix.
man fcntl says that the third argument is a long on my box
(gentoo). Is that the same for you (3rd arg is long)? I
don't think the first i (int arg) should be changed. ISTM
only the second part of your patch (modifying the arg
variable) is on the right track. I've attached my version
of the patch, can you test that this works for you? It
seems to work for dnotify, though I'm not sure what it
should do.

Finally, could you create a real unittest in
Lib/test/test_fcntl.py?

Let me know if you agree or disagree with my assessment.
Thanks.

[loewis]

Logged In: YES
user_id=21627

POSIX/Single Unix has this signature of fcntl:

#include <fcntl.h>

int fcntl(int fildes, int cmd, ...);

The additional parameters depend on the cmd argument:

• F_DUPFD, F_SETFD, F_SETFL, F_SETOWN: int
• F_GETFD, F_GETFL, F_GETOWN, F_SETLKW: no argument
• F_GETLK, F_SETLK: struct flock*
  Other values: implementation defined.

[nnorwitz]

Logged In: YES
user_id=33168

Hmmm, how should we handle "Other values: implementation
defined"? That really concerns me. It seems that we can
never do the right thing, because we can't really know what
is right.

I can use I (unsigned int) format for the third parameter
which is a single char change from "i". This fixes the
problem, but I'm not sure if it's best in the long run.
Suggestions?

[loewis]

Logged In: YES
user_id=21627

I think it will always be possible to produce crashes. If
some operating system treats a parameter as a pointer, it
doesn't matter whether we process ints or longs;it might
always cause overwriting some random memory (if the system
uses the pointer for output).

The only solution would be to restrict the commands to the
ones we understand, giving an exception for the rest. In the
spirit of "consenting adults", this would be unpythonic.

So if we want to give the user more control, we might do
type-based reasoning: if the user passes an int, pass an C
int (raising an exception if it is out of range). If the
user passes a long, pass a C long if it fits, else a C long
long, else raise an exception.

I'm concerned about the POSIX vs. Linux story, though: POSIX
claims that these are ints in many cases, yet Linux
apparently uses long throughout. Doesn't this give an
inconsistency in calling conventions on 64-bit platforms
(with 64-bit longs), which would make Linux not POSIX-compliant?

[pitrou]

I confirm this on trunk, using Brad's test script.

[pitrou]

Here is an updated patch with a test.

[pitrou]

As for the calling convention concern, it seems to me that a sane ABI
wouldn't make a difference between ints and longs - both should fit in a
standard machine word.

[bitdancer]

@ap: ran patched test on Freebsd (6.2) as requested. Result is:

test_fcntl_64_bit (test.test_fcntl.TestFcntl) ... skipped 'F_NOTIFY or
DN_MULTISHOT unavailable'

[pitrou]

@ap: ran patched test on Freebsd (6.2) as requested.

Thanks! For the record, did the rest of test_fcntl pass?
Is it a 32-bit or a 64-bit build?

[bitdancer]

On Tue, 19 May 2009 at 16:24, Antoine Pitrou wrote:

Thanks! For the record, did the rest of test_fcntl pass?
Is it a 32-bit or a 64-bit build?

Yes and 32 bit.

[pitrou]

Gonna apply if nobody opposes :)

[pitrou]

Committed in r72887, r72888, r72889. Thanks!