Fix window ub/segfault when quit/destroyed #3606
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughAdds weak-reference support and lifecycle tracking to the Window type. Introduces a global weak-ref set, a custom tp_new, destroyed-state guards, and revised deallocation. Header gains weakref fields; type exposes tp_weaklistoffset. Module init/quit manage the weak-ref set and clean up remaining windows on quit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python code
participant WT as WindowType
participant W as Window instance
participant WR as pg_window_weakrefs
Note over WT,W: Creation with weak-ref registration
Py->>WT: WT.__new__()
WT->>WT: window_new(subtype,args,kwds)
WT->>W: allocate & init fields
WT->>WR: add weakref(W)
WT-->>Py: return W
Note over Py,W: Method call guarded against destroyed state
Py->>W: some_window_method()
W->>W: WINDOW_INIT_CHECK(self->_win)
alt window destroyed
W-->>Py: return error/None per method
else window alive
W->>W: proceed with operation
W-->>Py: return result
end
sequenceDiagram
autonumber
participant Py as Python code
participant Mod as _window module
participant WR as pg_window_weakrefs
participant W as Window instance
Note over Mod,WR: Module quit cleanup
Py->>Mod: _window_internal_mod_quit()
Mod->>WR: iterate weak refs
loop each live Window
WR->>W: destroy via window_destroy()
W->>W: clear selfweakref / weakreflist
end
Mod->>WR: clear & dealloc set
Mod-->>Py: return
Note over W: Deallocation
Py->>W: GC/dealloc
W->>W: window_dealloc()
W->>W: window_destroy()
W->>WR: remove weakref
W->>W: clear weakref list
W-->>Py: memory freed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src_c/include/_pygame.h(1 hunks)src_c/window.c(37 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src_c/window.c (2)
src_c/include/pythoncapi_compat.h (1)
PyWeakref_GetRef(570-590)src_c/base.c (1)
pg_mod_autoinit(137-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: AMD64
- GitHub Check: x86
Comment on lines
993
to
1009
| window_dealloc(pgWindowObject *self, PyObject *_null) | ||
| { | ||
| if (self->_win) { | ||
| if (!self->_is_borrowed) { | ||
| if (self->context != NULL) { | ||
| SDL_GL_DeleteContext(self->context); | ||
| } | ||
| SDL_DestroyWindow(self->_win); | ||
| } | ||
| else if (pg_get_pg_window(self->_win) != NULL) { | ||
| pg_set_pg_window(self->_win, NULL); | ||
| Py_XDECREF(window_destroy(self, _null)); | ||
|
|
||
| if (self->selfweakref) { | ||
| /* First attempt to remove it from our set (errors ignored). Then | ||
| * free the weakref */ | ||
| if (pg_window_weakrefs) { | ||
| PySet_Discard(pg_window_weakrefs, self->selfweakref); | ||
| } | ||
| Py_DECREF(self->selfweakref); | ||
| } | ||
| if (self->surf) { | ||
| // Set the internal surface to NULL to make pygame surface invalid | ||
| // since this surface will be deallocated by SDL when the window is | ||
| // destroyed. | ||
| self->surf->surf = NULL; | ||
|
|
||
| Py_DECREF(self->surf); | ||
| if (self->weakreflist) { | ||
| PyObject_ClearWeakRefs((PyObject *)self); | ||
| } | ||
|
|
||
| Py_TYPE(self)->tp_free(self); |
There was a problem hiding this comment.
Fix tp_dealloc signature mismatch.
tp_dealloc is invoked with a single PyObject *. Defining window_dealloc with two parameters means CPython will call it with only the first argument, leaving _null as indeterminate data. That is undefined behaviour and can corrupt the stack on some ABIs. Switch back to the single-argument signature and call window_destroy with an explicit NULL.
-static void
-window_dealloc(pgWindowObject *self, PyObject *_null)
+static void
+window_dealloc(pgWindowObject *self)
{
- Py_XDECREF(window_destroy(self, _null));
+ Py_XDECREF(window_destroy(self, NULL));
if (self->selfweakref) {
/* First attempt to remove it from our set (errors ignored). Then
* free the weakref */
if (pg_window_weakrefs) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| window_dealloc(pgWindowObject *self, PyObject *_null) | |
| { | |
| if (self->_win) { | |
| if (!self->_is_borrowed) { | |
| if (self->context != NULL) { | |
| SDL_GL_DeleteContext(self->context); | |
| } | |
| SDL_DestroyWindow(self->_win); | |
| } | |
| else if (pg_get_pg_window(self->_win) != NULL) { | |
| pg_set_pg_window(self->_win, NULL); | |
| Py_XDECREF(window_destroy(self, _null)); | |
| if (self->selfweakref) { | |
| /* First attempt to remove it from our set (errors ignored). Then | |
| * free the weakref */ | |
| if (pg_window_weakrefs) { | |
| PySet_Discard(pg_window_weakrefs, self->selfweakref); | |
| } | |
| Py_DECREF(self->selfweakref); | |
| } | |
| if (self->surf) { | |
| // Set the internal surface to NULL to make pygame surface invalid | |
| // since this surface will be deallocated by SDL when the window is | |
| // destroyed. | |
| self->surf->surf = NULL; | |
| Py_DECREF(self->surf); | |
| if (self->weakreflist) { | |
| PyObject_ClearWeakRefs((PyObject *)self); | |
| } | |
| Py_TYPE(self)->tp_free(self); | |
| static void | |
| window_dealloc(pgWindowObject *self) | |
| { | |
| Py_XDECREF(window_destroy(self, NULL)); | |
| if (self->selfweakref) { | |
| /* First attempt to remove it from our set (errors ignored). Then | |
| * free the weakref */ | |
| if (pg_window_weakrefs) { | |
| PySet_Disard(pg_window_weakrefs, self->selfweakref); | |
| } | |
| Py_DECREF(self->selfweakref); | |
| } | |
| if (self->weakreflist) { | |
| PyObject_ClearWeakRefs((PyObject *)self); | |
| } | |
| Py_TYPE(self)->tp_free(self); | |
| } |
🤖 Prompt for AI Agents
In src_c/window.c around lines 993 to 1009, the tp_dealloc implementation uses a
two-argument signature window_dealloc(pgWindowObject *self, PyObject *_null)
which is incorrect because CPython will call tp_dealloc with a single PyObject*,
leaving the second parameter indeterminate; change the function to the correct
single-argument signature window_dealloc(pgWindowObject *self), update the body
to call window_destroy(self, NULL) instead of passing _null, and keep the rest
of the cleanup (weakref set removal, clearing weak refs, and tp_free) unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR to fix #3601
The central issue here is that when
pygame.quit/pygame.display.quitis called, all pygame window objects may hold invalid references in their attributes (like_winandsurface). It therefore becomes necessary to do the equivalent of callingwindow.destroyon all active windows before doing the quit.For this implementation, I went with a weakref approach where each window holds a self weakref upon creation. A new reference to this weakref is kept in a global set of weakrefs. Cleanup can happen in two paths:
This PR also adds a "window is alive" explicit check to all window methods/properties for consistent error raising.
tests will be added soon:tm: