Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal safety #163

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Signal safety #163

merged 2 commits into from
Oct 4, 2023

Conversation

tornaria
Copy link
Contributor

Follow up to #162.

The first commit gets rid of all usage of stdio in signal handlers (mostly, but not only, inside ENABLE_DEBUG_CYSIGNALS).

The second commit replaces use of gettimeofday() by clock_gettime(), the latter being safe according to signal-safety(7). It's also monotonic, unlike gettimeofday().

Other stuff that happens inside the signal handler:

  • cysigs_interrupt_handler() calls PyErr_SetInterrupt() (probably ok?).

  • when debug_level >= 2 the function do_raise_exception() calls PyGILState_Ensure / PyErr_Occurred / PyGILState_Release, I wonder if this could deadlock for some reason; in any case I don't understand why the GIL is necessary if the return value of PyErr_Occurred() is not used, just printed.

  • function sigdie() uses getenv() which is not safe, maybe the env variables should be read by init_cysignals() instead.

  • function print_enhanced_backtrace() uses execvp(), this is probably ok although the "correct" way would be to read PATH on init and here use execv() with the full path to cysignals-CSI already precomputed.

  • the function sig_raise_exception() is in cython and calls lots of python stuff, I wonder if this should be called after the longjmp() instead (it probably makes no difference since I don't think longjmp() will fix internal state of e.g. stdio).

@mkoeppe
Copy link

mkoeppe commented Sep 21, 2023

Rebased, to run the current tests

@mkoeppe
Copy link

mkoeppe commented Oct 3, 2023

@malb @dimpase should this be merged? It seems to pass all tests.
@tornaria Are you using these fixes in production already?

@tornaria
Copy link
Contributor Author

tornaria commented Oct 4, 2023

@malb @dimpase should this be merged? It seems to pass all tests. @tornaria Are you using these fixes in production already?

I'm only using #162 (see https://github.com/void-linux/void-packages/tree/master/srcpkgs/python3-cysignals/patches). This is the only issue that I know how to reproduce. This PR "fixes" potential issues of the same kind that I don't know how to reproduce.

Note that #162 is merged but never released. It would be nice to have a new release which fixes this issue and fully supports cython 3. The easy way to support cython 3 is to use legacy_implicit_noexcept but a proper fix is to actually add all the noexcept clauses so the code builds fine with cython 0.29 or cython 3.

@dimpase
Copy link
Member

dimpase commented Oct 4, 2023

rebased over the latest main, to see how it goes with CI

@dimpase dimpase merged commit 820d36a into sagemath:main Oct 4, 2023
5 checks passed
@tornaria tornaria deleted the signal-safety branch October 4, 2023 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants