Fix display.set_mode segfault after resizing #3501

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

Jump to bottom

Open

MightyJosip wants to merge 2 commits into pygame-community:main

from MightyJosip:surface-segfault

Open

Fix display.set_mode segfault after resizing #3501

MightyJosip wants to merge 2 commits into pygame-community:main from MightyJosip:surface-segfault

Conversation

@MightyJosip

Copy link

Member

@MightyJosip MightyJosip commented Jun 15, 2025 •

edited

Loading

This should fix the following code that segfaults

import pygame
pygame.display.set_mode([800, 600], flags=pygame.SCALED)
pygame.display.set_mode([800, 598], flags=pygame.RESIZABLE | pygame.SCALED)

This also fixes #2571

TLDR: Changing size of the window invalidates surface returned by SDL_GetWindowSurface() as explained in https://wiki.libsdl.org/SDL2/SDL_GetWindowSurface, so this commit makes sure the module always has the reference to the right SDL_Surface (hopefully). There are other possible fixes for this, but this one to me looks the most elegant (unless we rewrite the entire module/function).

And the long explanation is available in our discord server https://discord.com/channels/772505616680878080/772940667231928360/1383591122101604402

Also, if someone knows another place in our code when the resizing could happen, make sure to notify me to add this new helper function there as well

EDIT: Looks like there is a memory leak with this change, will need to check if it happens without it

EDIT 2: I am pretty sure that the memory leak happens because of #3502

@MightyJosip


 Fix display.set_mode segfault after resizing

59e8ab3

@MightyJosip MightyJosip requested a review from a team as a code owner

June 15, 2025 10:47

ankith26

ankith26 approved these changes

Jun 15, 2025

View reviewed changes

Copy link

Member

@ankith26 ankith26 left a comment •

edited

Loading

Choose a reason for hiding this comment

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

LGTM, thanks for the fix! 🎉

re: memory leak, I am not seeing any noticeable evidence of that. I believe that even if there is a memory leak, it would probably not be a result of this PR.

Also, if someone knows another place in our code when the resizing could happen, make sure to notify me to add this new helper function there as well

There is some logic in pg_flip_internal that is exactly like _check_window_resized you may wanna update that snippet as well, just for code quality reasons. Nothing related to this fix per se.

I have tested this change on Ubuntu and found no issues, but I am also requesting reviews from Windows users to verify that this fixes the issue at hand.

@MightyJosip

Copy link

Member Author

MightyJosip commented Jun 15, 2025

What I mean by the memory leak is the following:

import pygame
import random
import psutil
import os
pygame.display.set_mode((800, 600), flags=pygame.SCALED)
while True:
 print(f"Memory usage: {psutil.Process(os.getpid()).memory_info().rss / 1024 / 1024:.2f} MB")
 pygame.display.set_mode([random.randint(600, 800), random.randint(600, 800)], flags=pygame.RESIZABLE | pygame.SCALED)

It is probably happening because with this change the old surface is not freed. However, if I free the surface, then I return to the old problem of segfault 🤣 . Really hard issue

@ankith26

Copy link

Member

ankith26 commented Jun 15, 2025

the old surface should be freed already by SDL, it cannot be freed on our side.

@MightyJosip


 Fix display.set_mode segfault after resizing

2f9ebbe

gresm

gresm reviewed

Jun 16, 2025

View reviewed changes

src_c/display.c

surface = pgSurface_New2(surf, newownedsurf != NULL);

}

else {

_check_window_resized(win, 0);

Copy link

Contributor

@gresm gresm Jun 16, 2025

Choose a reason for hiding this comment

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

I'm probably failing to notice something here, but why aren't we freeing the old surface here?

Copy link

Member Author

@MightyJosip MightyJosip Jun 16, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

Because if we free then we get segmentation fault from the linked issue. You have more explanation in the discord link

ankith26

ankith26 requested changes

Jun 20, 2025

View reviewed changes

Copy link

Member

@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.

I'm not sure if the current freeing logic here is okay.

We essentially have to consider two cases:

The display surface is created on our side with a PG_CreateSurface call. In this case we are responsible for freeing it, otherwise it leaks
The display surface is obtained via a call from SDL_GetWindowSurface. In this case calling SDL_FreeSurface is a noop because its managed by SDL. The fact that this surface becomes invalidated by SDL is the reason for any segfault we get while accessing it.

This makes our freeing logic complex. We have to now actually keep track of how the previous surface was allocated and free it based on that condition.

@MyreMylar

Copy link

Member

MyreMylar commented Aug 25, 2025

I'm not sure if the current freeing logic here is okay.

We essentially have to consider two cases:

The display surface is created on our side with a PG_CreateSurface call. In this case we are responsible for freeing it, otherwise it leaks

The display surface is obtained via a call from SDL_GetWindowSurface. In this case calling SDL_FreeSurface is a noop because its managed by SDL. The fact that this surface becomes invalidated by SDL is the reason for any segfault we get while accessing it.

This makes our freeing logic complex. We have to now actually keep track of how the previous surface was allocated and free it based on that condition.

Almost sounds like we need something like 'PGObtainDisplaySurface' with a 'PGFreeDisplaySurface' and a 'PGDisplaySurface' that tracks who owns the surface.

@MightyJosip MightyJosip mentioned this pull request

Oct 24, 2025

Changing from SCALED to SCALED RESIZEABLE window frequently causes segmentation fault on MacOS #3621

Open

Labels

None yet

4 participants

@MightyJosip @ankith26 @MyreMylar @gresm

Uh oh!

Fix display.set_mode segfault after resizing #3501

Are you sure you want to change the base?

Fix display.set_mode segfault after resizing #3501

Uh oh!

Conversation

@MightyJosip MightyJosip commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@ankith26 ankith26 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MightyJosip commented Jun 15, 2025

Uh oh!

ankith26 commented Jun 15, 2025

Uh oh!

@gresm gresm Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

@MightyJosip MightyJosip Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

Uh oh!

MyreMylar commented Aug 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@MightyJosip MightyJosip commented Jun 15, 2025 •

edited

Loading

@ankith26 ankith26 left a comment •

edited

Loading

@MightyJosip MightyJosip Jun 16, 2025 •

edited

Loading