From 328bbc058004d47cdd6eda995e95e2ec24c77cd7 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 29 Sep 2023 14:23:36 +0100 Subject: [PATCH] SDLTest_CompareSurfaces: Decode pixels correctly on big-endian platforms Previously, if acting on a surface with less than 32 bits per pixel, this code was placing the pixel value from the surface in the first few bytes of the Uint32 to be decoded, and unrelated data from a subsequent pixel in the remaining bytes. Because SDL_GetRGBA takes the bits to be decoded from the least-significant bits of the given value, ignoring the higher-order bits if any, this happened to be correct on little-endian platforms, where the first few bytes store the least-significant bits of an integer. However, it was incorrect on big-endian, where the first few bytes are the most-significant bits of an integer. The previous implementation also assumed that unaligned access to a 32-bit quantity is possible, which is not the case on all CPUs (but happens to be true on x86). These issues were not discovered until now because SDLTest_CompareSurfaces() is only used in testautomation, which until recently was not being run routinely at build-time, because it contained other assumptions that can fail in an autobuilder or CI environment. Resolves: https://github.com/libsdl-org/SDL/issues/8315 Signed-off-by: Simon McVittie --- src/test/SDL_test_compare.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/test/SDL_test_compare.c b/src/test/SDL_test_compare.c index 247751cfc210b..023723de3bdbb 100644 --- a/src/test/SDL_test_compare.c +++ b/src/test/SDL_test_compare.c @@ -33,6 +33,25 @@ /* Counter for _CompareSurface calls; used for filename creation when comparisons fail */ static int _CompareSurfaceCount = 0; +static Uint32 +GetPixel(Uint8 *p, size_t bytes_per_pixel) +{ + Uint32 ret = 0; + + SDL_assert(bytes_per_pixel <= sizeof(ret)); + + /* Fill the appropriate number of least-significant bytes of ret, + * leaving the most-significant bytes set to zero, so that ret can + * be decoded with SDL_GetRGBA afterwards. */ +#if SDL_BYTEORDER == SDL_BIG_ENDIAN + SDL_memcpy(((Uint8 *) &ret) + (sizeof(ret) - bytes_per_pixel), p, bytes_per_pixel); +#else + SDL_memcpy(&ret, p, bytes_per_pixel); +#endif + + return ret; +} + /* Compare surfaces */ int SDLTest_CompareSurfaces(SDL_Surface *surface, SDL_Surface *referenceSurface, int allowable_error) { @@ -71,11 +90,14 @@ int SDLTest_CompareSurfaces(SDL_Surface *surface, SDL_Surface *referenceSurface, /* Compare image - should be same format. */ for (j = 0; j < surface->h; j++) { for (i = 0; i < surface->w; i++) { + Uint32 pixel; p = (Uint8 *)surface->pixels + j * surface->pitch + i * bpp; p_reference = (Uint8 *)referenceSurface->pixels + j * referenceSurface->pitch + i * bpp_reference; - SDL_GetRGBA(*(Uint32 *)p, surface->format, &R, &G, &B, &A); - SDL_GetRGBA(*(Uint32 *)p_reference, referenceSurface->format, &Rd, &Gd, &Bd, &Ad); + pixel = GetPixel(p, bpp); + SDL_GetRGBA(pixel, surface->format, &R, &G, &B, &A); + pixel = GetPixel(p_reference, bpp_reference); + SDL_GetRGBA(pixel, referenceSurface->format, &Rd, &Gd, &Bd, &Ad); dist = 0; dist += (R - Rd) * (R - Rd);