Segmentation Fault in SDL_BlitSurface (SDL 2.26.4)

[oddbookworm]

Someone reported a segfault to us at pygame-ce and I tracked it down to SDL_BlitSurface. Here is an MRE (pygame)

import pygame

screen = pygame.display.set_mode((385, 166))
surf = pygame.Surface((100, 100))
screen.blit(surf, (384.0, -2147483583.0))

Here is the exact call in the underlying C code, and the particular srcrect and dstrect that get fed into SDL_BlitSurface are

srcrect=(x=0, y=0, w=100, h=100)
dstrect=(x=384, y=-2147483583, w=100, h=100)

[1bsyl]

@oddbookworm that sounds really strange, since it's common function and has lots of checks: https://github.com/libsdl-org/SDL/blob/main/src/video/SDL_surface.c#L666

can you try a c test case ? double-check your sdl version ?

[weirddan455]

I was able to reproduce this in C.

#include <SDL2/SDL.h>
#include <stdio.h>

int main(void)
{
    SDL_Init(SDL_INIT_VIDEO);
    SDL_Window *window = SDL_CreateWindow("Test", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 385, 166, 0);
    SDL_Surface *surface = SDL_CreateRGBSurfaceWithFormat(0, 100, 100, 32, SDL_PIXELFORMAT_RGBA32);
    SDL_Surface *window_surface = SDL_GetWindowSurface(window);
    SDL_Rect dst_rect;
    dst_rect.x = 384;
    dst_rect.y = -2147483583;
    dst_rect.w = 100;
    dst_rect.h = 100;
    SDL_BlitSurface(surface, NULL, window_surface, &dst_rect);
    return 0;
}

It's an integer overflow that happens when calculating clipping: https://github.com/libsdl-org/SDL/blob/fd42a2f994c73e6c87b3eebfddd3a2d6829492af/src/video/SDL_surface.c#L722-L748

Is adding overflow checks to this function something we want to be doing? Or should we assume most users aren't going to pass in silly values like that?

[1bsyl]

yes, the is a bug here.... here the same sample case that crashes with SDL3:

#include <SDL3/SDL.h>
#include <stdio.h>

int main(void)
{
    SDL_Init(SDL_INIT_VIDEO);
    SDL_Window *window = SDL_CreateWindow("Test", 385, 166, 0);
    SDL_Surface *surface = SDL_CreateSurface(100, 100, SDL_PIXELFORMAT_RGBA32);
    SDL_Surface *window_surface = SDL_GetWindowSurface(window);
    SDL_Rect dst_rect;
    dst_rect.x = 384;
    dst_rect.y = -2147483583;
    dst_rect.w = 100;
    dst_rect.h = 100;
    SDL_BlitSurface(surface, NULL, window_surface, &dst_rect);
    return 0;
}

[1bsyl]

I've a diff that isn't working yet:

I think the clipping shouldn't be inlined, and it would be better to use SDL_GetRectIntersection functions, so that the blit looks like this.

(and maybe the SDL_GetRectIntersection should be protected against overlow)

int SDL_BlitSurface(SDL_Surface *src, const SDL_Rect *srcrect,
                  SDL_Surface *dst, SDL_Rect *dstrect)
{
    SDL_Rect r_src, r_dst;

    /* Make sure the surfaces aren't locked */
    if (!src) {
        return SDL_InvalidParamError("src");
    } else if (!dst) {
        return SDL_InvalidParamError("dst");
    } else if (src->locked || dst->locked) {
        return SDL_SetError("Surfaces must not be locked during blit");
    }

    r_src.x = 0;
    r_src.y = 0;
    r_src.w = src->w;
    r_src.h = src->h;

    r_dst.x = 0;
    r_dst.y = 0;
    r_dst.w = dst->w;
    r_dst.h = dst->h;

    /* src surface intersect with srcrect */
    if (srcrect) {
        SDL_Rect tmp;
        SDL_GetRectIntersection(srcrect, &r_src, &tmp);
        r_dst.x += tmp.x - r_src.x;
        r_dst.y += tmp.y - r_src.y;
        r_src = tmp;
    }

    /* dst surface intersect with dstrect */
    if (dstrect) {
        SDL_GetRectIntersection(dstrect, &r_dst, &r_dst);
    }

    /* clip the destination rectangle against the clip rectangle */
    {
        SDL_Rect r_dst_clipped;
        SDL_GetRectIntersection(&r_dst, &dst->clip_rect, &r_dst_clipped);

        /* And update src rect */
        r_src.x += r_dst_clipped.x - r_dst.x;
        r_src.y += r_dst_clipped.y - r_dst.y;
        r_src.w += r_dst_clipped.w - r_dst.w;
        r_src.h += r_dst_clipped.h - r_dst.h;

        /* Use the clipped version */
        r_dst = r_dst_clipped;
    }

    /* Switch back to a fast blit if we were previously stretching */
    if (src->map->info.flags & SDL_COPY_NEAREST) {
        src->map->info.flags &= ~SDL_COPY_NEAREST;
        SDL_InvalidateMap(src->map);
    }

    if (dstrect) {
        *dstrect = r_dst;
    }

    if (r_dst.w > 0 && r_dst.h > 0) {
        return SDL_BlitSurfaceUnchecked(src, &r_src, dst, &r_dst);
    }

    return 0;
}

[1bsyl]

@weirddan455 hey can you try the previous patch ! that should fix the issue !

[slouken]

I believe this is fixed in the latest SDL release. Please feel free to reopen this if that's not the case.

[1bsyl]

PR https://github.com/libsdl-org/SDL/pull/7808

[1bsyl]

adding more debugs here:

src/video/SDL_surface.c:765:12: runtime error: signed integer overflow: -2147483483 - 166 cannot be represented in type 'int'
src/video/SDL_surface.c:767:15: runtime error: signed integer overflow: -2147483483 - 2147483647 cannot be represented in type 'int'

where the code is:

 765         dy = dstrect->y + h - clip->y - clip->h;
 766         if (dy > 0) {
 767             h -= dy;
 768         }

so, maybe we can go for the fix: https://github.com/libsdl-org/SDL/pull/8682