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 crash on save-branch-as-scene #15571

Merged
merged 1 commit into from
Jan 11, 2018

Conversation

RandomShaper
Copy link
Member

Fixes #15539.
Fixes #9367.

These reports were made for 2.1, because original reporters weren't able to reproduce it on 3.0.

The truth is that given that 3.0 performs no byte-tagging on released memory, accessing a dangling object can be unnoticed. In this case. the crashing code would get the illusion that the signal map was just empty but valid, albeit actually the whole object had been released.

@reduz, maybe you can change your mind about byte-tagging on memory release for debug builds.

@reduz
Copy link
Member

reduz commented Jan 11, 2018

I guess we could do byte tagging, the problem is that as far as I know, it depends on OS (windows does byte tagging on debug too) so it should be optional by OS?

@akien-mga akien-mga merged commit 920715b into godotengine:master Jan 11, 2018
@RandomShaper RandomShaper deleted the fix-crash-save-branch branch January 11, 2018 16:18
@RandomShaper
Copy link
Member Author

The advantage of a custom approach would be that depending on the state of the memory you can tag it differently: released, just allocated, just freed after a realloc, just allocated by a realloc.
I think I helps a lot in debugging. When I saw 0xea in the stack trace of this bug for 2.1 I immediately knew it was accesing a dangling pointer.
When I have enough time I'll submit a PR and we will be able to continue discussion there.
Will be optional and only for debug builds, of course.

@reduz
Copy link
Member

reduz commented Jan 11, 2018 via email

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.

3 participants