classification
Title: multiprocessing_{send,recv}fd fail with fds > 256
Type: behavior Stage: resolved
Components: Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Ben.Darnell, asksol, exarkun, haypo, jnoller, ncoghlan, neologix, petri.lehtinen, pitrou, python-dev
Priority: normal Keywords: easy, needs review, patch

Created on 2011-03-24 00:57 by Ben.Darnell, last changed 2011-08-23 17:57 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue11657.patch petri.lehtinen, 2011-07-28 20:32
Messages (17)
msg131947 - (view) Author: Ben Darnell (Ben.Darnell) Date: 2011-03-24 00:57
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
msg141315 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-07-28 20:32
Attached a patch for v2.7. It changes the assignments to memcpy() calls.
msg141316 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2011-07-28 20:39
Thanks for the patch Petri.  Are you interested in writing a unit test for this as well?
msg141525 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-08-01 18:35
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?
msg141530 - (view) Author: Ben Darnell (Ben.Darnell) Date: 2011-08-01 19:31
These functions are used when passing a socket object across a multiprocessing.Queue.
msg142627 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-21 15:07
> 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...
msg142643 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2011-08-21 20:01
Yes, Charles - the test is not only welcome, but needed - it just can't rely on reading /etc/fstab ;)
msg142644 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2011-08-21 20:02
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).
msg142648 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-21 21:01
> 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 ;-)
msg142677 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-08-22 05:38
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.
msg142684 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-08-22 06:25
For 3.3, it may be relevant that send/recvmsg are now available via the socket API (see #6560)
msg142691 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-22 07:15
> 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).
msg142837 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-23 15:53
> 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.
msg142839 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-23 16:00
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)
msg142847 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-23 17:52
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
msg142849 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-23 17:56
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
msg142851 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-23 17:57
I committed a patch with some tests. Thank you!
History
Date User Action Args
2011-08-23 17:57:34pitrousetstatus: open -> closed
resolution: fixed
messages: + msg142851

stage: patch review -> resolved
2011-08-23 17:56:42python-devsetmessages: + msg142849
2011-08-23 17:52:08python-devsetnosy: + python-dev
messages: + msg142847
2011-08-23 16:00:36pitrousetmessages: + msg142839
2011-08-23 15:56:28hayposetnosy: + haypo
2011-08-23 15:53:59pitrousetnosy: + pitrou
messages: + msg142837
2011-08-22 07:15:05neologixsetmessages: + msg142691
2011-08-22 06:25:23ncoghlansetnosy: + ncoghlan
messages: + msg142684
2011-08-22 05:38:14petri.lehtinensetmessages: + msg142677
2011-08-21 21:01:44neologixsetmessages: + msg142648
2011-08-21 20:02:50jnollersetmessages: + msg142644
2011-08-21 20:01:40jnollersetmessages: + msg142643
2011-08-21 15:07:52neologixsetnosy: + neologix
messages: + msg142627
2011-08-01 19:31:28Ben.Darnellsetmessages: + msg141530
2011-08-01 18:35:36petri.lehtinensetmessages: + msg141525
2011-07-28 20:39:15exarkunsetnosy: + exarkun
messages: + msg141316
2011-07-28 20:32:54petri.lehtinensetfiles: + issue11657.patch

versions: - Python 3.1
keywords: + patch, easy, needs review
nosy: + petri.lehtinen

messages: + msg141315
stage: patch review
2011-03-25 02:09:55pitrousetnosy: + jnoller, asksol

versions: + Python 3.1, Python 2.7, Python 3.2, Python 3.3
2011-03-24 00:57:23Ben.Darnellcreate