socket.sendall() crash when receiving a signal

[pitrou]

BPO	9950
Nosy	@gpshead, @pitrou
Files	sendallinterrupt.patch

^{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 2010-09-27.18:33:38.195>
created_at = <Date 2010-09-25.19:07:46.937>
labels = ['library', 'type-crash']
title = 'socket.sendall() crash when receiving a signal'
updated_at = <Date 2010-09-28.07:44:59.715>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2010-09-28.07:44:59.715>
actor = 'ned.deily'
assignee = 'pitrou'
closed = True
closed_date = <Date 2010-09-27.18:33:38.195>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2010-09-25.19:07:46.937>
creator = 'pitrou'
dependencies = []
files = ['19015']
hgrepos = []
issue_num = 9950
keywords = ['patch']
message_count = 8.0
messages = ['117385', '117387', '117388', '117389', '117390', '117453', '117460', '117505']
nosy_count = 3.0
nosy_names = ['gregory.p.smith', 'exarkun', 'pitrou']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue9950'
versions = ['Python 3.1', 'Python 2.7', 'Python 3.2']

[pitrou]

This was introduced by r74426 which addressed bpo-1628205. socket.sendall() calls PyErr_CheckSignals() (and potentially returns to the caller) without having the GIL.

>>> import socket
>>> c, s = socket.socketpair()
>>> s.sendall(b"x"*(100 * 1024**2))
^C^CFatal Python error: PyThreadState_Get: no current thread

[pitrou]

The fix is very simple, but perhaps a test should be added.

diff -r af0d7b32d6ce Modules/socketmodule.c
--- a/Modules/socketmodule.c    Fri Sep 24 20:03:12 2010 +0200
+++ b/Modules/socketmodule.c    Sat Sep 25 21:09:58 2010 +0200
@@ -2581,8 +2581,8 @@ sock_sendall(PySocketSockObject *s, PyOb
         return select_error();
     }
 
-    Py_BEGIN_ALLOW_THREADS
     do {
+        Py_BEGIN_ALLOW_THREADS
         timeout = internal_select(s, 1);
         n = -1;
         if (timeout)
@@ -2592,6 +2592,7 @@ sock_sendall(PySocketSockObject *s, PyOb
 #else
         n = send(s->sock_fd, buf, len, flags);
 #endif
+        Py_END_ALLOW_THREADS
         if (n < 0) {
 #ifdef EINTR
             /* We must handle EINTR here as there is no way for
@@ -2610,7 +2611,6 @@ sock_sendall(PySocketSockObject *s, PyOb
         buf += n;
         len -= n;
     } while (len > 0);
-    Py_END_ALLOW_THREADS
     PyBuffer_Release(&pbuf);
 
     if (timeout == 1) {

[pitrou]

Actually, the patch is enough to suppress the crash, but sendall() behaviour is buggy in another way: EINTR may be received as part of the select() call (on sockets with a timeout), in which case the loop will be exited early instead of retrying, losing track of the number of bytes written (something which the original patch aimed to avoid).

For example:

>>> def handler(*args): print (args)
... 
>>> signal.signal(signal.SIGALRM, handler)
0
>>> c, s = socket.socketpair()
>>> s.settimeout(60.0)
>>> signal.alarm(1); s.sendall(b"x" * (100 * 1024**2))
0
(14, <frame object at 0x2b8f220>)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
socket.error: [Errno 4] Interrupted system call

[pitrou]

Oh, and an additional bug is that send() can return a successful partial write when it was actually interrupted by a signal.

[pitrou]

Here is a patch fixing the aforementioned issues, and with tests.

[pitrou]

Patch committed in r85032. I'm gonna watch the buildbots a bit, in case the test fails on some platforms.

[pitrou]

The patch passes at least on Linux, OS X and Solaris buildbots. Backported to 3.1 (r85034) and 2.7 (r85035).

[gpshead]

thanks Antoine!