Fix for 926423: socket timeouts + Ctrl-C don't play nice

[irmen]

BPO	1102879
Nosy	@loewis, @irmen
Files	patch.txt: patch for Modules/socketmodule.c

^{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 = None
closed_at = <Date 2006-07-07.23:03:52.000>
created_at = <Date 2005-01-15.11:37:24.000>
labels = ['extension-modules']
title = "Fix for 926423: socket timeouts + Ctrl-C don't play nice"
updated_at = <Date 2006-07-07.23:03:52.000>
user = 'https://github.com/irmen'

bugs.python.org fields:

activity = <Date 2006-07-07.23:03:52.000>
actor = 'tony_nelson'
assignee = 'none'
closed = True
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2005-01-15.11:37:24.000>
creator = 'irmen'
dependencies = []
files = ['6436']
hgrepos = []
issue_num = 1102879
keywords = ['patch']
message_count = 7.0
messages = ['47541', '47542', '47543', '47544', '47545', '47546', '47547']
nosy_count = 3.0
nosy_names = ['loewis', 'irmen', 'tony_nelson']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1102879'
versions = []

[irmen]

Please have a close look at the attached patch.
Essentially, it fixes internal_select to not return
zero on error condition, and also adds a test for errno
at all calls to internal_select.

A few remarks:

1. as indicated in the patch, I'm not sure if errno
   should also be tested in the internal_connect function;
2. 'timeout' is no longer a boolean indicating a
   timeout condition, it should probably be renamed to
   'selectresult' or something similar;
3. I'm not too happy with the fact that the
   if(timeout==-1 && erro) test must be duplicated in a
   lot of functions;
4. does it do the right thing? The check for
   timeout==-1 MUST be there otherwise a lot of errors
   turn up in the regression tests. With this patch they
   run fine, btw

[loewis]

Logged In: YES
user_id=21627

Thanks for the patch.

1. I could not see where you dealt with internal_connect in
   the patch. However, as a connect can also be cancelled with
   Ctrl-C, I think you need to deal with it, too.

2. I agree, renaming it would be good

3. you could come up with a macro to avoid code duplication
   (relying on consistent naming of variables, but that might
   confure more than help

4. This is almost right. The critical thing is that you read
   errno much too late. errno is meant to be read immediately
   after the system call. Any later system call can modify the
   value - including all of the pthread calls that we do
   in-between. So the right solution is to copy errno right
   after select, into a local variable of the caller, and read
   *that* variable. In order to avoid modifying SetFromErrno,
   you can assign back to errno before calling it.

[tonynelson]

Logged In: YES
user_id=1356214

Martin, I'm working on an update to this patch so that it
will finally get fixed. I have a question about your answer
to #4. You say that errno is read "much too late", but
that's the same way that errorhandler() does it, and it
seems acceptable there. I don't see any calls in between
that would trash errno (at least after my patch is applied
:). Is there really a problem? I do have it fixed, by
returning errno from internal_select(), but if it isn't
needed I'd like to simplify the patch.

BTW, I'm handling #1 (internal_connect()). There is still
more work to do -- I think that it would be better to handle
Ctl-C as the interpreter does on its periodic checks, so
that it becomes KeyboardInterrupt instead of some other
error with a ' (4, ' in it. Umm, I need to do more research
on that.

[loewis]

Logged In: YES
user_id=21627

If I understand correctly, you have Py_END_ALLOW_THREADS
between the system call and the access to errno.
Py_END_ALLOW_THREADS expands to PyEval_RestoreThread, which
calls PyThread_acquire_lock, which (on pthreads) calls
sem_wait, which may set errno to EINTR.

It may well be that the same mistake is made in other
places. That the code is in Python is no proof that it is
"acceptable" - only that earlier reviewers did not catch the
mistake. If so, this should perhaps be discussed on
python-dev: if we want an explicit guarantee that errno
won't change across these macros, we should change the
functions to provide that guarantee.

In general, you should assume that any C library call may
change errno, unless it does not document the values that
errno may receive due to this system call. So for any
library call between the original call and the usage of
errno, you have to analyse whether it may change errno
(directly or indirectly).

[tonynelson]

Logged In: YES
user_id=1356214

Hmm, not me, but yes, socketmodule.c wraps its system calls
in Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS, and then
does its error checking. Here's a small function (from trunk):

static PyObject *
sock_connect(PySocketSockObject *s, PyObject *addro)
{
	struct sockaddr *addr;
	int addrlen;
	int res;
	int timeout;

if (!getsockaddrarg(s, addro, &addr, &addrlen))
	return NULL;

Py_BEGIN_ALLOW_THREADS
res = internal_connect(s, addr, addrlen, &timeout);
Py_END_ALLOW_THREADS

	if (timeout) {
		PyErr_SetString(socket_timeout, "timed out");
		return NULL;
	}
	if (res != 0)
		return s->errorhandler();
	Py_INCREF(Py_None);
	return Py_None;
}

Note that both the check for timeout and also the
s->errorhandler() call (which defaults to set_error() and
probably pre-dates timeout) use errno, everywhere. If this
is wrong, I'll bring it up on python-dev.

Do you have an example of a properly-coded module? For
example, the first one I checked, fctlmodule.c, does it the
same way.

[tonynelson]

Logged In: YES
user_id=1356214

Ehh, we can stop worrying. From ceval.h:

"For convenience, that the value of 'errno' is restored
across Py_END_ALLOW_THREADS and Py_BLOCK_THREADS."

This means that I can remove from my patch the extra code to
restore errno.

[tonynelson]

Logged In: YES
user_id=1356214

I've submitted a new patch as 1519025.