This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Signal handlers must preserve errno
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, exarkun, hfuru, loewis, pitrou
Priority: normal Keywords: patch

Created on 2010-11-04 12:18 by hfuru, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
signalmodule-errno.diff hfuru, 2010-11-04 12:18 patch: save/restore errno in signal handler
Messages (12)
msg120395 - (view) Author: Hallvard B Furuseth (hfuru) Date: 2010-11-04 12:18
Signal handlers that can change errno, must restore it.
I enclose a patch for <2.7, 3.2a3>/Modules/signalmodule.c
which also rearranges the code to make this a bit easier.

The patch does   if (errno != save_errno) errno = save_errno;
instead of just  errno = save_errno;
in case it's less thread-safe to write than to read errno,
which would not surprise me but may be pointless paranoia.

I don't know what needs to be done on non-Unix systems,
like Windows' WSAError stuff.
msg120397 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-04 12:24
This is a good idea IMO. It would be better if you minimized style changes, so that the patch is easier to review.
Also, is the while loop around write() necessary here?
msg120400 - (view) Author: Hallvard B Furuseth (hfuru) Date: 2010-11-04 12:51
Parser/intrcheck.c:intcatcher() should do the same.  Covered in Issue
10312.

Antoine Pitrou writes:
> This is a good idea IMO. It would be better if you minimized style
> changes, so that the patch is easier to review.

I'm afraid the un-rearranged code would be fairly ugly, so I cleaned
up first.  Single exit point.

> Also, is the while loop around write() necessary here?

Whoops, I'd forgotten I did that too, it was on my TODO list to
check if Python makes it unnecessary by making write restartable.
I don't remember if that's possible to ensure on all modern Posixes
msg120403 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-11-04 13:41
This issue is not really relevant on Windows:
- signals are actually run in a new thread specially created.
- errno is a thread-local variable; its value is thus local to the signal handler, same for WSAGetLastError().
msg120405 - (view) Author: Hallvard B Furuseth (hfuru) Date: 2010-11-04 13:45
Amaury Forgeot d'Arc writes:
> This issue is not really relevant on Windows:
> - signals are actually run in a new thread specially created.
> - errno is a thread-local variable; its value is thus local to the
>   signal handler, same for WSAGetLastError().

Nice.  Then I suggest a config macro for whether this is needed.
Either a test for windows, or an autoconf thing in case some Unixes
are equally sensible.  (Linux isn't, I checked.)
msg120406 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-11-04 13:57
> Nice.  Then I suggest a config macro for whether this is needed.
> Either a test for windows, or an autoconf thing in case some Unixes
> are equally sensible.  (Linux isn't, I checked.)

I'm quite sure that all Unixes invoke signal handlers in some existing thread. So even if errno is thread-local, it needs to be saved and restored.
OTOH this is really a micro optimization.
msg120409 - (view) Author: Hallvard B Furuseth (hfuru) Date: 2010-11-04 14:36
Amaury Forgeot d'Arc writes:
> OTOH this is really a micro optimization.

["this" = only saving/restoring errno when needed]
True, but practically nothing is officially safe to do in signal
handlers, so it's good to avoid code which can be avoided there.
If one can be bothered to, that is.
msg120412 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-04 14:50
> ["this" = only saving/restoring errno when needed]
> True, but practically nothing is officially safe to do in signal
> handlers, so it's good to avoid code which can be avoided there.
> If one can be bothered to, that is.

I think it is extremely unlikely that mutating errno in a signal handler is unsafe (after all, the library functions called from that handler can mutate errno too: that's the whole point of the patch IIUC). Adding some configure machinery for this seems unwarranted to me.
msg120413 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-04 14:55
By the way, I'd like to clear out a potential misunderstanding: the function you are patching doesn't call Python signal handlers in itself (those registered using signal.signal()). It only schedules them for later execution. If you want to save errno around Python signal handlers themselves, PyErr_CheckSignals must be patched as well.
msg120479 - (view) Author: Hallvard B Furuseth (hfuru) Date: 2010-11-05 09:49
Antoine Pitrou writes:
> By the way, I'd like to clear out a potential misunderstanding: the
> function you are patching doesn't call Python signal handlers in itself
> (those registered using signal.signal()). (...)

Good point - I'm talking C signal handlers, not Python signal handlers.

> If you want to save errno around Python signal handlers
> themselves, PyErr_CheckSignals must be patched as well.

Probably not.  I don't know Python's errno conventions, if any, but
it's usually a bug to use errno at any distance from the error.  The C
code near the error can save errno if it wants it later.  It can't do
that around C signal handlers, since those can get called anywhere.
msg120481 - (view) Author: Hallvard B Furuseth (hfuru) Date: 2010-11-05 10:07
Antoine Pitrou writes:
> I think it is extremely unlikely that mutating errno in a signal handler
> is unsafe (after all, the library functions called from that handler can
> mutate errno too: that's the whole point of the patch IIUC). Adding some
> configure machinery for this seems unwarranted to me.

Fine by me.
msg120528 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-11-05 19:55
Ok, fixed in r86214 (3.x), r86215 (3.1) and r86216 (2.7). Thanks for the patch!
History
Date User Action Args
2022-04-11 14:57:08adminsetgithub: 54520
2010-11-05 19:55:49pitrousetstatus: open -> closed
resolution: fixed
messages: + msg120528

stage: patch review -> resolved
2010-11-05 10:07:38hfurusetmessages: + msg120481
2010-11-05 09:49:55hfurusetmessages: + msg120479
2010-11-04 14:55:35pitrousetmessages: + msg120413
2010-11-04 14:50:06pitrousetmessages: + msg120412
2010-11-04 14:36:50hfurusetmessages: + msg120409
2010-11-04 13:57:13amaury.forgeotdarcsetmessages: + msg120406
2010-11-04 13:45:02hfurusetmessages: + msg120405
2010-11-04 13:41:23amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg120403
2010-11-04 12:51:50hfurusetmessages: + msg120400
2010-11-04 12:24:05pitrousetversions: + Python 3.1, Python 2.7
nosy: + exarkun, loewis, pitrou

messages: + msg120397

stage: patch review
2010-11-04 12:18:17hfurucreate