4 ms·
There is a bug in the memmove() function from ulib.c: void* memmove(void *vdst, void *vsrc, int n) { char *dst, *src; dst = vdst;
by ssp 15y ago
There is a bug in the memmove() function from ulib.c:
void*
memmove(void *vdst, void *vsrc, int n)
{
char *dst, *src;
dst = vdst;
src = vsrc;
while(n-- > 0)
*dst++ = *src++;
return vdst;
}
If the two blocks overlap, and the address of dst is greater than the address of src, then parts of the source will be overwritten before it can be copied.
- agumonkey 15y agoreminds me of the intel comment in their implementation of memcpy, something like 'no overlay check as implicit convention'
- microtherion 15y agoYes, the standards say that the behavior of memcpy, as opposed to memmove, is undefined when the arguments overlap.
- robinhouston 15y agoThe original v6 doesn’t have any of the mem* functions, but its bcopy implementation has the same issue. Presumably the convention that bcopy permits overlap had not been established at that time.
- derleth 15y agoRight. Quoting from the manpage: > The memory areas may overlap: copying takes place as though the bytes in src are first copied into a temporary array that does not overlap src or dest, and the bytes are then copied from the temporary array to dest. http://www.kernel.org/doc/man-pages/online/pages/man3/memmove.3.html http://www.kernel.org/doc/man-pages/online/pages/man3/memmov... Notice how it says 'as though'; I can't see why doing the copy backwards (from end to start) wouldn't work, myself.
- eddington 15y agoStill doesn't work going end to start. src starts at 50, ends at 100 dst starts at 25, ends at 75. First we copy 100 to 75, then 99 to 74, etc ... now we get to copy from 75 to 50, but we've overwritten it already!
- sp332 15y agoIf it's higher, move from the front. If it's lower, start at the back. You have to check each time you do it.
- suivix 15y agoLinux isn't Unix, so are you sure it's the same?
- antihero 15y agoI'm not massively great at C but would this be a fix? void* memmove(void *vdst, void *vsrc, int n) { char *dst, *src; dst = vdst; src = vsrc; if(*vdst > *vsrc) { *vdst += n; *vsrc += n; while(n-- > 0) *dst-- = *src--; } else if(*vdst < *vsrc) while(n-- > 0) *dst++ == *src++; return vdst; }
- unwantedLetters 15y agoI think this might have an off-by-one error in the case you've corrected. I'm not sure I remember how to write this in C, the correction might be: --dst = --src; OR (--dst) = (--src); edit: I have the dereferences in there, but I don't know how to correctly format it so they show up. How do you format the comment to have code show up?
- abecedarius 15y agoAt least two bugs: the test should compare vdst to vsrc, not * vdst to * vsrc, and in the first branch you're copying vsrc[n] which is out of range (and skipping vsrc[0]).
- antihero 15y agoThe first bug I'm confused? Surely we want to use the * because we're comparing the position in memory, not what's in the memory? Or does the * mean "compare what is in that memory slot"? Second bug, cheers. http://news.ycombinator.com/item?id=3214807 http://news.ycombinator.com/item?id=3214807
- simcop2387 15y agoI see an additional bug in there compared to the original, you're copying the memory below vsrc into the memory below vdst in the first branch. you need to add to dst and src not vdst/vsrc.
- antihero 15y agoThanks, C rather rusty :) http://news.ycombinator.com/item?id=3214807 http://news.ycombinator.com/item?id=3214807
- suivix 15y agoHere is a standard implementation I found from some code at work. /* memmove.c -- copy memory. Copy LENGTH bytes from SOURCE to DEST. Does not null-terminate. In the public domain. By David MacKenzie <djm@gnu.ai.mit.edu>. */ #if HAVE_CONFIG_H # include <config.h> #endif void * memmove (dest, source, length) char *dest; const char *source; unsigned length; { char *d0 = dest; if (source < dest) /* Moving from low mem to hi mem; start at end. */ for (source += length, dest += length; length; --length) *--dest = *--source; else if (source != dest) { /* Moving from hi mem to low mem; start at beginning. */ for (; length; --length) *dest++ = *source++; } return (void *) d0; }