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 segfault in surface.fblits #2667

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

Matiiss
Copy link
Member

@Matiiss Matiiss commented Jan 14, 2024

Moved reference decrementing to after the values are used for stuff (see https://docs.python.org/3/c-api/iter.html#c.PyIter_Next).
I'm not sure why this wasn't causing issues for like actual tuples like (0, 0) being used directly for the destination, but it appears this was related to the object being passed having to get an attribute dynamically? Not entirely sure, but the segfault seems to have gone away.

MRE:

import pygame

pygame.init()
screen = pygame.display.set_mode((640, 360))
surf = pygame.Surface((10, 10))
screen.fblits((surf, surf.get_rect().topleft) for _ in range(1))
print("finished")

@Matiiss Matiiss requested a review from a team as a code owner January 14, 2024 21:31
@Matiiss Matiiss added this to the 2.5.0 milestone Jan 14, 2024
@Matiiss Matiiss added Surface pygame.Surface bugfix PR that fixes bug labels Jan 14, 2024
@Matiiss
Copy link
Member Author

Matiiss commented Jan 14, 2024

Also with these changes in place it's possible to wrap most of the functionality of both cases (lists/tuples and generators) in a single helper function, thus reducing repetition (call one function in two places), should that be done in this PR or a separate one?

Copy link
Member

@itzpr3d4t0r itzpr3d4t0r left a comment

Choose a reason for hiding this comment

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

LGTM! I guess we could do it either way, so it's your choice.

@Matiiss
Copy link
Member Author

Matiiss commented Jan 15, 2024

Alright then, since this PR focuses on fixing a bug, I will submit another PR focusing on code robustness once this gets merged.

src_c/surface.c Outdated Show resolved Hide resolved
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.

LGTM, thanks for the PR 🥳

@ankith26 ankith26 merged commit 977d6d5 into pygame-community:main Jan 19, 2024
30 checks passed
Starbuck5 pushed a commit that referenced this pull request Feb 18, 2024
@Starbuck5 Starbuck5 modified the milestones: 2.5.0, 2.4.1 Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug Surface pygame.Surface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants