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 #include #include /* 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
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: > > > >> 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 #include #include /* 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 ove
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: > 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 -~--~~~~--~~--~--~---
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
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. -- hundred-and-one symptoms of being an internet addict: 204. You're being audited because you mailed your tax return to the IRC. /// Bram Moolenaar -- [EMAIL PROTECTED] -- http://www.Moolenaar.net \\\ ///sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\download, build and distribute -- http://www.A-A-P.org/// \\\help me help AIDS victims -- http://ICCF-Holland.org/// --~--~-~--~~~---~--~~ 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;