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

Fix physics platform behaviour regression #97315

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Sep 22, 2024

Lifetime checks for stored RIDs for collision objects assumed they had valid object_ids. It turns out that some are not derived from Object and thus checking ObjectDB returns false for some valid RIDs. To account for this we only perform lifetime checks on valid object_ids.

Fixes #97293

Discussion

Although the original MRP in #74732 had valid object ids, it turns out that physics also stores RIDs for objects which are not in ObjectDB. This means we can't lifetime check them with ObjectDB, and the same vulnerability exists for accessing dangling RIDs that caused the original issue.

This should ideally be closed as the current design is unsafe, although there are no reports afaik of this occurring in the wild (although such errors may not result in crash and may only be seen in e.g. asan build).

Making completely safe in this situation is out of scope for this PR, and as stated in the original issue, would involve e.g.

@fire
Copy link
Member

fire commented Sep 22, 2024

Any commentary for testing this or should be merge as is?

@lawnjelly
Copy link
Member Author

lawnjelly commented Sep 23, 2024

Any commentary for testing this or should be merge as is?

It's a fairly simple change, aside from the comments, it's just changing:

if (ObjectDB::get_instance(platform_object_id)) {

to

if (platform_object_id.is_null() || ObjectDB::get_instance(platform_object_id)) {

This means it passes through and ignores the lifetime check for the physics objects that are not derived from Object (such as the platform in this case), as these are meaningless, testing ObjectID 0 against the ObjectDB will always fail.

This is simply returning to the previous behaviour before #74732 (for physics objects), so should be pretty safe to merge.

There should be no real need to "test" as the logic is super simple, and it's reverting to previous behaviour (which has been tested for years). It's essentially reverting #74732 except for objects derived from Object (which can be protected in this way).

Original Bug

To remind, the original bug is that physics is keeping a RID (essentially glorified pointer) to an object which may have been deleted. This is one of the most common bugs in programming (and the solutions). See the linked issue for discussion of various ways to address this.

For now I just used ObjectDB (as it is simple to implement) but in the long term we might consider a more robust check given that we have learned that physics objects cannot be checked in this way.

@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 23, 2024
scene/3d/physics/character_body_3d.cpp Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Lifetime checks for stored `RIDs` for collision objects assumed they had valid `object_ids`.
It turns out that some are not derived from `Object` and thus checking `ObjectDB` returns false for some valid `RIDs`.
To account for this we only perform lifetime checks on valid `object_ids`.
@lawnjelly lawnjelly force-pushed the fix_character_platform branch from 7826daa to 6764338 Compare September 23, 2024 09:56
@akien-mga akien-mga merged commit b9b7932 into godotengine:master Sep 23, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the fix_character_platform branch September 23, 2024 10:49
@lawnjelly
Copy link
Member Author

Reminder there's a similar 3.x PR too which needs approval to merge:
#97316

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release regression topic:physics topic:3d
Projects
None yet
4 participants