We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3340

Summary: SDL_BlitScaled causes access violation in some cases. Possible error in rect calculation.
Product: SDL Reporter: Simon Hug <chli.hug>
Component: videoAssignee: Sam Lantinga <slouken>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: sylvain.becker
Version: HG 2.0   
Hardware: All   
OS: All   
Attachments: Testcase for a scaled blit that causes an access violation.
patch
test case

Description Simon Hug 2016-05-20 19:06:24 UTC 
Created attachment 2460 [details]
Testcase for a scaled blit that causes an access violation.

The SDL_BlitScaled function runs into an access violation for specific blit coordinates and surface sizes. The attached testcase blits a 800x600 surface to a 1280x720 surface at the coordinates -640,-345 scaled to 1280x720. The blit function that moves the data then runs over and reads after the pixel data from the src surface causing an access violation.

I can't say where exactly it goes wrong, but I think it could have something to do with the rounding in SDL_UpperBlitScaled. final_src.y is 288 and final_src.h is 313. Together that's 601, which I believe is one too much, but I just don't know the code enough to make sure that's the problem.

Call stack looks like this:

SDL2!SDL_Blit_Slow+0x66f [src\video\sdl_blit_slow.c @ 75]
SDL2!SDL_SoftBlit+0x225 [src\video\sdl_blit.c @ 88]
SDL2!SDL_LowerBlit_REAL+0xc1 [src\video\sdl_surface.c @ 518]
SDL2!SDL_LowerBlitScaled_REAL+0x12b [src\video\sdl_surface.c @ 795]
SDL2!SDL_UpperBlitScaled_REAL+0x7ee [src\video\sdl_surface.c @ 768]
SDL2!SDL_UpperBlitScaled+0x32 [src\dynapi\sdl_dynapi_procs.h @ 504]
scaled_blit_test!SDL_main+0xd6 [scaled_blit_test.c @ 17]
scaled_blit_test!main_utf8+0x20 [src\main\windows\sdl_windows_main.c @ 127]
scaled_blit_test!main+0x1b [src\main\windows\sdl_windows_main.c @ 135]

That's on Windows, obviously. Linux doesn't segfault here, but Valgrind notes an invalid read in the same function.

I encountered this when testing TBFTSS (https://forums.libsdl.org/viewtopic.php?t=11544) with the SDL software renderer.
Comment 1 Sylvain 2016-11-03 11:59:18 UTC 
Created attachment 2600 [details]
patch

I think this patch fix the issue, but maybe it's worth re-writing "SDL_UpperBlitScaled" using SDL_FRect.
Comment 2 Sam Lantinga 2016-11-06 17:28:20 UTC 
Simon, can you verify that this fixes your crash and works correctly?

Thanks!
Comment 3 Simon Hug 2016-11-08 13:15:09 UTC 
Yes, that fixes it. Seems to work correctly.

It looks like the rounding of the width also had the same issue. I didn't notice it before, but my testing showed that certain SDL_Rect values could cause wrong rounding, the scaler would overread and use the first pixel of the next row. All fixed with that patch.
Comment 4 Sam Lantinga 2016-11-09 04:47:28 UTC 
Thanks!
Comment 5 Sylvain 2016-11-29 17:48:49 UTC 
Hello, I am just noticing I still have the diff in my repos.
So this means nothing has been merged for this ticket. 
Is this getting lost or does it need more testing?
Comment 6 Sam Lantinga 2016-11-30 19:15:47 UTC 
I think your patch is no longer necessary. I believe the bug was fixed by another patch.
Comment 7 Sylvain 2016-11-30 19:33:49 UTC 
Created attachment 2641 [details]
test case

I just re-tested it with the latest trunk and this still exists: this is detected by valgrind as an invalid read:

==17367== Invalid read of size 4
==17367==    at 0x4F3C0D7: SDL_Blit_Slow (SDL_blit_slow.c:77)
==17367==    by 0x4EF2734: SDL_SoftBlit (SDL_blit.c:88)
==17367==    by 0x4F48767: SDL_LowerBlit_REAL (SDL_surface.c:553)
==17367==    by 0x4F4931C: SDL_LowerBlitScaled_REAL (SDL_surface.c:830)
==17367==    by 0x4F491DC: SDL_UpperBlitScaled_REAL (SDL_surface.c:803)
==17367==    by 0x4E7727B: SDL_UpperBlitScaled (SDL_dynapi_procs.h:505)
==17367==    by 0x108A1D: main (bug_3340_blitscaled.c:24)
==17367==  Address 0x9c36000 is 0 bytes after a block of size 1,920,000 alloc'd
==17367==    at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17367==    by 0x4EE6634: SDL_malloc_REAL (SDL_malloc.c:36)
==17367==    by 0x4F479B5: SDL_CreateRGBSurfaceWithFormat_REAL (SDL_surface.c:83)
==17367==    by 0x4F47E4E: SDL_CreateRGBSurface_REAL (SDL_surface.c:127)
==17367==    by 0x4E76B56: SDL_CreateRGBSurface (SDL_dynapi_procs.h:478)
==17367==    by 0x1089A9: main (bug_3340_blitscaled.c:12)

I attached my testcase!
Comment 8 Sylvain 2016-11-30 19:37:02 UTC 
Marked as reopened
Comment 9 Sam Lantinga 2016-12-01 06:06:29 UTC 
Fixed, thanks!
https://hg.libsdl.org/SDL/rev/1889c850fafc
Comment 10 Sylvain 2021-01-02 08:48:07 UTC 
for the record:
https://hg.libsdl.org/SDL/rev/7101c5a0d561

- better fix to clip after scaling computation is done, it was still crash in some case
- add back back SDL_round (removed in bug #2687, added in bug #5404)