Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
[3.12]: gh-113055: Use pointer for interp->obmalloc state#118618
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
For interpreters that share state with the main interpreter, this pointsto the same static memory structure. For interpreters with their ownobmalloc state, it is heap allocated. Add free_obmalloc_arenas() whichwill free the obmalloc arenas and radix tree structures for interpreterswith their own obmalloc state.
nascheme commentedMay 6, 2024
This unfortunately causes The crash error is "double free or corruption" and the backtrace shows |
neonene commentedMay 6, 2024
It looks like L640 and L2062 in |
neonene commentedMay 6, 2024
Leaving a patch just in case. Not sure if it's correct, but at least avoids the Detailsdiff --git a/Python/pylifecycle.c b/Python/pylifecycle.cindex fb833ba..31a24d4 100644--- a/Python/pylifecycle.c+++ b/Python/pylifecycle.c@@ -637,13 +637,6 @@ pycore_create_interpreter(_PyRuntimeState *runtime, return status; }- // initialize the interp->obmalloc state. This must be done after- // the settings are loaded (so that feature_flags are set) but before- // any calls are made to obmalloc functions.- if (_PyMem_init_obmalloc(interp) < 0) {- return _PyStatus_NO_MEMORY();- }- /* Auto-thread-state API */ status = _PyGILState_Init(interp); if (_PyStatus_EXCEPTION(status)) {@@ -658,6 +651,13 @@ pycore_create_interpreter(_PyRuntimeState *runtime, return status; }+ // initialize the interp->obmalloc state. This must be done after+ // the settings are loaded (so that feature_flags are set) but before+ // any calls are made to obmalloc functions.+ if (_PyMem_init_obmalloc(interp) < 0) {+ return _PyStatus_NO_MEMORY();+ }+ PyThreadState *tstate = _PyThreadState_New(interp); if (tstate == NULL) { return _PyStatus_ERR("can't make first thread");@@ -2059,14 +2059,6 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) return _PyStatus_OK(); }- // initialize the interp->obmalloc state. This must be done after- // the settings are loaded (so that feature_flags are set) but before- // any calls are made to obmalloc functions.- if (_PyMem_init_obmalloc(interp) < 0) {- status = _PyStatus_NO_MEMORY();- goto error;- }- PyThreadState *tstate = _PyThreadState_New(interp); if (tstate == NULL) { PyInterpreterState_Delete(interp);@@ -2110,6 +2102,14 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) goto error; }+ // initialize the interp->obmalloc state. This must be done after+ // the settings are loaded (so that feature_flags are set) but before+ // any calls are made to obmalloc functions.+ if (_PyMem_init_obmalloc(interp) < 0) {+ status = _PyStatus_NO_MEMORY();+ goto error;+ }+ status = init_interp_create_gil(tstate, config->gil); if (_PyStatus_EXCEPTION(status)) { goto error; |
nascheme commentedMay 6, 2024
Thanks@neonene, I think your fix is correct. When I merged then those calls got moved. They need to be done after the settings are loaded and feature flags are set. Maybe that was a problem. After moving the calls as you suggest, all the unit tests pass. |
nascheme commentedMay 6, 2024
There is a failing check about the ABI changing. The PyThreadState structure has indeed changed. due to make the obmalloc state a pointer rather than an embedded structure. I think it might be okay because it seems the |
ericsnowcurrently commentedMay 6, 2024
The ABI check (which is only in non-main branches) currently catches changes to internal ABI (e.g. You can regenerate the file yourself (perhttps://devguide.python.org/setup/#regenerate-the-abi-dump) or you can copy the data file that was generated when the check ran. It's the "abi-data" artifact onhttps://github.com/python/cpython/actions/runs/8972849336?pr=118618 (the PR actions "Summary" page). Replace Doc/data/python3.12.abi with the artifact. |
obaterspok commentedMay 7, 2024
@nascheme, I did report this issue for kodi (xbmc/xbmc#24440) and I have just verified the PR fixes the kodi crash that occurred during exit. |
nascheme commentedMay 7, 2024
Output of abidiff attached. I believe the |
nascheme commentedMay 27, 2024
ping@Yhg1s |
Yhg1s commentedJun 4, 2024
I'm uncomfortable with this backport, more for its size and impact than the ABI difference. (Ithink the ABI difference is fine, although it may make debuggers' lives harder.) I'm not sure the bugs it fixes in subinterpreter usage warrant the risk of the change, but I can be persuaded otherwise. |
dguglielmi commentedJun 12, 2024
Tested on Gentoo Linux with dev-lang/python-3.12.3 (cpython), works well! @Yhg1s : To try to convince you, it's a rather annoying bug for Kodi users. I understand that it probably doesn't represent a large number of users (compared to the overall use of CPython), but for them, it creates a crash report file every time Kodi is shut down when they encounter this issue. |
bacon-cheeseburger commentedJun 12, 2024
There may be more Python users overall but the Kodi user base numbers in the millions across multiple platforms. That's not insignificant. I'd think debuggers, IF this fix makes anything harder for them, would certainly be more capable of dealing with it than regular users. A simple search relating to this problem easily warrants giving the fix serious consideration. However, if the motivation to do something about it isn't there then ... |
nascheme commentedJun 12, 2024
Perhaps there is a way to avoid the Kodi crash with a smaller and simpler for to 3.12.x. I can investigate that. Thomas is correct in that this PR is a fairly big change to merge as a backport. |
trygveaa commentedJun 14, 2024
Sorry I'm a bit late with testing this PR, but I can confirm it fixes the crash in WeeChat as well. |
graysky2 commentedJun 23, 2024
@nascheme - I think this needs a rebase to apply. |
nascheme commentedJul 3, 2024
Closing since we are not planning to backport this change. |
graysky2 commentedJul 3, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nascheme - is there a work-around that has not yet made it into a tagged release? Formy issue of kodi crashing on exit, applying this PR to 3.12.4 fixes the issue. Running 3.12.4 unmodified triggers the crash. |
nascheme commentedJul 3, 2024
We don't have a work-around. I can investigate the kodi crash and see if I can figure out why it happens. |
graysky2 commentedJul 3, 2024
Thanks@nascheme - beyond in the info in the linked bug report (some crash dumps and dmesg output) please let me know if I can provide you with anything or test a patch etc. |
nascheme commentedJul 3, 2024
I cannot comment on the gitlab bug (account registration closed). Based on that call stack dump, I would guess it's a similar problem to what WeeChat is encountering. The traceback is starting from |
graysky2 commentedJul 4, 2024
Thanks@nascheme - implementing what you're asking is beyond my skill set (basic shell scripting only). |
dguglielmi commentedJul 4, 2024
It seems that a solution has been found to prevent the problem in Kodi. |
graysky2 commentedJul 4, 2024
Thanks for pointing that out,@dguglielmi |
graysky2 commentedJul 4, 2024
To update - building Kodi with that commit applied does not fix the issue. |
hbiyik commentedJul 12, 2024
@graysky2 at this point it would make sense to depend kodi on python311 at least on AUR, not only enter/exit crashes also a lot of memory leaks are happening with python3.12 |
graysky2 commentedJul 12, 2024
No, because kodi is in [extra] and cannot have any depends that are not in the official repos. |
hbiyik commentedJul 12, 2024
yeah, sure i meant the kodi-stable-git in AUR, sorry too much valgrind. trying this approach now at least it is a workaround until python fixes its own stuff. |
hbiyik commentedJul 12, 2024
here is a diff for diff --git a/PKGBUILD b/PKGBUILDindex 8c49304..4882273 100644--- a/PKGBUILD+++ b/PKGBUILD@@ -25,7 +25,7 @@ _renderer=gles pkgbase=kodi-stable-git pkgname=("$pkgbase" "$pkgbase-eventclients" "$pkgbase-tools-texturepacker" "$pkgbase-dev")-pkgver=r65506.24278a28fb6+pkgver=r65642.7e5bb325bda pkgrel=1 arch=('x86_64') url="https://kodi.tv"@@ -47,6 +47,7 @@ makedepends=( 'wayland-protocols' 'waylandpp' 'libxkbcommon' # gbm 'libinput'+ 'python311' ) options=(!lto)@@ -166,6 +167,7 @@ build() { -DENABLE_INTERNAL_FSTRCMP=ON -DENABLE_INTERNAL_FLATBUFFERS=ON -DENABLE_INTERNAL_UDFREAD=ON+ -DPYTHON_VER=3.11 -Dlibdvdcss_URL="$srcdir/libdvdcss-$_libdvdcss_version.tar.gz" -Dlibdvdnav_URL="$srcdir/libdvdnav-$_libdvdnav_version.tar.gz" -Dlibdvdread_URL="$srcdir/libdvdread-$_libdvdread_version.tar.gz"@@ -193,7 +195,7 @@ package_kodi-stable-git() { 'mariadb-libs' 'mesa' 'libpipewire' 'python-pillow' 'python-pycryptodomex' 'python-simplejson' 'shairplay' 'smbclient' 'sndio' 'spdlog' 'sqlite' 'taglib' 'tinyxml' 'libxrandr' 'libxkbcommon' 'waylandpp' 'libinput'- 'pcre' 'tinyxml2' 'libdisplay-info'+ 'pcre' 'tinyxml2' 'libdisplay-info' 'python311' ) [[ -n "$_clangbuild" ]] && depends+=('glu') everything is back to normal again, sorry if this sidetracking the original issue.. |
encukou commentedSep 23, 2024
I talked to Thomas, Eric and Neil.
Is that right? If not, let's get everyone on the same page. |
graysky2 commentedSep 24, 2024
@encukou - that is my understand. Python 3.12.6 for Kodi is still affected. |
Uh oh!
There was an error while loading.Please reload this page.
A backport of this change has been requested by a some users. SeeGh-113412 comments.
Crash in WeeChat which might be fixed by this:gh-116510
Comment from@bacon-cheeseburger: