classification
Title: Pass read only FD to signal.set_wakeup_fd
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: felipecruz, haypo, neologix, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2012-10-01 21:05 by felipecruz, last changed 2013-08-17 19:16 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue16105_v1.patch felipecruz, 2012-10-03 01:55 review
issue16105_v2.patch felipecruz, 2012-10-04 02:14 review
issue16105_v3.patch felipecruz, 2012-10-04 23:55 review
issue16105_v4.patch felipecruz, 2012-10-16 20:22 review
wakeup_fd_error.patch pitrou, 2013-08-15 21:55 review
wakeup_fd_error2.patch pitrou, 2013-08-16 19:27 review
Messages (23)
msg171745 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-01 21:05
It's possible to set a read only FD to signal.set_wakeup_fd(fd)

Since write call[1] inside 'trip_signal' return code is ignored, no error will be raised.


An untested solution is to call fcntl in this FD to check presence of write flags.

1 - http://hg.python.org/cpython/file/fb90e2ff95b7/Modules/signalmodule.c#l187
msg171760 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-10-01 23:26
Is it really a bug? A file descriptor is just an integer, it may be replaced later. Passed fd may be writable when set_wakeup_fd() is called, but then become read-only.
msg171770 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-02 02:05
I would not say that is a bug, but there is a write(wakeup_fd) call with ignored return code and maybe this can be improved to an output to stderr, or maybe a better solution.
msg171776 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-02 07:55
> I would not say that is a bug, but there is a write(wakeup_fd) call
> with ignored return code and maybe this can be improved to an output
> to stderr, or maybe a better solution.

The problem is that it's called from the signal handler, so there's not much we can do here (and certainly not log a warning or raise an exception).
There's a contract to respect here, and if the user doesn't follow it, trouble will follow: for example, if the passed FD isn't non-blocking, then the signal handler can deadlock if the write() blocks (pipe, socket, whatever).
msg171841 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-10-03 00:05
> The problem is that it's called from the signal handler, so there's not 
> much we can do here (and certainly not log a warning or raise an
> exception).

However, I think the errno could be passed via the "void *" argument to Py_AddPendingCall. Then checksignals_witharg can raise an error if the received errno is non-zero.

I agree with Felipe that issues here can be difficult to diagnose. For example the fd could get mistakingly closed, but the write() EBADF would then be ignored and the expected signal wakeups would be lost.
msg171847 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-03 01:55
Hello,

since Antonie mentioned Py_AddPendingCall I came up with a patch describing what he proposed.

Let me know if this patch can be improved or discarded(if the problem requires a more sophisticated solution). In case of improvement I can also submit another patch with a test case.
msg171853 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-03 05:43
> since Antonie mentioned Py_AddPendingCall I came up with a patch describing what he proposed.
>
> Let me know if this patch can be improved or discarded(if the problem requires a more sophisticated solution). In case of improvement I can also submit another patch with a test case.

Why limit to EBADF? You could also have EPIPE, EINVAL and many other errors.
The only error you may not want to report is EAGAIN.
msg171854 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-03 05:51
> I agree with Felipe that issues here can be difficult to diagnose. For example the fd could get mistakingly closed, but the write() EBADF would then be ignored and the expected signal wakeups would be lost.

Yeah, but it's also completely possible that the saved fd now points
to another file descriptor, and you end up corrupting a file, without
getting any error at all, and not getting signal wakeups. This API is
really fragile and dangerous, and warrants care anyway...
msg171861 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-10-03 07:04
A signal handler can be called anymore, anywhere. How do you handle such
exception in an application? "handle": do something better than exit the
apllication.
msg171865 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-10-03 07:55
> A signal handler can be called anymore, anywhere.

Oopos: anywhere/anytime.
msg171872 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-03 08:40
> A signal handler can be called anymore, anywhere. How do you handle such
> exception in an application? "handle": do something better than exit the
> apllication.

Well, chances are you won't, but failing with an explicit error
message is better than silently failing to deliver signals (which may
result in a deadlock, for example).
msg171888 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-03 14:23
> Why limit to EBADF? You could also have EPIPE, EINVAL and many other errors.
> The only error you may not want to report is EAGAIN.

Charles,
You're right! If all errno cases get covered in the patch,  will It looks reasonable?
msg171901 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-03 18:19
>> Why limit to EBADF? You could also have EPIPE, EINVAL and many other errors.
>> The only error you may not want to report is EAGAIN.
>
> Charles,
> You're right! If all errno cases get covered in the patch,  will It looks reasonable?

Raising an error in case the signal can't be written to the FD
(because the other end didn't drain the pipe/socket) seems reasonable.
You should just retry on EINTR (although any sane implementation
shouldn't return EINTR on non-blocking write, I don't think it's
strictly prohibited by POSIX).
msg171917 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-04 02:14
> Raising an error in case the signal can't be written to the FD
> (because the other end didn't drain the pipe/socket) seems reasonable.
> You should just retry on EINTR (although any sane implementation
> shouldn't return EINTR on non-blocking write, I don't think it's
> strictly prohibited by POSIX).

You mean retry one time or until success?
msg171997 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-10-04 21:34
> You mean retry one time or until success?

Until success.

It should also come with a test.
msg172030 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-04 23:55
This patch retries write() until success if errno == EINTR.
There is also a test.
msg173091 - (view) Author: Felipe Cruz (felipecruz) * Date: 2012-10-16 20:22
I've followed latest suggestions.

Test and code updated.
msg195294 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-15 21:55
Here is a cleaned up patch.
However, I'm wondering if raising an error is a good idea. The effect will be to get cryptic OSErrors at random execution points. Perhaps using PyErr_WriteUnraisable would be better?
msg195304 - (view) Author: Felipe Cruz (felipecruz) * Date: 2013-08-16 00:04
Looks like PyErr_WriteUnraisable can be a better choice. Exceptions at random execution points looks a little bit dirty at least for this case.
msg195402 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-16 19:27
Updated patch using PyErr_WriteUnraisable().
msg195422 - (view) Author: Felipe Cruz (felipecruz) * Date: 2013-08-16 21:18
Looks good.
msg195502 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-17 18:33
New changeset e2b234f5bf7d by Antoine Pitrou in branch 'default':
Issue #16105: When a signal handler fails to write to the file descriptor registered with ``signal.set_wakeup_fd()``, report an exception instead of ignoring the error.
http://hg.python.org/cpython/rev/e2b234f5bf7d
msg195508 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-17 19:16
Ok, I pushed the patch to 3.4. Thanks for the report!
History
Date User Action Args
2013-08-17 19:16:38pitrousetstatus: open -> closed
resolution: fixed
messages: + msg195508

stage: patch review -> resolved
2013-08-17 18:33:11python-devsetnosy: + python-dev
messages: + msg195502
2013-08-16 21:18:05felipecruzsetmessages: + msg195422
2013-08-16 19:27:26pitrousetfiles: + wakeup_fd_error2.patch

messages: + msg195402
stage: patch review
2013-08-16 00:04:22felipecruzsetmessages: + msg195304
2013-08-15 21:55:57pitrousetfiles: + wakeup_fd_error.patch

messages: + msg195294
versions: - Python 2.6, Python 2.7, Python 3.3
2012-10-16 20:22:17felipecruzsetfiles: + issue16105_v4.patch

messages: + msg173091
2012-10-04 23:55:36felipecruzsetfiles: + issue16105_v3.patch

messages: + msg172030
2012-10-04 21:34:57neologixsetmessages: + msg171997
2012-10-04 02:14:57felipecruzsetfiles: + issue16105_v2.patch

messages: + msg171917
2012-10-03 18:19:19neologixsetmessages: + msg171901
2012-10-03 14:23:17felipecruzsetmessages: + msg171888
2012-10-03 08:40:39neologixsetmessages: + msg171872
2012-10-03 07:55:01hayposetmessages: + msg171865
2012-10-03 07:04:53hayposetmessages: + msg171861
2012-10-03 05:51:51neologixsetmessages: + msg171854
2012-10-03 05:43:28neologixsetmessages: + msg171853
2012-10-03 01:55:11felipecruzsetfiles: + issue16105_v1.patch
keywords: + patch
messages: + msg171847
2012-10-03 00:05:48pitrousetnosy: + pitrou
messages: + msg171841
2012-10-02 07:55:55neologixsetnosy: + neologix
messages: + msg171776
2012-10-02 02:05:10felipecruzsetmessages: + msg171770
2012-10-01 23:26:29hayposetmessages: + msg171760
2012-10-01 22:15:07hayposetnosy: + haypo
2012-10-01 21:05:59felipecruzcreate