Re: [patch] fixed overlapping memcpy() in if_xcmdsrv.c
2008/11/9 Matt Wozniski [EMAIL PROTECTED]:
On Sat, Nov 8, 2008 at 11:18 PM, Tony Mechelynck wrote:
Configure is supposed to check whether one of the system provided
string-move operations handle overlap. Here's what I see in the logs and
files produced by configure on my system:
snip
So I suppose mch_memmove should be used everywhere for moves of byte
strings (overlapping or not), and it will be resolved by bcopy, memmove,
memcpy or the owncoded function according to what configure has found.
You're right up til this point, but mch_memmove() should only be used
where the bytes are overlapping, since it's so much less efficient
than just a normal memcpy() and that loss is only justified when its
extra feature is being used. memmove() should never be used in the
vim sources.
~Matt
Yes, memcpy() is more efficient then memmove() for copying
memory, and can/should be used when there we can guarantee
that there is such overlap. If there can be overlap, memmove()
must be used, but it does not exists on all systems, hence the
need for mch_memmove() for portability. See man pages of
memcpy() memmove().
In practice, memcpy() may actually work on some system,
or may not work depending on compiler, optimization options
or whether copy goes upward or downward, etc. In any case,
don't rely on it, it's not portable.
I just wrote a simple test case, to see how my system (linux x86)
behaves, it shows that memcpy() does not work in practice for
overlapping memory:
$ gcc -Wall -o test-memcpy-memmove test-memcpy-memmove.c
$ ./memcpy-memmove
testing descending memcpy() with overlapping mem...OK
testing ascending memcpy() with overlapping mem...FAIL
expected=[abcdeabcdeabcdepqrstuvwxyz]
actual =[abcdeabcdefghijpqrstuvwxyz]
testing descending memmove() with overlapping mem...OK
testing ascending memmove() with overlapping mem...OK
Interestingly, running the same test case under valgrind gives
different results:
$ valgrind ./test-memcpy-memmove 2 vg.log
testing descending memcpy() with overlapping mem...OK
testing ascending memcpy() with overlapping mem...OK
testing descending memmove() with overlapping mem...OK
testing ascending memmove() with overlapping mem...OK
Results may be different on other systems, but at least,
on Linux x86, using memcpy() for overlapping memory
does not give the correct results. So don't treat those
overlapping memory messages from valgrind as theoretical
bugs, but as real severe nasty bugs.
I attach the source code of my test case if you want to
run it on your system.
-- Dominique
--~--~-~--~~~---~--~~
You received this message from the vim_dev maillist.
For more information, visit http://www.vim.org/maillist.php
-~--~~~~--~~--~--~---
/*
* Test whether memcpy()/memmove() give the correct
* results when moving overlapping memory upward or
* backward. mempcy() is not guarantee to be correct
* when memory overlaps, memmove() should be correct.
*
* Results on my system (Linux x86):
*
* $ gcc -O2 -Wall -o test-memcpy-memmove test-memcpy-memmove.c
* $ ./test-memcpy-memmove
* testing descending memcpy() with overlapping mem...OK
* testing ascending memcpy() with overlapping mem...FAIL
* expected=[abcdeabcdefghijpqrstuvwxyz]
* actual =[abcdeabcdeabcdepqrstuvwxyz]
* testing descending memmove() with overlapping mem...OK
* testing ascending memmove() with overlapping mem...OK
*
* Dominique Pelle [EMAIL PROTECTED]
*/
#include string.h
#include stdio.h
#include stdlib.h
/* Compare expected and actual results, and print outcome */
static void compare(const char *expected, const char *actual)
{
if (strcmp(expected, actual) == 0) {
printf(OK\n);
} else {
printf(FAIL\n
expected=[%s]\n
actual =[%s]\n, expected, actual);
}
}
int main()
{
/* 0 1 2
* .01234567890123456789012345 */
const char *pristine_str = abcdefghijklmnopqrstuvwxyz;
char *str;
/* Expected results when copying 10 char by 5 positions (hence
* overlapping memory).
*
* When copying downward memmove(str, str + 5, 10)
* (.) for unchanged char (X) for changed char:
*
* pristine: abcdefghijklmnopqrstuvwxyx
* XX */
const const char *expected1 = fghijklmnoklmnopqrstuvwxyz;
/*
* When copying upward memmove(str + 5, str, 10)
* (.) for unchanged char (X) for changed char:
*
* pristine: abcdefghijklmnopqrstuvwxyx
* .XX... */
const const char *expected2 = abcdeabcdefghijpqrstuvwxyz;
printf(testing descending memcpy() with overlapping mem...);
str = strdup(pristine_str);
memcpy(str, str + 5, 10);
compare(str, expected1);
printf(testing ascending memcpy() with overlapping mem...);
Re: [patch] fixed overlapping memcpy() in if_xcmdsrv.c
2008/11/9 Dominique Pelle [EMAIL PROTECTED]:
$ gcc -Wall -o test-memcpy-memmove test-memcpy-memmove.c
$ ./memcpy-memmove
testing descending memcpy() with overlapping mem...OK
testing ascending memcpy() with overlapping mem...FAIL
expected=[abcdeabcdeabcdepqrstuvwxyz]
actual =[abcdeabcdefghijpqrstuvwxyz]
testing descending memmove() with overlapping mem...OK
testing ascending memmove() with overlapping mem...OK
Oops, small mistake, which does not change the conclusion,
output should have been:
$ ./memcpy-memmove
testing descending memcpy() with overlapping mem...OK
testing ascending memcpy() with overlapping mem...FAIL
expected=[abcdeabcdefghijpqrstuvwxyz]
actual =[abcdeabcdeabcdepqrstuvwxyz]
testing descending memmove() with overlapping mem...OK
testing ascending memmove() with overlapping mem...OK
-- Dominique
--~--~-~--~~~---~--~~
You received this message from the vim_dev maillist.
For more information, visit http://www.vim.org/maillist.php
-~--~~~~--~~--~--~---
/*
* Test whether memcpy()/memmove() give the correct
* results when moving overlapping memory upward or
* backward. mempcy() is not guarantee to be correct
* when memory overlaps, memmove() should be correct.
*
* Results on my system (Linux x86):
*
* $ gcc -O2 -Wall -o test-memcpy-memmove test-memcpy-memmove.c
* $ ./test-memcpy-memmove
* testing descending memcpy() with overlapping mem...OK
* testing ascending memcpy() with overlapping mem...FAIL
* expected=[abcdeabcdefghijpqrstuvwxyz]
* actual =[abcdeabcdeabcdepqrstuvwxyz]
* testing descending memmove() with overlapping mem...OK
* testing ascending memmove() with overlapping mem...OK
*
* Dominique Pelle [EMAIL PROTECTED]
*/
#include string.h
#include stdio.h
#include stdlib.h
/* Compare expected and actual results, and print outcome */
static void compare(const char *expected, const char *actual)
{
if (strcmp(expected, actual) == 0) {
printf(OK\n);
} else {
printf(FAIL\n
expected=[%s]\n
actual =[%s]\n, expected, actual);
}
}
int main()
{
/* 0 1 2
* .01234567890123456789012345 */
const char *pristine_str = abcdefghijklmnopqrstuvwxyz;
char *str;
/* Expected results when copying 10 char by 5 positions (hence
* overlapping memory).
*
* When copying downward memmove(str, str + 5, 10)
* (.) for unchanged char (X) for changed char:
*
* pristine: abcdefghijklmnopqrstuvwxyx
* XX */
const const char *expected1 = fghijklmnoklmnopqrstuvwxyz;
/*
* When copying upward memmove(str + 5, str, 10)
* (.) for unchanged char (X) for changed char:
*
* pristine: abcdefghijklmnopqrstuvwxyx
* .XX... */
const const char *expected2 = abcdeabcdefghijpqrstuvwxyz;
printf(testing descending memcpy() with overlapping mem...);
str = strdup(pristine_str);
memcpy(str, str + 5, 10);
compare(expected1, str);
printf(testing ascending memcpy() with overlapping mem...);
strcpy(str, pristine_str);
memcpy(str + 5, str, 10);
compare(expected2, str);
printf(testing descending memmove() with overlapping mem...);
strcpy(str, pristine_str);
memmove(str, str + 5, 10);
compare(expected1, str);
printf(testing ascending memmove() with overlapping mem...);
strcpy(str, pristine_str);
memmove(str + 5, str, 10);
compare(expected2, str);
free(str);
return 0;
}
Re: [patch] fixed overlapping memcpy() in if_xcmdsrv.c
On 08/11/08 14:24, Bram Moolenaar wrote:
Dominique Pelle wrote:
While using Vim-7.2.30 built with GUI GTK (huge), I stumbled
upon this error with Valgrind memory checker:
==13326== Source and destination overlap in memcpy(0x4B5D8D8, 0x4B5D8E4, 13)
==13326==at 0x4024C92: memcpy (mc_replace_strmem.c:402)
==13326==by 0x8102E99: LookupName (if_xcmdsrv.c:1021)
==13326==by 0x810197E: DoRegisterName (if_xcmdsrv.c:303)
==13326==by 0x8101718: serverRegisterName (if_xcmdsrv.c:225)
==13326==by 0x81076D1: prepare_server (main.c:3379)
==13326==by 0x8104253: main (main.c:721)
Attached patch fixes it by calling mch_memmove() instead of memcpy().
Looking at the code, I found another place where the same problem would
also happen (also fixed in patch).
Some versions of memcpy() can handle overlaps, but I suppose it's not
guaranteed. Thanks for another patch.
Configure is supposed to check whether one of the system provided
string-move operations handle overlap. Here's what I see in the logs and
files produced by configure on my system:
-- in configure stdout/stderr:
checking whether memmove handles overlaps... yes
-- in src/auto/config.log
configure:15252: checking whether memmove handles overlaps
configure:15274: gcc -o conftest -O2 -fno-strength-reduce -Wall -L.
-rdynamic -Wl,-export-dynamic -Wl,-E
-Wl,-rpath,/usr/lib/perl5/5.10.0/i586-linux-thread-multi/CORE
-L/usr/local/lib conftest.c -lm -lncurses -lnsl -lacl -lattr -lgpm 5
conftest.c:10: warning: return type defaults to 'int'
configure:15278: $? = 0
configure:15284: ./conftest
configure:15288: $? = 0
configure:15310: result: yes
-- in src/auto/config.cache:
vim_cv_memmove_handles_overlap=${vim_cv_memmove_handles_overlap=yes}
-- in src/auto/config.h
/*
* If we cannot trust one of the following from the libraries, we use our
* own safe but probably slower vim_memmove().
*/
/* #undef USEBCOPY */
#define USEMEMMOVE 1
/* #undef USEMEMCPY */
-- in src/os_unix.h:
/* memmove is not present on all systems, use memmove, bcopy, memcpy or our
* own version */
/* Some systems have (void *) arguments, some (char *). If we use (char
*) it
* works for all */
#ifdef USEMEMMOVE
# define mch_memmove(to, from, len) memmove((char *)(to), (char
*)(from), len)
#else
# ifdef USEBCOPY
# define mch_memmove(to, from, len) bcopy((char *)(from), (char *)(to),
len)
# else
# ifdef USEMEMCPY
# define mch_memmove(to, from, len) memcpy((char *)(to), (char
*)(from), len)
# else
# define VIM_MEMMOVE /* found in misc2.c */
# endif
# endif
#endif
-- in src/misc2.c:
#ifdef VIM_MEMMOVE
/*
* Version of memmove() that handles overlapping source and destination.
* For systems that don't have a function that is guaranteed to do that
(SYSV).
*/
void
mch_memmove(dst_arg, src_arg, len)
void*src_arg, *dst_arg;
size_t len;
{
/*
* A void doesn't have a size, we use char pointers.
*/
char *dst = dst_arg, *src = src_arg;
/* overlap, copy backwards */
if (dst src dst src + len)
{
src += len;
dst += len;
while (len-- 0)
*--dst = *--src;
}
else /* copy forwards */
while (len-- 0)
*dst++ = *src++;
}
#endif
So I suppose mch_memmove should be used everywhere for moves of byte
strings (overlapping or not), and it will be resolved by bcopy, memmove,
memcpy or the owncoded function according to what configure has found.
Best regards,
Tony.
--
This Fortue Examined By INSPECTOR NO. 2-14
--~--~-~--~~~---~--~~
You received this message from the vim_dev maillist.
For more information, visit http://www.vim.org/maillist.php
-~--~~~~--~~--~--~---
Re: [patch] fixed overlapping memcpy() in if_xcmdsrv.c
On Sat, Nov 8, 2008 at 11:18 PM, Tony Mechelynck wrote: Configure is supposed to check whether one of the system provided string-move operations handle overlap. Here's what I see in the logs and files produced by configure on my system: snip So I suppose mch_memmove should be used everywhere for moves of byte strings (overlapping or not), and it will be resolved by bcopy, memmove, memcpy or the owncoded function according to what configure has found. You're right up til this point, but mch_memmove() should only be used where the bytes are overlapping, since it's so much less efficient than just a normal memcpy() and that loss is only justified when its extra feature is being used. memmove() should never be used in the vim sources. ~Matt --~--~-~--~~~---~--~~ You received this message from the vim_dev maillist. For more information, visit http://www.vim.org/maillist.php -~--~~~~--~~--~--~---
[patch] fixed overlapping memcpy() in if_xcmdsrv.c
Hi While using Vim-7.2.30 built with GUI GTK (huge), I stumbled upon this error with Valgrind memory checker: ==13326== Source and destination overlap in memcpy(0x4B5D8D8, 0x4B5D8E4, 13) ==13326==at 0x4024C92: memcpy (mc_replace_strmem.c:402) ==13326==by 0x8102E99: LookupName (if_xcmdsrv.c:1021) ==13326==by 0x810197E: DoRegisterName (if_xcmdsrv.c:303) ==13326==by 0x8101718: serverRegisterName (if_xcmdsrv.c:225) ==13326==by 0x81076D1: prepare_server (main.c:3379) ==13326==by 0x8104253: main (main.c:721) Attached patch fixes it by calling mch_memmove() instead of memcpy(). Looking at the code, I found another place where the same problem would also happen (also fixed in patch). Cheers -- Dominique --~--~-~--~~~---~--~~ You received this message from the vim_dev maillist. For more information, visit http://www.vim.org/maillist.php -~--~~~~--~~--~--~--- Index: if_xcmdsrv.c === RCS file: /cvsroot/vim/vim7/src/if_xcmdsrv.c,v retrieving revision 1.11 diff -c -r1.11 if_xcmdsrv.c *** if_xcmdsrv.c 6 Aug 2008 16:38:13 - 1.11 --- if_xcmdsrv.c 7 Nov 2008 19:47:32 - *** *** 1018,1024 p++; count = numItems - (p - regProp); if (count 0) ! memcpy(entry, p, count); XChangeProperty(dpy, RootWindow(dpy, 0), registryProperty, XA_STRING, 8, PropModeReplace, regProp, (int)(numItems - (p - entry))); --- 1018,1024 p++; count = numItems - (p - regProp); if (count 0) ! mch_memmove(entry, p, count); XChangeProperty(dpy, RootWindow(dpy, 0), registryProperty, XA_STRING, 8, PropModeReplace, regProp, (int)(numItems - (p - entry))); *** *** 1072,1078 p++; lastHalf = numItems - (p - regProp); if (lastHalf 0) ! memcpy(entry, p, lastHalf); numItems = (entry - regProp) + lastHalf; p = entry; continue; --- 1072,1078 p++; lastHalf = numItems - (p - regProp); if (lastHalf 0) ! mch_memmove(entry, p, lastHalf); numItems = (entry - regProp) + lastHalf; p = entry; continue;
