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

fixed pixelarray indexing to prevent segfault reported in #2275 #2276

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

oddbookworm
Copy link
Member

fixes #2275

@oddbookworm oddbookworm added PixelArray pygame.PixelArray bugfix PR that fixes bug labels Jun 28, 2023
@oddbookworm oddbookworm requested a review from a team as a code owner June 28, 2023 02:08
@oddbookworm
Copy link
Member Author

Further note:
It is also possible to segfault by trying to just index into that created pixelarray. This fixes both and the regression test has both cases

@Starbuck5
Copy link
Member

I think this may be more complicated.

If I create a PixelArray from a normal Surface and access the shape attribute, I get (w, h). With the example code in the issue, I get (w,). However, if I flip the dimension that it is zero dimensional in, everything works perfectly fine and it raises an IndexError as I would expect.

>>> weird_surface = pygame.Surface((0,5))
>>> pixarray = pygame.PixelArray(weird_surface)
>>> pixarray.shape
(0, 5)
>>> pixarray[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: array index out of range

So the problem is that the code is using 0 in shape[1] as a special value in places to indicate the dimensionality of the data? And this just happens to be slotting into that? Maybe the special value should be replaced with -1, which would never come from a Surface's dimensions.

@simplyrohan
Copy link
Contributor

simplyrohan commented Jun 29, 2023

Another thing I found was that when the 0d value is in the Y value, then the shape of the pixel array does not include the Y value at all...

import pygame
test_surf1 = pygame.Surface((0, 5))
test_surf2 = pygame.Surface((5,0))
pixarr1 = pygame.PixelArray(test_surf1)
pixarr2 = pygame.PixelArray(test_surf2)
print(pixarr1.shape, pixarr2.shape)

Outputs:
(0, 5) (5,)

Also, calling any magic method on the broken PixelArray will cause it to segfault. This means that the issue isn't only in the indexing.

Furthermore, the make_surface method of the broken PixelArray will simply hang indefinitely. It also does not close with KeyboardInterrupt. I had to force quit the terminal...

@oddbookworm oddbookworm marked this pull request as draft July 1, 2023 04:23
@oddbookworm
Copy link
Member Author

My most recent commit makes it impossible to create that pixelarray in the first place. I'll keep looking at how to solve the issue in a less strict way, but this should at least prevent any segfault conditions for now

@oddbookworm oddbookworm marked this pull request as ready for review July 14, 2023 01:57
Copy link
Member

@MyreMylar MyreMylar 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 think not being able to create an array of pixels for a surface with absolutely no pixels is a perfectly acceptable solution to this issue.

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.

thanks 👍

@MyreMylar MyreMylar added this to the 2.3.2 milestone Aug 14, 2023
@MyreMylar MyreMylar merged commit 095a2fe into pygame-community:main Aug 14, 2023
29 checks passed
ankith26 pushed a commit that referenced this pull request Sep 2, 2023
fixed pixelarray indexing to prevent segfault reported in #2275
@oddbookworm oddbookworm deleted the zero_area_surface branch December 21, 2023 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug PixelArray pygame.PixelArray
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault when using PixelArray with 0 height/width surface.
5 participants