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.

Sign up for GitHub

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

Jump to bottom

Fix segfault with error report in FITS compression #15489

Merged
merged 3 commits into from Oct 19, 2023
Merged

Fix segfault with error report in FITS compression #15489

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4de4877
Fix segfault with error report in FITS compression
saimn Oct 16, 2023
4554f28
Added a regression test for segfaults when there are issues with tiles
astrofrog Oct 19, 2023
c51b1af
Make sure that a CfitsioException is raised
astrofrog Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions astropy/io/fits/hdu/compressed/src/compression.c
View file
Open in desktop
Expand Up @@ -90,7 +90,10 @@ PyInit__compression(void)
// cfitsio call. In our wrapper functions we can then check if the Python error
// state is set and then return NULL to raise the error.
void ffpmsg(const char *err_message) {
PyGILState_STATE gstate;
gstate = PyGILState_Ensure();
PyErr_SetString(CfitsioException, err_message);
PyGILState_Release(gstate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assuming that we are always releasing the GIL when calling out to cfitsio? If for some reason that were to change would it explode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void PyGILState_Release(PyGILState_STATE)
Part of the Stable ABI.
Release any resources previously acquired. After this call, Python’s state will be the same as it was prior to the corresponding PyGILState_Ensure() call (but generally this state will be unknown to the caller, hence the use of the GILState API).

So should be fine ?
https://docs.python.org/3/c-api/init.html#c.PyGILState_Release
But this will not work with subinterpreters (https://docs.python.org/3/c-api/init.html#non-python-created-threads).
The other option would be to stop releasing the GIL before calling cfitsio, not sure what the impact is ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not releasing the GIL would mean no longer being able to do multithreaded decompression of tiles which does work at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess the fix here is fine for now, but we may have to reconsider when subinterpreters become a thing (planned for 3.13) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need to call Python code from these threads (often this will be part of a callback API provided by the aforementioned third-party library), you must first register these threads with the interpreter by creating a thread state data structure, then acquiring the GIL, and finally storing their thread state pointer, before you can start using the Python/C API. When you are done, you should reset the thread state pointer, release the GIL, and finally free the thread state data structure.

Nope, I am out lol too much C 🤣

}

/* PLIO/IRAF compression */
Expand Down
1 change: 1 addition & 0 deletions docs/changes/io.fits/15489.bugfix.rst
View file
Open in desktop
@@ -0,0 +1 @@
Fix segfault with error report in tile decompression.