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

[BUG] Refnanny + aiohttp + Fedora 36 = Segmentation fault #5495

Closed
socketpair opened this issue Jun 20, 2023 · 13 comments
Closed

[BUG] Refnanny + aiohttp + Fedora 36 = Segmentation fault #5495

socketpair opened this issue Jun 20, 2023 · 13 comments

Comments

@socketpair
Copy link

Describe the bug

Segmentation fault in code generated by Cython

Code to reproduce the behaviour:

podman run \
--net=host \
-it fedora:36 \
/bin/sh -c 'dnf install -y python3-aiohttp glibc-langpack-ru && export LC_ALL=ru_RU.KOI8-R; exec python3 -c "from aiohttp import web; web.run_app(web.Application(), port=1234)"'

Make request to http://localhost:1234 - and it will die with segmentation fault. -DCYTHON_REFNANNY gives:

aiohttp/_helpers.c: __get__()
REFNANNY: NULL argument on line 1674
REFNANNY: NULL argument on line 1726
aiohttp/_helpers.c: __get__()
REFNANNY: NULL argument on line 1674
REFNANNY: NULL argument on line 1726
aiohttp/_helpers.c: __get__()
REFNANNY: NULL argument on line 1674
REFNANNY: NULL argument on line 1726
aiohttp/_helpers.c: __get__()
REFNANNY: NULL argument on line 1674
REFNANNY: NULL argument on line 1726
aiohttp/_helpers.c: __get__()
REFNANNY: NULL argument on line 1674
REFNANNY: NULL argument on line 1726

And according to GDB, yes, it fails on __Pyx_DECREF(__pyx_t_10) (null pointer dereference)

It is not reproduced in Fedora 37. Yes. But I need to know if the bug was fixed.

Possibly Python does not return NULL on broken locale, who knows. But I think Cython does not check for NULL where it should. What I mean, another error may appear again in this place under different conditions (not broken locale)

Expected behaviour

No response

OS

Linux

Python version

Python 3.10.7

Cython version

0.29.26

Additional context

No response

@da-woods
Copy link
Contributor

I think installing a podman+fedora in a language I don't know is probably more debugging that I'm personally able to do.

It'd be useful if you could show what's in lines 1670-1730 of _helpers.c just so we can get a little context.

My guess (from compiling https://github.com/aio-libs/aiohttp/blob/master/aiohttp/_helpers.pyx myself) is that __pyx_t_10 is probably supposed to be a traceback. But I can't get the line numbers to match exact. If that's the case, I suspect __Pyx_AddTraceback has failed (which it can do, silently). We possibly need to treat all traceback's a potentially NULL.

@socketpair
Copy link
Author

socketpair commented Jun 21, 2023

@da-woods, No problem:
_helpers.c.gz

@scoder
Copy link
Contributor

scoder commented Jun 21, 2023 via email

@socketpair
Copy link
Author

@scoder I can. But , seems someone (Cython or Python) does not honour contract exception handling case. I think the problem will be the same. I need some time because I need to recompile the package with custom Cython version.

I don't think this place was changed in newer Cython.

@socketpair
Copy link
Author

image
image

Flamegraphs, so you can guess what has happened

@socketpair
Copy link
Author

socketpair commented Jun 21, 2023

unicode_decode_locale, I guess, has failed. and some code does not check for error in this place. Or, checks, but Cython maybe is not aware of.

@socketpair
Copy link
Author

socketpair commented Jun 21, 2023

image

https://docs.python.org/3/c-api/exceptions.html#c.PyException_GetTraceback

Seems it's the case when NULL is returned, but Cython code thinks it can not be NULL

@scoder @da-woods

@socketpair
Copy link
Author

__Pyx_GetException may return Success (>=0) and write NULL to the last argument (PyObject **tb).

Next,

          if (__Pyx_GetException(&__pyx_t_8, &__pyx_t_7, &__pyx_t_10) < 0) __PYX_ERR(0, 25, __pyx_L11_except_error)
          __Pyx_GOTREF(__pyx_t_8);
          __Pyx_GOTREF(__pyx_t_7);
          __Pyx_GOTREF(__pyx_t_10);

__Pyx_GOTREF thinks it is never NULL. this is the bug. Later it tries to do __Pyx_DECREF where it makes segfault

@socketpair
Copy link
Author

I don't know how to fix, I'm not familiar with Cython structures

@socketpair
Copy link
Author

Seems, other return values for __Pyx_GetException also can be NULL. please recheck.

@da-woods
Copy link
Contributor

I had a quick look at the code you sent this morning and that was basically my conclusion too. The reason we're assuming the traceback isn't null is because we set the traceback. However that can fail so we shouldn't assume the traceback is null.

The other two values are definitely set in this case though so aren't an issue.

@scoder
Copy link
Contributor

scoder commented Jun 21, 2023

I'm sure I worked on this at some point. Let me see if I find the branch or changes somewhere.

@scoder scoder closed this as completed in 8d54a67 Jun 21, 2023
@scoder
Copy link
Contributor

scoder commented Jun 21, 2023

I'm sure I worked on this at some point. Let me see if I find the branch or changes somewhere.

… ok, I failed to find it. But this seems a simple fix.

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

No branches or pull requests

3 participants