This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Test case (sdl_undefined_memcpy.c, text/plain, 2011-02-16 07:14:16 +0000, 2668 bytes)
• replace SDL_memcpy() by SDL_memmove() patch (SDL_blit.c.patch, text/plain, 2011-02-16 13:53:17 +0000, 350 bytes)
• Patch for the 1.3 branch (SDL_blit.c.patch-1.3, text/plain, 2011-02-17 01:20:03 +0000, 402 bytes)
• Complete test case (sdl_undefined_memcpy.c, text/plain, 2011-02-17 08:55:29 +0000, 5800 bytes)

Reported in version: HG 2.0
Reported for operating system, platform: Linux, x86_64

Comments on the original bug report:

On 2011-01-18 06:59:12 +0000, Petr Pisar wrote:

SDL_BlitCopyOverlap() copies data between overlapping bitmaps. The 1.2 branch (1.2.14 version and 1.2 branch head) has following problem:

The function will call SDL_memcpy() if destination is before source. SDL_memcpy() is implemented as memcpy(3) provided by standard library on some platforms (e.g. x86_64). Problem is POSIX says:

If copying takes place between objects that overlap, the behavior is undefined.

And this exactly happens with latest glibc that implements memcpy() using SSE3 instructions on some platforms and results in corrupted memory.

One would attempt to replace the SDL_memcpy() call by SDL_memmove(). That would be good except SDL_memmove() defined in include/SDL_stdinc.h can be implemented manually (on some platforms) and the implementation is wrong again as it relies on SDL_memcpy() and thus system memcpy(3) again.

You can see this bug in action in Wesnoth in Fedora 14 distribution while scrolling the board to right direction. This bug has been originally reported to Fedora's bug tracking system https://bugzilla.redhat.com/show_bug.cgi?id=669844

On 2011-02-16 07:14:16 +0000, Petr Pisar wrote:

Created attachment 573
Test case

This is simple test blitting the horizontally backward overlapping rectangles.

Complete test would need to test overlapping in vertical direction and in forward direction to exclude any regressions.

Also this test does not check system memcpy(3) is affected thus when running on unaffected system, the test case does not prove anything.

If you are interested, I can improve the test to cover all the ideas.

On 2011-02-16 11:50:06 +0000, Sam Lantinga wrote:

Yes, that would be great.

Thanks!

On 2011-02-16 13:43:00 +0000, Tolga Dalman wrote:

(In reply to comment # 0)

One would attempt to replace the SDL_memcpy() call by SDL_memmove().

I suggest that SDL_memcpy() in SDL_BlitCopyOverlap() is replaced by
SDL_memmove() in any case.

That would
be good except SDL_memmove() defined in include/SDL_stdinc.h can be implemented
manually (on some platforms) and the implementation is wrong again as it relies
on SDL_memcpy() and thus system memcpy(3) again.

Can you state, which platforms are affected - i.e., where memmove() is really unavailable ? Does memcpy() on overlapping areas also break there ?

You can see this bug in action in Wesnoth in Fedora 14 distribution while
scrolling the board to right direction. This bug has been originally reported
to Fedora's bug tracking system
https://bugzilla.redhat.com/show_bug.cgi?id=669844

Yes. Please see for the Gentoo bug report:
http://bugs.gentoo.org/show_bug.cgi?id=354175.

Thanks!

On 2011-02-16 13:53:17 +0000, Tolga Dalman wrote:

Created attachment 574
replace SDL_memcpy() by SDL_memmove() patch

As Petr suggested, here's the trivial patch.

On 2011-02-16 15:26:32 +0000, Sam Lantinga wrote:

This is fixed in SDL 1.3, thanks!
http://hg.libsdl.org/SDL/rev/b9c224e16859

On 2011-02-16 23:32:35 +0000, Tolga Dalman wrote:

Thanks, Sam!

How about applying this patch to the 1.2 branch also ? I don't think this is
a regression of any kind ...

On 2011-02-17 01:05:34 +0000, Sam Lantinga wrote:

I don't have time right now, but if you post a patch I'll apply it.

Thanks!

On 2011-02-17 01:20:03 +0000, Tolga Dalman wrote:

Created attachment 577
Patch for the 1.3 branch

The previous patch had been against 1.2.

On 2011-02-17 02:33:27 +0000, Sam Lantinga wrote:

The 1.2 fix is in, thanks!
http://hg.libsdl.org/SDL/rev/d898ee5431f5

On 2011-02-17 08:55:29 +0000, Petr Pisar wrote:

Created attachment 579
Complete test case

This augmented test checks for rectangles overlapping horizontally, vertically, and in both directions (destination before source and vice versa). I addition it checks if memcpy(3) provided by standard system is affected.

I must admit I could not have found reliable way who to make affected memcpy() to fail. It usually fails on my system, sometimes it doesn't. OTOH, the SDL blitter fails always. Also memory layout and all the offsets and lengths are important. If you change them you can get different results.

Despite all of that I believe the test is helpful as it fails on SDL-1.2.14 release and it passes on SDL-1.2 tip (at least on my system).

On 2011-02-17 09:05:15 +0000, Petr Pisar wrote:

(In reply to comment # 3)

(In reply to comment # 0)

That would
be good except SDL_memmove() defined in include/SDL_stdinc.h can be
implemented manually (on some platforms) and the implementation is wrong
again as it relies on SDL_memcpy() and thus system memcpy(3) again.

Can you state, which platforms are affected - i.e., where memmove() is really
unavailable ? Does memcpy() on overlapping areas also break there ?

I have no real system that offers affected memcpu() and does not provide memmove().

Just while reading the code and playing with some ifdefs I found and verified that system memcpy() could be called as implementation of SDL's memmove(). I believe such crazy system does nobody use, but I just wanted to be precise and to point there could be a problem.

On 2011-02-18 02:40:28 +0000, Petr Pisar wrote:

Maybe my hg-fu is low today, talking about 1.2 branch, but changeset d898ee5431f5 has different parrent than previous changeset and squashes everything after 6bb01861c4c0, including b9c224e16859 (The MacOSX memcpy tunes). Is it Ok?

On 2011-02-18 09:08:40 +0000, Sam Lantinga wrote:

Yep, they're in different branches, so it's fine:
http://hg.libsdl.org/SDL/graph