Skip to content

fixed segfault #3666

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 4 commits into from
Jan 15, 2023
Merged

fixed segfault #3666

merged 4 commits into from
Jan 15, 2023

Conversation

oddbookworm
Copy link
Contributor

Fixes #3665 by raising a ValueError if the surface access is after closing the PixelArray

Matiiss
Matiiss suggested changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@Matiiss Matiiss 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 fairly certain it would be important to add this check to other places where it hasn't been added:

pygame/src_c/pixelarray.c

Lines 432 to 438 in 25aac95

static PyObject *
_pxarray_get_itemsize(pgPixelArrayObject *self, void *closure)
{
SDL_Surface *surf = pgSurface_AsSurface(self->surface);
return PyLong_FromLong((long)surf->format->BytesPerPixel);
}

and I'm not sure about this but if shape also gets set to NULL (even if that doesn't happen, I suppose these should then still check if self->surface == NULL):

pygame/src_c/pixelarray.c

Lines 443 to 450 in 25aac95

static PyObject *
_pxarray_get_shape(pgPixelArrayObject *self, void *closure)
{
if (self->shape[1]) {
return Py_BuildValue("(nn)", self->shape[0], self->shape[1]);
}
return Py_BuildValue("(n)", self->shape[0]);
}

pygame/src_c/pixelarray.c

Lines 455 to 462 in 25aac95

static PyObject *
_pxarray_get_strides(pgPixelArrayObject *self, void *closure)
{
if (self->shape[1]) {
return Py_BuildValue("(nn)", self->strides[0], self->strides[1]);
}
return Py_BuildValue("(n)", self->strides[0]);
}

pygame/src_c/pixelarray.c

Lines 467 to 471 in 25aac95

static PyObject *
_pxarray_get_ndim(pgPixelArrayObject *self, void *closure)
{
return PyLong_FromLong(self->shape[1] ? 2L : 1L);
}

And I'm fairly certain there are a couple more functions that need this check, perhaps, also in pixelarray_methods.c

@MyreMylar
Copy link
Contributor

MyreMylar commented Jan 14, 2023 •
edited
Loading

From my testing these are the PixelArray public functions and members that segfault after a first call to .close():

  • .surface
  • .itemsize
  • .close() - a second call to it.
  • transpose()

The others already say:

ValueError: Operation on closed PixelArray.

Or return some valid data. I didn't bother testing all the dunder methods because this is already a fairly dumb thing for a user to be doing.

MyreMylar
MyreMylar suggested changes Jan 14, 2023
View reviewed changes
Copy link
Contributor

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

Please add a similar check to clear the other segfaults outlined above.

MyreMylar
MyreMylar approved these changes Jan 14, 2023
View reviewed changes
Copy link
Contributor

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

@Starbuck5
Copy link
Contributor

This looks reasonable to me, thanks Andrew!

illume pushed a commit that referenced this pull request Apr 30, 2023
Merge pull request #3666 from oddbookworm/pixelarray-segfault
9fb6c20 
fixed segfault
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
3 more reviewers

@Matiiss Matiiss Matiiss requested changes

@MyreMylar MyreMylar MyreMylar approved these changes

@Starbuck5 Starbuck5 Starbuck5 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
PixelArray pygame.PixelArray segfault
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.

Segfault in PixelArray when trying to access surface after closing the PixelArray
4 participants
@oddbookworm @MyreMylar @Starbuck5 @Matiiss