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

[3.x] Fix physics platform behaviour regression #97316

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 (for 3.x)
3.x version of #97315

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.

  • RID database
  • Clearing stored RID references on deletion of objects

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 added bug topic:physics regression topic:3d cherrypick:3.6 Considered for cherry-picking into a future 3.6.x release labels Sep 22, 2024
@lawnjelly lawnjelly added this to the 3.7 milestone Sep 22, 2024
@lawnjelly lawnjelly requested a review from a team as a code owner September 22, 2024 07:57
@akien-mga akien-mga merged commit 53cccca into godotengine:3.x Sep 23, 2024
14 checks passed
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the fix_character_platform3 branch September 23, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.6 Considered for cherry-picking into a future 3.6.x release regression topic:physics topic:3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants