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

[ResourceLoader] Add check to prevent double free crashes. #95186

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Aug 6, 2024

Fixes #95138

It's probably not a complete fix, since if I understand correctly, this code should not be called if load_paths_stack is already freed, but it's fixing the crash (tested on macOS, threaded loading seems to be working fine), and should not have any negative effects.

@bruvzg bruvzg added this to the 4.3 milestone Aug 6, 2024
@bruvzg bruvzg requested a review from a team as a code owner August 6, 2024 05:35
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, probably needs a more thorough safety analysis but this should be a sufficient fix for now

@akien-mga akien-mga merged commit b36885c into godotengine:master Aug 6, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper
Copy link
Member

It's very likely the actual fix is included in #94169. That PR includes a fix for the nesting level. It may be that without it, it can reach zero multiple times so the element is tried to be freed multiple times as well. If this PR succeeds in preventing the crash, that's good for now.

For completeness, let me mention that #93336 replaces the heap-allocated path stack with an in-class object. However, that's only about his it's lifecycle is managed, so if there's one PR potentially fixing the logic bug, that's not this one.

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.

Crash with deferred texture loading on multi-threaded scene loading
4 participants