This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Make fcntl work properly on AMD64
Type: behavior Stage: patch review
Components: Extension Modules Versions: Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: bradh, loewis, nnorwitz, pitrou, r.david.murray
Priority: normal Keywords: patch

Created on 2005-09-30 10:18 by bradh, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
fcntl-2005-10-06.patch bradh, 2005-09-30 10:18 Patch correcting problem
dnotify.py bradh, 2005-09-30 10:20 dnotify.py from buildbot.sf.net, showing problem
fcntl.patch2.txt nnorwitz, 2005-10-01 03:33 nn version of patch
fcntl.patch3.txt pitrou, 2009-05-15 12:36
Messages (13)
msg48806 - (view) Author: Brad Hards (bradh) Date: 2005-09-30 10:18
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 
 
 
msg48807 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2005-10-01 03:33
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.
msg48808 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2005-10-11 23:58
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.
msg48809 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2005-10-12 04:45
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?
msg48810 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2005-10-12 08:29
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?
msg87811 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-15 12:20
I confirm this on trunk, using Brad's test script.
msg87812 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-15 12:36
Here is an updated patch with a test.
msg87813 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-15 12:49
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.
msg88068 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2009-05-19 06:30
@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'
msg88082 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-19 16:24
> @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?
msg88083 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2009-05-19 17:11
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.
msg88259 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-24 11:46
Gonna apply if nobody opposes :)
msg88264 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-24 15:47
Committed in r72887, r72888, r72889. Thanks!
History
Date User Action Args
2022-04-11 14:56:13adminsetgithub: 42434
2009-05-24 15:47:53pitrousetstatus: open -> closed
resolution: fixed
messages: + msg88264
2009-05-24 11:46:07pitrousetassignee: nnorwitz -> pitrou
messages: + msg88259
2009-05-19 17:11:55r.david.murraysetmessages: + msg88083
2009-05-19 16:24:25pitrousetmessages: + msg88082
2009-05-19 06:30:42r.david.murraysetnosy: + r.david.murray
messages: + msg88068
2009-05-15 12:49:23pitrousetmessages: + msg87813
stage: test needed -> patch review
2009-05-15 12:36:17pitrousetfiles: + fcntl.patch3.txt

messages: + msg87812
2009-05-15 12:20:21pitrousetnosy: + pitrou

messages: + msg87811
versions: + Python 3.1, Python 2.7
2009-05-15 02:43:34ajaksu2settype: behavior
stage: test needed
2008-01-12 04:34:30christian.heimessetversions: + Python 2.6, - Python 2.5
2005-09-30 10:18:49bradhcreate