From 31705496aed94e10ec4f6facc11722b972fb67d6 Mon Sep 17 00:00:00 2001 From: Dan Lawrence Date: Thu, 30 May 2024 19:37:09 +0100 Subject: [PATCH 1/7] One method to fix the NaN/INT_MAX segfault in blit fixes #2694 --- src_c/surface.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src_c/surface.c b/src_c/surface.c index 957e7bcaee..c993bf9bc1 100644 --- a/src_c/surface.c +++ b/src_c/surface.c @@ -3828,6 +3828,15 @@ pgSurface_Blit(pgSurfaceObject *dstobj, pgSurfaceObject *srcobj, SDL_Rect orig_clip, sub_clip; Uint8 alpha; + if ((dstrect->x < -INT_MAX) || (dstrect->x > INT_MAX) || (dstrect->y < -INT_MAX) || (dstrect->y > INT_MAX)){ + // destination position has values that are too large + return 0; + } + if ((srcrect->x < -INT_MAX) || (srcrect->x > INT_MAX) || (srcrect->y < -INT_MAX) || (srcrect->y > INT_MAX)){ + // source position has values that are too large: + return 0; + } + /* passthrough blits to the real surface */ if (((pgSurfaceObject *)dstobj)->subsurface) { PyObject *owner; From a24727e00883fe22c14d4a33f89ec63c96fd08a4 Mon Sep 17 00:00:00 2001 From: Dan Lawrence Date: Thu, 30 May 2024 19:39:57 +0100 Subject: [PATCH 2/7] One method to fix the NaN/INT_MAX segfault in blit fixes #2694 --- src_c/surface.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src_c/surface.c b/src_c/surface.c index c993bf9bc1..1ab9602a18 100644 --- a/src_c/surface.c +++ b/src_c/surface.c @@ -3828,11 +3828,13 @@ pgSurface_Blit(pgSurfaceObject *dstobj, pgSurfaceObject *srcobj, SDL_Rect orig_clip, sub_clip; Uint8 alpha; - if ((dstrect->x < -INT_MAX) || (dstrect->x > INT_MAX) || (dstrect->y < -INT_MAX) || (dstrect->y > INT_MAX)){ + if ((dstrect->x < -INT_MAX) || (dstrect->x > INT_MAX) || + (dstrect->y < -INT_MAX) || (dstrect->y > INT_MAX)) { // destination position has values that are too large return 0; } - if ((srcrect->x < -INT_MAX) || (srcrect->x > INT_MAX) || (srcrect->y < -INT_MAX) || (srcrect->y > INT_MAX)){ + if ((srcrect->x < -INT_MAX) || (srcrect->x > INT_MAX) || + (srcrect->y < -INT_MAX) || (srcrect->y > INT_MAX)) { // source position has values that are too large: return 0; } From ef2b15279a4be80b53d7cc555c7ef0a3a72bc41c Mon Sep 17 00:00:00 2001 From: Dan Lawrence Date: Thu, 30 May 2024 19:52:18 +0100 Subject: [PATCH 3/7] fix for srcrect --- src_c/surface.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src_c/surface.c b/src_c/surface.c index 1ab9602a18..4a5a0970e6 100644 --- a/src_c/surface.c +++ b/src_c/surface.c @@ -3833,8 +3833,9 @@ pgSurface_Blit(pgSurfaceObject *dstobj, pgSurfaceObject *srcobj, // destination position has values that are too large return 0; } - if ((srcrect->x < -INT_MAX) || (srcrect->x > INT_MAX) || - (srcrect->y < -INT_MAX) || (srcrect->y > INT_MAX)) { + if (srcrect != NULL && + ((srcrect->x < -INT_MAX) || (srcrect->x > INT_MAX) || + (srcrect->y < -INT_MAX) || (srcrect->y > INT_MAX))) { // source position has values that are too large: return 0; } From 9f53d26e50ace15b222bf170f3fceae42d7d9559 Mon Sep 17 00:00:00 2001 From: Dan Lawrence Date: Fri, 31 May 2024 17:02:31 +0100 Subject: [PATCH 4/7] Switch to INT_MIN and add test for fix code --- src_c/surface.c | 8 +++---- test/surface_test.py | 51 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src_c/surface.c b/src_c/surface.c index 4a5a0970e6..76fdad2d6a 100644 --- a/src_c/surface.c +++ b/src_c/surface.c @@ -3828,14 +3828,14 @@ pgSurface_Blit(pgSurfaceObject *dstobj, pgSurfaceObject *srcobj, SDL_Rect orig_clip, sub_clip; Uint8 alpha; - if ((dstrect->x < -INT_MAX) || (dstrect->x > INT_MAX) || - (dstrect->y < -INT_MAX) || (dstrect->y > INT_MAX)) { + if ((dstrect->x <= INT_MIN) || (dstrect->x >= INT_MAX) || + (dstrect->y <= INT_MIN) || (dstrect->y >= INT_MAX)) { // destination position has values that are too large return 0; } if (srcrect != NULL && - ((srcrect->x < -INT_MAX) || (srcrect->x > INT_MAX) || - (srcrect->y < -INT_MAX) || (srcrect->y > INT_MAX))) { + ((srcrect->x <= INT_MIN) || (srcrect->x >= INT_MAX) || + (srcrect->y <= INT_MIN) || (srcrect->y >= INT_MAX))) { // source position has values that are too large: return 0; } diff --git a/test/surface_test.py b/test/surface_test.py index d0c01c28a5..c2944e8be3 100644 --- a/test/surface_test.py +++ b/test/surface_test.py @@ -1061,6 +1061,57 @@ def test_blit_zero_overlap(self): ) # Remaining corners self.assertEqual(self.dst_surface.get_at((0, 63)), (0, 0, 0)) + def test_blit_bad_frect_position_values(self): + """Previously segfaulted - these should now silently exit and blit nothing""" + + # test the old segfault case + screen = pygame.display.set_mode((800, 600)) + screen.fill((0, 0, 0)) + test_surf = pygame.Surface((1057, 398), flags=pygame.SRCALPHA) + pos_rect = pygame.FRect(float("nan"), 202.0, 100.0, 100.0) + screen.blit(test_surf, pos_rect) + self.assertEqual(screen.get_at((0, 0)), (0, 0, 0)) + self.assertEqual(screen.get_at((63, 63)), (0, 0, 0)) + + # thoroughly test the fix code + # these parts could be removed or changed once FRect is able to + # handle/reject large values like this + self.dst_surface.blit(self.src_surface, + pygame.FRect(float("nan"), 0.0, 300, 300)) + + self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) + self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) + + self.dst_surface.blit(self.src_surface, + pygame.FRect(0.0, float("nan"), 300, 300)) + + c_max_int = 2147483647 + c_min_int = -2147483648 + self.dst_surface.blit(self.src_surface, + pygame.FRect(c_max_int, c_max_int, 300, 300)) + + self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) + self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) + + self.dst_surface.blit(self.src_surface, + pygame.FRect(c_min_int, c_min_int, 300, 300)) + + self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) + self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) + + # test area too + self.dst_surface.blit(self.src_surface, (0, 0), + pygame.FRect(c_max_int, c_max_int, 300, 300)) + + self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) + self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) + + self.dst_surface.blit(self.src_surface, (0, 0), + pygame.FRect(c_min_int, c_min_int, 300, 300)) + + self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) + self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) + def test_blit__SRCALPHA_opaque_source(self): src = pygame.Surface((256, 256), SRCALPHA, 32) dst = src.copy() From a10e222b58a0664fbaaf13c820d56dd9041099b0 Mon Sep 17 00:00:00 2001 From: Dan Lawrence Date: Fri, 31 May 2024 17:25:02 +0100 Subject: [PATCH 5/7] Switch to INT_MIN and add test for fix code --- test/surface_test.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/test/surface_test.py b/test/surface_test.py index c2944e8be3..b2a3c45a24 100644 --- a/test/surface_test.py +++ b/test/surface_test.py @@ -1076,38 +1076,44 @@ def test_blit_bad_frect_position_values(self): # thoroughly test the fix code # these parts could be removed or changed once FRect is able to # handle/reject large values like this - self.dst_surface.blit(self.src_surface, - pygame.FRect(float("nan"), 0.0, 300, 300)) + self.dst_surface.blit( + self.src_surface, pygame.FRect(float("nan"), 0.0, 300, 300) + ) self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) - self.dst_surface.blit(self.src_surface, - pygame.FRect(0.0, float("nan"), 300, 300)) + self.dst_surface.blit( + self.src_surface, pygame.FRect(0.0, float("nan"), 300, 300) + ) c_max_int = 2147483647 c_min_int = -2147483648 - self.dst_surface.blit(self.src_surface, - pygame.FRect(c_max_int, c_max_int, 300, 300)) + self.dst_surface.blit( + self.src_surface, pygame.FRect(c_max_int, c_max_int, 300, 300) + ) self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) - self.dst_surface.blit(self.src_surface, - pygame.FRect(c_min_int, c_min_int, 300, 300)) + self.dst_surface.blit( + self.src_surface, pygame.FRect(c_min_int, c_min_int, 300, 300) + ) self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) # test area too - self.dst_surface.blit(self.src_surface, (0, 0), - pygame.FRect(c_max_int, c_max_int, 300, 300)) + self.dst_surface.blit( + self.src_surface, (0, 0), pygame.FRect(c_max_int, c_max_int, 300, 300) + ) self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) - self.dst_surface.blit(self.src_surface, (0, 0), - pygame.FRect(c_min_int, c_min_int, 300, 300)) + self.dst_surface.blit( + self.src_surface, (0, 0), pygame.FRect(c_min_int, c_min_int, 300, 300) + ) self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) From 4e94f5ce1e44a8f5bc7237a01c4c8a82a5f6b61a Mon Sep 17 00:00:00 2001 From: Dan Lawrence Date: Fri, 31 May 2024 17:34:44 +0100 Subject: [PATCH 6/7] Switch to INT_MIN and add test for fix code --- test/surface_test.py | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/test/surface_test.py b/test/surface_test.py index b2a3c45a24..949afebb76 100644 --- a/test/surface_test.py +++ b/test/surface_test.py @@ -1064,29 +1064,16 @@ def test_blit_zero_overlap(self): def test_blit_bad_frect_position_values(self): """Previously segfaulted - these should now silently exit and blit nothing""" - # test the old segfault case + # test the old segfault case - just to make sure it doesn't segfault screen = pygame.display.set_mode((800, 600)) screen.fill((0, 0, 0)) test_surf = pygame.Surface((1057, 398), flags=pygame.SRCALPHA) pos_rect = pygame.FRect(float("nan"), 202.0, 100.0, 100.0) screen.blit(test_surf, pos_rect) - self.assertEqual(screen.get_at((0, 0)), (0, 0, 0)) - self.assertEqual(screen.get_at((63, 63)), (0, 0, 0)) # thoroughly test the fix code # these parts could be removed or changed once FRect is able to # handle/reject large values like this - self.dst_surface.blit( - self.src_surface, pygame.FRect(float("nan"), 0.0, 300, 300) - ) - - self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) - self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) - - self.dst_surface.blit( - self.src_surface, pygame.FRect(0.0, float("nan"), 300, 300) - ) - c_max_int = 2147483647 c_min_int = -2147483648 self.dst_surface.blit( From 924817b602bbe74901e5b74d306c01949060e642 Mon Sep 17 00:00:00 2001 From: Dan Lawrence Date: Sat, 1 Jun 2024 13:34:32 +0100 Subject: [PATCH 7/7] improve tests, adjust early exits to include dimensions --- src_c/surface.c | 17 ++++++++++------- test/surface_test.py | 19 ++++++++++++++----- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src_c/surface.c b/src_c/surface.c index 76fdad2d6a..95dabbb372 100644 --- a/src_c/surface.c +++ b/src_c/surface.c @@ -3828,15 +3828,18 @@ pgSurface_Blit(pgSurfaceObject *dstobj, pgSurfaceObject *srcobj, SDL_Rect orig_clip, sub_clip; Uint8 alpha; - if ((dstrect->x <= INT_MIN) || (dstrect->x >= INT_MAX) || - (dstrect->y <= INT_MIN) || (dstrect->y >= INT_MAX)) { - // destination position has values that are too large + if ((dstrect->x <= INT_MIN + dstrect->w) || + (dstrect->x >= INT_MAX - dstrect->w) || + (dstrect->y <= INT_MIN + dstrect->h) || + (dstrect->y >= INT_MAX - dstrect->h)) { + // destination rect has values that are too large return 0; } - if (srcrect != NULL && - ((srcrect->x <= INT_MIN) || (srcrect->x >= INT_MAX) || - (srcrect->y <= INT_MIN) || (srcrect->y >= INT_MAX))) { - // source position has values that are too large: + if (srcrect != NULL && ((srcrect->x <= INT_MIN + srcrect->w) || + (srcrect->x >= INT_MAX - dstrect->w) || + (srcrect->y <= INT_MIN + srcrect->h) || + (srcrect->y >= INT_MAX - dstrect->h))) { + // source rect has values that are too large: return 0; } diff --git a/test/surface_test.py b/test/surface_test.py index 949afebb76..100796ef21 100644 --- a/test/surface_test.py +++ b/test/surface_test.py @@ -1064,27 +1064,36 @@ def test_blit_zero_overlap(self): def test_blit_bad_frect_position_values(self): """Previously segfaulted - these should now silently exit and blit nothing""" - # test the old segfault case - just to make sure it doesn't segfault + # test the old segfault case s- just to make sure it doesn't segfault + pygame.display.init() screen = pygame.display.set_mode((800, 600)) screen.fill((0, 0, 0)) test_surf = pygame.Surface((1057, 398), flags=pygame.SRCALPHA) pos_rect = pygame.FRect(float("nan"), 202.0, 100.0, 100.0) screen.blit(test_surf, pos_rect) + pygame.display.quit() - # thoroughly test the fix code + surf_width = 2048 + c_max_int = 2147483647 + pygame.display.init() + screen = pygame.display.set_mode((63, 600)) + test_surf = pygame.Surface((surf_width, 400), flags=pygame.SRCALPHA) + screen.blit(test_surf, (c_max_int - surf_width - 64, 200.0)) + pygame.display.quit() + + # thoroughly test the fix code in FRect # these parts could be removed or changed once FRect is able to # handle/reject large values like this - c_max_int = 2147483647 c_min_int = -2147483648 self.dst_surface.blit( - self.src_surface, pygame.FRect(c_max_int, c_max_int, 300, 300) + self.src_surface, pygame.FRect(float("nan"), float("inf"), 300, 300) ) self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0)) self.assertEqual(self.dst_surface.get_at((63, 63)), (0, 0, 0)) self.dst_surface.blit( - self.src_surface, pygame.FRect(c_min_int, c_min_int, 300, 300) + self.src_surface, pygame.FRect(c_max_int, c_max_int, c_max_int, c_max_int) ) self.assertEqual(self.dst_surface.get_at((0, 0)), (0, 0, 0))