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

Prevent a crash when calling AnimationMixer::restore with an invalid resource #85428

Conversation

jsjtxietian
Copy link
Contributor

Fixes #85340

@jsjtxietian jsjtxietian requested a review from a team as a code owner November 27, 2023 11:21
@AThousandShips
Copy link
Member

AThousandShips commented Nov 27, 2023

Is this actually likely to happen when the engine calls this method? This is not a part of the exposed API and users providing invalid input is not necessarily a priority, though the fix is cheap (this method can't even really be called from GDScript as the type isn't exposed or functional from that side)

@jsjtxietian
Copy link
Contributor Author

This is not a part of the exposed API and users providing invalid input is not necessarily a priority, though the fix is cheap

I believe it's not that high priority.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 27, 2023

A note for a better commit message: describe what problem is addressed rather than how you address it. The how is already obvious from the change. So the message could be "Prevent a crash when calling AnimationMixer::restore when an invalid reference".

@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 27, 2023
@YuriSizov YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 27, 2023
@TokageItLab TokageItLab requested a review from qarmin November 27, 2023 11:27
scene/animation/animation_mixer.cpp Outdated Show resolved Hide resolved
@qarmin
Copy link
Contributor

qarmin commented Nov 27, 2023

@AThousandShips it is exposed to gdscript - just look at the issue.

I remember that once there was a discussion about the fact that from GDScript you can call private api and the conclusion was that is not possible to fix the problem(at that moment), so nothing was done about it(there is definitely an issue about it somewhere)

@AThousandShips
Copy link
Member

AThousandShips commented Nov 27, 2023

It isn't part of the exposed API, there's a difference, a significant one

The method can't be called properly ever as it's not a class that's available to GDScript so all this solves is a crash when using this method incorrectly

Worth it but not a part of the exposed API

@jsjtxietian jsjtxietian force-pushed the prevent-nullptr-crash-in-AnimationMixer--restore branch 2 times, most recently from 8e9ab0f to 707e262 Compare November 28, 2023 04:53
@jsjtxietian jsjtxietian force-pushed the prevent-nullptr-crash-in-AnimationMixer--restore branch from 707e262 to 937411e Compare November 28, 2023 04:54
@AThousandShips AThousandShips changed the title Use ERR_FAIL_NULL to prevent crash when p_backup is nullptr Prevent a crash when calling AnimationMixer::restore with an invalid resource Nov 28, 2023
@akien-mga akien-mga merged commit 6b21a18 into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@jsjtxietian jsjtxietian deleted the prevent-nullptr-crash-in-AnimationMixer--restore branch December 9, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Executing AnimationPlayer._restore function crashes Godot
5 participants