Skip to content

Fix ASAN issues in surface.scroll #3497

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Aug 17, 2025
Merged

Fix ASAN issues in surface.scroll #3497

merged 1 commit into from
Aug 17, 2025

Conversation

ankith26
Copy link
Member

fixes #3420

I could reproduce the ASAN issues locally, then iteratively fixed all the issues until the tests passed. Requesting @damusss to review this code (yes I did check out your branch from the linked issue but that was not fixing all the ASAN issues)

@ankith26 ankith26 requested a review from a team as a code owner June 11, 2025 10:11
@ankith26 ankith26 added this to the 2.5.6 milestone Jun 11, 2025
@ankith26 ankith26 requested a review from damusss June 11, 2025 10:11
damusss
damusss approved these changes Jun 11, 2025
View reviewed changes
Copy link
Member

@damusss damusss left a comment

Choose a reason for hiding this comment

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

Not familiar with ASAN issues, I was waiting for their response. Glad you fixed them, looks good to me 👍

@Starbuck5 Starbuck5 added the Surface pygame.Surface label Aug 17, 2025
Starbuck5
Starbuck5 approved these changes Aug 17, 2025
View reviewed changes
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

So memmove is supposed to be safer and slower than memcpy. And correctness wins over performance.

But I still wanted to test performance, so I did, and I found no significant difference. If anything, it's faster with memmove.

@Starbuck5 Starbuck5 changed the title Fix ASAN issues in surface scroll tests Fix ASAN issues in surface.scroll Aug 17, 2025
@Starbuck5 Starbuck5 added the bugfix PR that fixes bug label Aug 17, 2025
@ankith26 ankith26 merged commit a88e23b into main Aug 17, 2025
27 checks passed
@ankith26 ankith26 deleted the ankith26-surface-asan-fixes branch August 17, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Starbuck5 Starbuck5 Starbuck5 approved these changes

@damusss damusss damusss approved these changes

Assignees
No one assigned
Labels
bugfix PR that fixes bug Surface pygame.Surface
Projects
None yet
Milestone
2.5.6
Development

Successfully merging this pull request may close these issues.

ASAN: Overlapping memcpy() in scroll_default() when running surface_test
3 participants
@ankith26 @Starbuck5 @damusss