Skip to content

One method to fix the NaN/INT_MAX segfault in blit #2893

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

Closed
wants to merge 9 commits into from

Conversation

MyreMylar
Copy link
Member

fixes #2694

The logic being that these values are so large that they can only realistically come from bad data, but also we don't want to error out inside blit?

Hopefully bits earlier up the chain - like Rect/Frect can catch these invalid values and error there.

@MyreMylar MyreMylar requested a review from a team as a code owner May 30, 2024 18:39
Copy link
Member

@andrewhong04 andrewhong04 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
Member

I'd like to hold on merging this until I understand it, somewhat selfishly.

Um, how did the original poster get nan values in the first place?

@damusss
Copy link
Member

damusss commented Jun 1, 2024

I'd like to hold on merging this until I understand it, somewhat selfishly.

Um, how did the original poster get nan values in the first place?

They didn't, and their code did not produce any segfaults for me and for other contributors. The nan was added by MyreMylar and we discovered it segfaults.

@MyreMylar
Copy link
Member Author

MyreMylar commented Jun 1, 2024 via email

@MyreMylar
Copy link
Member Author

Poking at my segfault test program a bit more it looks like it is related to the width of the blit itself & the INT_MAX / INT_MIN values overflowing. Try this program and mess around with the numbers a bit:

import pygame

int_max = 2147483647

surf_width = 1057
int_max_buffer = surf_width + 32

for x in reversed(range(0, 700)):
    print(x)
    pygame.display.init()
    screen = pygame.display.set_mode((x, 600))
    test_surf = pygame.Surface((surf_width, 400), flags=pygame.SRCALPHA)
    pos_rect = pygame.FRect(int_max - int_max_buffer, 200.0, 100.0, 100.0)
    screen.blit(test_surf, pos_rect)
    pygame.display.quit()

the surface to be blitted needs to be overlapping the target vertically here to get into the 'danger zone' but then you can turn the segfault on or off by having an x position of anything greater than: INT_MAX - surface_width - 32

@MyreMylar
Copy link
Member Author

That suggests to me that is specifically something to do with clipping the source surface to the destination surface when the right hand side of the source surface position is greater than INT_MAX. I mean maybe the other sides would also crash in the clipping but that one gets hit first because it is furthest out in the pygame coordinate system.

Basically we need to keep all four corners of the surface-to-be-clipped rectangle inside INT_MIN to INT_MAX - possibly with a small safety buffer as well.

@MyreMylar
Copy link
Member Author

Further refinement to strip out FRect as that is not required for the segfault:

import pygame

int_max = 2147483647
surf_width = 2048

for x in reversed(range(0, 400)):
    print(x, surf_width)
    int_max_buffer = (
        surf_width 
    )  # subtract values here (e.g. 32, 64) and you will get a segfault pretty quick
    pygame.display.init()
    screen = pygame.display.set_mode((x, 600))
    test_surf = pygame.Surface((surf_width, 400), flags=pygame.SRCALPHA)
    screen.blit(test_surf, (int_max - int_max_buffer, 200.0))
    pygame.display.quit()

I think this probably makes it about as clear as it can get debugging from the python side. If anyone can narrow it down on the C side somehow that would be useful too.

@MyreMylar
Copy link
Member Author

MyreMylar commented Jun 1, 2024

OK, I adjusted the early exits to just always account for the dimensions of the surfaces and I can't make it segfault now. We could check for the dimensions of the source surface minus the dimensions of the destination surface - but that is extra math for something that I don't believe there is any legitimate reason to be doing (surfaces can't actually get very large so these blits would all be clipped anyway if the clipping wasn't segfaulting). Doing it this way we will early exit with stupid source rect parameters whatever the width of the destination surface.

Other things we could do:

  1. Make Rect handle float("nan") and float("inf) so they don't get converted to INT_MIN when passed in via various methods. See:
import pygame

rect = pygame.Rect(float("nan"), float("inf"), float("nan"), float("inf"))
print(rect)

Output:

Rect(-2147483648, -2147483648, -2147483648, -2147483648)

Meanwhile:

int(float("NaN"))

Output:

ValueError: cannot convert float NaN to integer

And:

int(float("inf"))

Output:

OverflowError: cannot convert float infinity to integer
  1. Add a note to the SDL issue on this topic here: SDL_rect_impl.h:90:10: runtime error: signed integer overflow libsdl-org/SDL#8879 if we think we have anything useful to add. It sounds like SDL is already aware of INT_MAX/INT_MIN overflows in the code base but hasn't come up with a definitive solution, or decided if there is one.

@MyreMylar MyreMylar added the bugfix PR that fixes bug label Jun 1, 2024
@MyreMylar
Copy link
Member Author

I think this probably won't get merged as the segfault is too obscure and originates inside SDL, so I'm going to close it.

@MyreMylar MyreMylar closed this Sep 29, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal Python error: Segmentation fault caused by Pygame
4 participants