homepage

Line 125 of multiprocessing.c is "*CMSG_DATA(cmsg) = fd;". CMSG_DATA returns an unsigned char*, while fd is an int, so this code does not support file descriptors > 256 (additionally, I'm not sure if the buffer is guaranteed to be initialized with zeros).  recvfd has an analogous problem at line 168. Both of these need to be changed to copy the entire integer, e.g. by casting the result of CMSG_DATA to an int*.
http://hg.python.org/cpython/file/5deb2094f033/Modules/_multiprocessing/multiprocessing.c 

Attached a patch for v2.7. It changes the assignments to memcpy() calls.

Thanks for the patch Petri. Are you interested in writing a unit test for this as well?

I looked at multiprocessing code, but didn't understand how to trigger a call to these functions. Makes it hard to come up with a unit test...
Ben: Do you still remember how you stumbled upon this issue?

These functions are used when passing a socket object across a multiprocessing.Queue.

> I looked at multiprocessing code, but didn't understand how to trigger a
> call to these functions. Makes it hard to come up with a unit test...
Here's a sample test:
"""
import _multiprocessing
import os
import socket
for i in range(4, 256):
 os.dup2(1, i)
s, r = socket.socketpair()
pid = os.fork()
if pid == 0:
 # child
 fd = _multiprocessing.recvfd(r.fileno())
 f = os.fdopen(fd)
 print(f.read())
 f.close()
else:
 # parent
 f = open('/etc/fstab')
 _multiprocessing.sendfd(s.fileno(), f.fileno())
 f.close()
 os.waitpid(pid, 0)
"""
What happens is that the parent process opens /etc/fstab, and sends the FD to the child process, which prints it.
Now, if I run it with the current code, here's what I get:
"""
cf@neobox:~/cpython$ ./python ~/test.py 
Traceback (most recent call last):
 File "/home/cf/test.py", line 18, in <module>
 _multiprocessing.sendfd(s.fileno(), f.fileno())
OSError: [Errno 9] Bad file descriptor
cf@neobox:~/cpython$ strace -e sendmsg ./python ~/test.py 
sendmsg(3, {msg_name(0)=NULL, msg_iov(1)=[{"10円", 1}], msg_controllen=16, {cmsg_len=16, cmsg_level=SOL_SOCKET, cmsg_type=SCM_RIGHTS, {171137285}}, msg_flags=0}, 0) = -1 EBADF (Bad file descriptor)
Traceback (most recent call last):
 File "/home/cf/test.py", line 18, in <module>
 _multiprocessing.sendfd(s.fileno(), f.fileno())
OSError: [Errno 9] Bad file descriptor
"""
Duh, it's failing with EBADF.
Why?
 cmsg->cmsg_len = CMSG_LEN(sizeof(int));
 msg.msg_controllen = cmsg->cmsg_len;
 *CMSG_DATA(cmsg) = fd;
Since we only set one byte in CMSG_DATA, if the other bytes are non-zero, the value stored in CMSG_DATA(cmsg) ends up referring to a non existing FD, hence the EBDAF.
With this simple patch:
"""
diff -r e49dcb95241f Modules/_multiprocessing/multiprocessing.c
--- a/Modules/_multiprocessing/multiprocessing.c Sun Aug 21 12:54:06 2011 +0200
+++ b/Modules/_multiprocessing/multiprocessing.c Sun Aug 21 16:56:01 2011 +0200
@@ -111,7 +111,7 @@
 cmsg->cmsg_type = SCM_RIGHTS;
 cmsg->cmsg_len = CMSG_LEN(sizeof(int));
 msg.msg_controllen = cmsg->cmsg_len;
- *CMSG_DATA(cmsg) = fd;
+ *(int *)CMSG_DATA(cmsg) = fd;
 
 Py_BEGIN_ALLOW_THREADS
 res = sendmsg(conn, &msg, 0);
@@ -154,7 +154,7 @@
 if (res < 0)
 return PyErr_SetFromErrno(PyExc_OSError);
 
- fd = *CMSG_DATA(cmsg);
+ fd = *(int *)CMSG_DATA(cmsg);
 return Py_BuildValue("i", fd);
 }
"""
It works fine.
Note that if you want to check that for FD > 255, you'd have to add something like this at the top:
for i in range(4, 256):
 os.dup2(1, i)
Note that I just used a cast to (int *) instead of memcpy() because CMSG_DATA is actually int-aligned, so there's no risk of unaligned-access, and also it's what's commonly used in the litterature.
So, would you like to add a test along those lines to test_multiprocessing?
AFAICT, multiprocessing.connection is not even documented, but this shows that it really needs some testing...

Yes, Charles - the test is not only welcome, but needed - it just can't rely on reading /etc/fstab ;)

Charles; you have +commit, it seems. I would welcome the patch and test (just as long as the aforementioned reliance on /etc/fstab was removed).

> Charles; you have +commit, it seems.
Yeah, I could of course do this myself, but since Petri already
submitted a patch and seemed to be willing to update it with a test, I
thought he might be interested in this (I've also seen he's
participating to core-mentorship, so this would be a opportunity to
help fixing a bug).
As for the reliance on /etc/fstab, it was of course just a standalone
example to demonstrate how one could test this corner-case ;-)

Charles-François Natali wrote:
> Yeah, I could of course do this myself, but since Petri already
> submitted a patch and seemed to be willing to update it with a test, I
> thought he might be interested in this (I've also seen he's
> participating to core-mentorship, so this would be a opportunity to
> help fixing a bug).
I'm fine if you fix it, as I'm currently really short on time myself.
When I was skimming through the multiprocessing tests a month ago, I
noticed that much of the multiprocessing test suite is disabled, and
it confused me. That was one reason why I didn't write a test right
away.

For 3.3, it may be relevant that send/recvmsg are now available via the socket API (see #6560)

> I'm fine if you fix it, as I'm currently really short on time myself.
OK, I'll go ahead.
> For 3.3, it may be relevant that send/recvmsg are now available via the socket API (see #6560).
Indeed. We might still need C code for the Windows part (I won't be
able to help much with Windows, I'm blissfully ignorant).

> For 3.3, it may be relevant that send/recvmsg are now available via 
> the socket API (see #6560)
I think sendfds/recvfds helper methods could be added to the socket type, so that programmers don't have to get the incantations right by themselves (note the plural, because passing several fds at once is more generic :-)).
That said, +1 on committing Petri's patch. Will do it in a hour or two if noone is faster.

Now that I think of it, it would be nice to add some simple tests for recvfd and sendfd in test_multiprocessing.
(also, when os.dup2() is available, you can easily generate an arbitrary big file descriptor)

New changeset 5b2f357989bb by Antoine Pitrou in branch '3.2':
Issue #11657: Fix sending file descriptors over 255 over a multiprocessing Pipe.
http://hg.python.org/cpython/rev/5b2f357989bb
New changeset 4e7a4e098f38 by Antoine Pitrou in branch 'default':
Issue #11657: Fix sending file descriptors over 255 over a multiprocessing Pipe.
http://hg.python.org/cpython/rev/4e7a4e098f38 

New changeset d4d9a3e71897 by Antoine Pitrou in branch '2.7':
Issue #11657: Fix sending file descriptors over 255 over a multiprocessing Pipe.
http://hg.python.org/cpython/rev/d4d9a3e71897 

I committed a patch with some tests. Thank you!