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 issue #2543 segfault in font dealloc after reinit #2548

Merged
merged 4 commits into from Oct 23, 2021

Conversation

REW1L
Copy link
Contributor

@REW1L REW1L commented Apr 3, 2021

TTF_Quit frees all the resources allocated by freetype. However pointers to FT_Face from TTF_Font are not set to NULL after this. Therefore when font_dealloc tries to call TTF_CloseFont, FT_Done_Face is called on already freed object and segmentation fault appears.

It can be so when Font module quits and then GC is called. (test is provided)

Solution provided is a little hack for setting font->face to NULL. It will tell TTF_CloseFont that face has been already freed and all that's left is to free allocated SDL resources.

Also I had to introduce a field current_ttf_generation to the PyFontObject to understand if object passed to the font_dealloc function is being destructed after TTF_Quit.

@REW1L
Copy link
Contributor Author

REW1L commented Apr 3, 2021

This should fix #2543

@REW1L REW1L marked this pull request as draft April 3, 2021 13:13
@REW1L REW1L marked this pull request as ready for review April 3, 2021 13:22
if (self->ttf_init_generation != current_ttf_generation) {
// Since TTF_Font is a private structure
// it's impossible to access face field in a common way.
int** face_pp = font;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is enough to stop the problem instead?

TTF_CloseFont(font);
font=NULL;

Copy link
Contributor Author

@REW1L REW1L Apr 3, 2021

Choose a reason for hiding this comment

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

Because of tp_free further we are able not to aware about font. Therefore setting font to NULL is just redundant. Also font_dealloc is called only once after TTF_Quit to get a segfault inside a function TTF_CloseFont.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the explanation :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still not so clear about what

int **face_pp = font;
*face_pp = NULL;

does, exactly. Can you please explain this bit?
Anyways, I guess it is some weird hack, and I see it works because the tests pass :)

@ghost
Copy link

ghost commented Apr 8, 2021

@illume: Is there a timeline for this being released? I'm using pygame as part of a project and want to be able to update the supervisor as it's approaching deadlines.

Thanks for the quick patch @REW1L

@ankith26 ankith26 added this to the 2.0.2 milestone Aug 14, 2021
@Starbuck5
Copy link
Contributor

Starbuck5 commented Aug 20, 2021

Thanks for looking into this @REW1L, sorry it's been dormant on our end for so long.

Perhaps this should be turned into a more abstract "font object invalidation system," since something like this also segfaults:

import pygame
import gc
 
pygame.font.init()
font = pygame.font.Font(None, 20)
pygame.font.quit()
pygame.font.init()

font.render("hello", True, "white")
print("rendered") # never happens, segfaults on the render

But I still have some experimenting around this to do, I think. This is a weird situation / bug.

@ankith26 ankith26 modified the milestones: 2.0.2, 2.0.3 Oct 11, 2021
Copy link
Contributor

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

This PR looks good to me! Thanks for the PR 👍
Yeah since this fixes a segfault, it would be great to have this merged in ASAP, for v2.0.3

@REW1L there is a merge conflict that needs to be fixed before this being merged, if you want to fix it, you can go ahead! Or I can do it too if you'd like :)

@illume
Copy link
Member

illume commented Oct 23, 2021

I (hopefully) fixed the merge conflict. If it passes tests, we can merge it :)

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

👍 🎉 Thanks

@illume illume merged commit b72ba79 into pygame:main Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants