This is the mail archive of the glibc-bugs@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug libc/12518] New: memcpy acts randomly (and differently) with overlapping areas


http://sourceware.org/bugzilla/show_bug.cgi?id=12518

           Summary: memcpy acts randomly (and differently) with
                    overlapping areas
           Product: glibc
           Version: 2.13
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
        AssignedTo: drepper.fsp@gmail.com
        ReportedBy: torvalds@linux-foundation.org


Created attachment 5264
  --> http://sourceware.org/bugzilla/attachment.cgi?id=5264
Example patch to give the basic idea

I realize that it's technically "undefined", but the behavior has changed, and
in the process broken existing binaries. It's a common bug to use memcpy()
instead of memmove(), and the traditional behavior of "copy upwards" means that
that bug can go unnoticed for a *long* time if the memory move moves things
downwards.

The change was introduced in commit 6fb8cbcb58a29fff73eb2101b34caa19a7f88eba
("Improve 64bit memcpy/memmove for Atom, Core 2 and Core i7"), which was part
of the 2.13 release.

As a result, upgrading from Fedora-13 to Fedora-14 caused applications like
flash to fail. But it's a bug that has been reported for other applications too
(and it's a bug that I've personally introduced in 'git' too - happily, people
had run things like valgrind, so I don't know of any _current_ cases of it).

And there is absolutely _zero_ reason to not handle overlapping areas
correctly. The "undefined behavior" is due to historical implementation issues,
not because it's better than just doing the right thing. 

And now applications will randomly do different things depending on the phase
of the moon (technically, depending on which CPU they have and what particular
version of memcpy() glibc happens to choose).

So from a pure Quality-of-Implementation standpoint, just make glibc implement
memcpy() as memmove(), if you don't want to re-instate the traditional behavior
that binaries have at least been tested with. Don't break random existing
binaries just because the glibc version changed, and they had never been tested
with the new random behavior!

I'm attaching an EXAMPLE patch against the current glibc git tree: it just
tries to get rid of the unnecessary differences between memcpy and memmove for
the normal ssse3 case. The approach is simple:

 - small copies (less than 80 bytes) have hand-coded optimized code that gets
called through a jump table. Those cases already handle overlapping areas
correctly (simply because they do all loads up-front, and stores at the end),
and they are used for memmove() as-is. 

   So change the memcpy() function to test for the small case first, since
that's the one that can be latency critical.

 - once we're talking about bigger memcpy() cases, the extra couple of cycles
to check "which direction should I copy" are totally immaterial, and having two
different versions of basically the same code is just silly and is quite likely
to cost more than (in I$ and page fault overhead) than just doing it in the
same code. So just remove all the USE_AS_MEMMOVE games.

I haven't actually tested the patch, since building glibc is black magic and
the  simple "just alias __memmove_ssse3 to __memcpy_sse3" thing didn't work for
me and resulted in linker errors. There's probably some simple (but subtle)
magic reason for that, that I simply don't know about.

So take the patch as a "hey, doing it this way should be simpler and avoid the
problem" rather than "apply this patch as-is". The ssse3-back case (which
prefers backwards copies) needs similar treatment, I'll happily do that and
test it if people just let me know that it's worth my time (and tell me what
the black magic incantation is).

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]