Auto-remove custom callables also when connected in untyped APIs#1612
Merged
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1612 |
Bromeon
force-pushed
the
bugfix/custom-callable-hot-reload
branch
from
3834f1d to
a2f4340
Compare
Bromeon
force-pushed
the
bugfix/custom-callable-hot-reload
branch
from
c17596f to
6a7903c
Compare
Member
Author
|
I found a way to recognize Rust callables, which gives us a relatively good heuristic: we auto-disconnect common cases ( On the bright side, we don't touch legitimate Godot callables during hot-reload. |
Contributor
|
|
Yarwin
reviewed
May 24, 2026
Yarwin
reviewed
May 24, 2026
| // Rust callable | yes | yes | yes <- registered, disconnected before reload | ||
| // Rust callable + bind/bindv | yes | no | yes <- MISSED, see below | ||
| // Rust callable, other extension | yes | no | no <- other extension takes care of it | ||
| // Engine method | no | no | no |
Contributor
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment.
Good point, it also applies to user-defined #[func].
Bromeon
force-pushed
the
bugfix/custom-callable-hot-reload
branch
from
6a7903c to
2851e37
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Bromeon
force-pushed
the
bugfix/custom-callable-hot-reload
branch
from
2851e37 to
063daa0
Compare
A custom Rust callable connected to a signal survives hot-reload, but Godot runs into UAF the next time it accesses it. There is already `Callable::from_linked_fn` to mitigate this problem. This change extends the signal connection registry to also track custom callables registered in untyped `Object::connect*`/`Signal::connect*` calls, auto-disconnecting them before hot-reload. Engine-side (non-custom) callables are not affected by this.
Bromeon
force-pushed
the
bugfix/custom-callable-hot-reload
branch
from
063daa0 to
44bc5cd
Compare
Hot-reload registry for signal-connection: instead of broad `is_custom()`, we can narrow down the affected Callables to reduce some false positives (while also adding a few false negatives).
Bromeon
force-pushed
the
bugfix/custom-callable-hot-reload
branch
from
44bc5cd to
7109b3f
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fixes #984.
@Yarwin already laid out the groundwork in #1223. This extends the auto-disconnection to untyped signal connections via
Object::connect*andSignal::connect*APIs.Custom callables can be Rust-based closures, but also just any callables that were wrapped with
bind()/bindv(), and also GDScript lambdas. As such, it's inevitable that this approach has some false positives. Concretely, some callables may not survive hot-reload even though they would be safe -- on the other hand, we prevent UB for Rust callables.Now I'm not 100% sure if this is the right trade-off, some considerations:
Object::connectthen works slightly differently thanObject.connectin GDScript/reflection calls (although it's just an extra best-effort check)Maybe there's a better way to recognize Rust callables, heuristic based on their string representation, or trying to interpret the
user_data(might also be UB though 🤔 )