Skip to content

Conversation

oddbookworm
Copy link
Member

Fixes #2961
@ankith26 figured out that it was an int overflow when multiplying surf->pitch by y

@oddbookworm oddbookworm added draw pygame.draw bugfix PR that fixes bug labels Jun 30, 2024
@oddbookworm oddbookworm requested a review from a team as a code owner June 30, 2024 16:52
@oddbookworm oddbookworm force-pushed the fix-draw-line-segfault branch from c612961 to 1dc41d6 Compare June 30, 2024 17:43
@bilhox
Copy link
Contributor

bilhox commented Jul 27, 2024

I don't like how the problem is solved, we aren't clamping the line in the clip rect so we have to check if each pixel of the line one by one are outside the clip rect. IMO, the best way to solve this problem is to delimit with new coordinates which part of the line is going to be drawn.

@ankith26 ankith26 marked this pull request as draft August 14, 2024 20:02
@oddbookworm oddbookworm force-pushed the fix-draw-line-segfault branch from 1dc41d6 to 4691737 Compare June 25, 2025 02:21
@oddbookworm oddbookworm marked this pull request as ready for review June 25, 2025 02:43
@oddbookworm oddbookworm changed the title set_at now take a long long for y instead of int set_at now take intptr_t for x, y instead of int, and guards 32-bit architectures Jun 29, 2025
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 🎉

@ankith26 ankith26 added this to the 2.5.6 milestone Jun 29, 2025
@Starbuck5
Copy link
Member

I think this could be simpler by using long long everywhere instead of a pointer type with a scary 32 bit path. Even on 32 bit, long long is 64 bit. Or you could be explicit and use int64_t?

The math being done is not really about pointers so its also strange to put it in a pointer type.

@Starbuck5
Copy link
Member

Gave it a test and just bringing in x and y as long longs in the set_at function call worked. It brings the changes to draw.c from 41 lines to 2 lines (because now the function definition spills onto 2 lines).

@Starbuck5 Starbuck5 marked this pull request as draft August 17, 2025 07:02
@oddbookworm
Copy link
Member Author

@Starbuck5 blame Ankith on those last two comments of yours. That's what I originally had

@ankith26
Copy link
Member

My reasoning for using intptr_t is that it is guaranteed to be as big as the pointer type. What this fix is doing is very relevant to pointer sizes. On a 32 bit platform, using a long long/int64_t doesn't fix the issue because the underlying pointer size is still 32 bits. We are essentially doing algebra on stuff that translates to addresses, and for this the C standard recommends this type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug draw pygame.draw
Projects
None yet
Development

Successfully merging this pull request may close these issues.

segfault when we use large coordinates in pygame.draw.line
4 participants